Re: [PATCH 1/1] mingw: only test index entries for backslashes, not tree entries

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 26, 2019, 20:03 UTC
Message-ID: <20191226200316.GD170890@google.com>
In-Reply-To: <4a120fd0b32d2d6492eac6b0494ad6b1bc2ba500.1577382151.git.gitgitgadget@gmail.com>

Hi,

Johannes Schindelin wrote:

Show 12 quoted lines

> 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?

Show 11 quoted lines

> --- 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?

[...]

Show 12 quoted lines

> --- 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.

Thanks and hope that helps, Jonathan