Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 26, 2019, 18:56 UTC
- Message-ID
- <xmqqr20qlxtz.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <4a120fd0b32d2d6492eac6b0494ad6b1bc2ba500.1577382151.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 10 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > During a clone of a repository that contained a file with a backslash in > its name in the past, as of v2.24.1(2), Git for Windows prints errors > like this: > > error: filename in tree entry contains backslash: '\' > > While the clone still succeeds, a similar error prevents the equivalent > `git fetch` operation, which is inconsistent.
Yes, inconsistent is bad and it is puzzling. I would have expected, if this gate on the transport layer is desirable, such a check would be implemented as a part of transfer.fsckObjects and covered equally by fetch and clone codepaths. What went wrong to allow "clone" to go through while stopping "fetch"? Can you describe the root cause of the difference in the log message?
> Arguably, this is the wrong layer for that error, anyway: As long as the > user never checks out the files whose names contain backslashes, there > should not be any problem in the first place.
I do agree that rejecting these tree objects that has a slash in its path component is probably wrong. A GIT_WINDOWS_NATIVE box should be able to host a bare repository on it, and users on machines that are OK with paths that Windows may not like should be able to interact with it, by pushing to it, fetching from it, and updating the repository on that Windows box by going there and fetching from elsewhere. Rejecting these names at the object validity level means Git on Windows would be incompatible with Git elsewhere.
And It hink the same logic apply to those names like prn, con, nul, etc. How are the users protected from them? We should prevent these names from entering the index the same way, shouldn't we?
> So let's instead prevent such files to be added to the index.
... and loosen the check that (incorrectly) gets triggered from what codepaths in "git fetch" (but not from "git clone")?
Show 19 quoted lines
> This addresses https://github.com/git-for-windows/git/issues/2435 > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > read-cache.c | 5 +++++ > t/t7415-submodule-names.sh | 7 ++++--- > tree-walk.c | 6 ------ > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index ad0b48c84d..737916ebd9 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -1278,6 +1278,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e > int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK; > int new_only = option & ADD_CACHE_NEW_ONLY; > > +#ifdef GIT_WINDOWS_NATIVE > + if (protect_ntfs && strchr(ce->name, '\\'))
As I wondered above, names that must not enter the index may not be limited to those with backslashes in them. Perhaps you'd want a separate helper function so that you can extend the logic more easily, i.e.
if (protect_ntfs && invalid_name_on_windows(ce->name))
or something like that.
Show 17 quoted lines
> diff --git a/tree-walk.c b/tree-walk.c
> index b3d162051f..d5a8e096a6 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
> strbuf_addstr(err, _("empty filename in tree entry"));
> return -1;
> }
> -#ifdef GIT_WINDOWS_NATIVE
> - if (protect_ntfs && strchr(path, '\\')) {
> - strbuf_addf(err, _("filename in tree entry contains backslash: '%s'"), path);
> - return -1;
> - }
> -#endif
> len = strlen(path) + 1;
>
> /* Initialize the descriptor entry */