Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inconsistent error message for windows file system invalid chars on create new branch #2907

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

whitneyschmidt
Copy link

Fixes #2904

Validation:
Before:

$ git checkout -b "|"
fatal: cannot lock ref 'refs/heads/|': Unable to create 'C:/Users/whschm/source/repos/TestRepo/.git/refs/heads/|.lock': Invalid argument

After:

$ ./bin-wrappers/git --exec-path=/usr/src/git checkout -b "|"
fatal: '|' is not a valid branch name.

Thanks for taking the time to contribute to Git!

Those seeking to contribute to the Git for Windows fork should see
http://gitforwindows.org/#contribute on how to contribute Windows specific
enhancements.

If your contribution is for the core Git functions and documentation
please be aware that the Git community does not use the github.com issues
or pull request mechanism for their contributions.

Instead, we use the Git mailing list ([email protected]) for code and
documenatation submissions, code reviews, and bug reports. The
mailing list is plain text only (anything with HTML is sent directly
to the spam folder).

Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

…mes when creating a new branch

Signed-off-by: Whitney Schmidt <[email protected]>
@PhilipOakley
Copy link

Is it possible to tweak the error message in some way to clarify that the problem relates to the local OS.

Also there looks to be some tests that may need a PREREQ check so that they don't fail the CI tests. (these may be due to a recent security fix on GitHub, so ..)

@dscho
Copy link
Member

dscho commented Nov 23, 2020

Is it possible to tweak the error message in some way to clarify that the problem relates to the local OS.

Also there looks to be some tests that may need a PREREQ check so that they don't fail the CI tests. (these may be due to a recent security fix on GitHub, so ..)

@PhilipOakley unfortunately, this would require deeper changes than I am prepared to ask for. See for yourself: check_refname_format() is expected to be silent, it returns 0 upon success, -1 upon failure. It does not give any indication of the type of issue when the refname was deemed invalid.

@PhilipOakley
Copy link

Is it possible to tweak the error message in some way to clarify that the problem relates to the local OS.
Also there looks to be some tests that may need a PREREQ check so that they don't fail the CI tests. (these may be due to a recent security fix on GitHub, so ..)

@PhilipOakley unfortunately, this would require deeper changes than I am prepared to ask for. See for yourself: check_refname_format() is expected to be silent, it returns 0 upon success, -1 upon failure. It does not give any indication of the type of issue when the refname was deemed invalid.

That silentness could be mentioned in the commit message, and/or the place where the error message is generated might be able to get a compilation (Git for Windows) specific tweak, just so we don't get more folks thinking that Git for Windows is faulty (in new ways) because the branch name is 'valid', it's just the OS that can't cope, or it could be punted on with a short note in the commit message.

It's still a worthwhile contribution though.

@dscho
Copy link
Member

dscho commented Nov 23, 2020

@whitneyschmidt sadly, this seems to break the PR build:

fatal: invalid refspec '+refs/heads/*:refs/remotes/origin/*'

This error message originates from https://github.com/git/git/blob/v2.29.2/refspec.c#L167, and the reason is that it tries to validate the left-hand side refs/heads/* as a valid ref, and then the right-hand side, too, which is of course not quite correct, as those are not even intended to be refs, but ref patterns.

I guess a valid way forward would be to patch is_valid_win32_path() (and is_valid_path()) to accept a function parameter, say, allow_glob, and then patch https://github.com/git/git/blob/v2.29.2/compat/mingw.c#L2711 to conditionally break; when encountering a question mark or a star if allow_glob is non-zero.

We should also think about adding a test case or two, to verify that the code introduced by this PR does what it is supposed to do, and keeps doing it even in the future. I am thinking about something like invalid_ref MINGW "p|pe" in t/t1402-check-ref-format.sh. (See how to run individual test scripts in Git's test suite).

@whitneyschmidt
Copy link
Author

whitneyschmidt commented Nov 24, 2020

@dscho Thanks for the guidance! I'll take a look at this over the next few days and get back to you with any questions that come up.

@pinguin999
Copy link

The Problem is a little bit more complex than only the "Windows create branch side".

On Unix Systems it's ok to create branches with names including < and push it.
Than a Windows user can not fetch the repo anymore.

I think escape the special characters for the Windows path could work. Or git has to disallow create branches with special characters on all systems.

@dscho
Copy link
Member

dscho commented May 13, 2022

@whitneyschmidt are you still interested in pursuing this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear error handling when trying to create a branch with Windows filesystem disallowed characters < > | "
4 participants