Hi Jonathan, On Thu, 26 Dec 2019, Jonathan Nieder wrote: > Johannes Schindelin wrote: > > > 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. > > > > 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. > > Hm. The choice of right layer depends on what repositories in the wild > contain. If there are none containing filenames with '\', then fsck et > al would be an appropriate layer for this. With hindsight, it was not > a good idea to support this kind of filename. > > However, between the lines of this commit messages I sense that there > *are* repositories in the wild using these kinds of filenames. > > Can you say more about that? What repositories are affected? Do they > contain such filenames at HEAD or only in their history? If someone > wants to check out a revision with such filenames, what should happen? Yes: https://github.com/zephyrproject-rtos/civetweb contains history where at some stage, by mistake, a directory was called `\`. It has been fixed a long time ago, but users obviously still want to be able to clone it ;-) > > --- 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, '\\')) > > + return error(_("filename in tree entry contains backslash: '%s'"), ce->name); > > +#endif > > + > > Why is this specific to the GIT_WINDOWS_NATIVE case? Wouldn't it affect > ntfs usage on other platforms as well? > > [...] > > --- 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 > > Ah, it's inherited from there, so orthogonal to this patch. > > To summarize: I think the commit message and docs could use some work > to describe what invariants we're trying to maintain and what > real-world usage motivates them. I added this paragraph to the commit message: Note: just as before, the check is guarded by `core.protectNTFS` (to allow overriding the check by toggling that config setting), and it is _only_ performed on Windows, as the backslash is not a directory separator elsewhere, even when writing to NTFS-formatted volumes. Does that clarify the issue enough? Dscho > > Thanks and hope that helps, > Jonathan > >
