Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present

> Am 30.09.21 um 18:35 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Looping in Stolee as the author of 6e773527b6 (sparse-index: convert
>>> from full to sparse, 2021-03-30).

>>> Here's a raw patch for that.  Versions before 6e773527b6 pass the
>>> included test.
>>>
>>> The magic return value of 2 is a bit ugly, but allows adding the
>>> additional check only to the call-site relevant to the bug report.
>>>
>>> I don't know if other callers of verify_path() might also need that
>>> check, or if it is too narrow.
>>
>> The callers inside read-cache.c, which I think were the original
>> intended audiences of the function, always call verify_path() with
>> the pathname suitable to be stored as a cache entry.
>>
>> The callee never has to assume an extra trailing slash might exist
>> at the end.  And verify_path() said "no", if any caller passed an
>> extra trailing slash before the commit in question.
>>
>> I _think_ we defined the new "tree" type entries the sparse index
>> stuff added to be the _only_ cache entries whose path always ends
>> with a slash?  If so, perhaps we should audit the existing callers
>> and move the "paths that come from end-users to be entered to the
>> index must never end with a slash" sanity check this function gave
>> us, which was broken by 6e773527b6, perhaps?
>>
>> "git update-index" should never allow to create a "tree" kind of
>> cache entry (making it sparse again should be the task of systems
>> internal, and should not be done by end-user feeding a pre-shrunk
>> "tree" kind of entry to record a sparsely populated state, if I
>> understand correctly), so its call to verify_path(), if it ends with
>> a directory separator, should say "that's not a good path".
>>
>> Prehaps we can rename verify_path() to verify_path_internal() that
>> is _known_ to be called with names that are already vetted to be
>> stored in ce_name and convert callers inside read-cache.c to call
>> it, and write verify_path() as a thin wrapper of it to fail when the
>> former stops by returning S_ISDIR() at the place your fix touched,
>> or something?
> 
> Restoring the stricter check for the general case and providing a way
> out for the code handling sparse indexes sounds like a good idea.
> 
> I don't know which callers are supposed to need that, but the following
> patch passes all tests, including the new one.

> +enum verify_path_result {
> +	PATH_OK,
> +	PATH_INVALID,
> +	PATH_DIR_WITH_SEP,
> +};
> +
> +static enum verify_path_result verify_path_internal(const char *, unsigned);
> +
> +int verify_path(const char *path, unsigned mode)
> +{
> +	return verify_path_internal(path, mode) == PATH_OK;
> +}
> +

>  			/*
>  			 * allow terminating directory separators for
>  			 * sparse directory entries.
>  			 */
>  			if (c == '\0')
> -				return S_ISDIR(mode);
> +				return S_ISDIR(mode) ? PATH_DIR_WITH_SEP :
> +						       PATH_INVALID;

> @@ -1349,7 +1364,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
> 
>  	if (!ok_to_add)
>  		return -1;
> -	if (!verify_path(ce->name, ce->ce_mode))
> +	if (verify_path_internal(ce->name, ce->ce_mode) == PATH_INVALID)
>  		return error(_("invalid path '%s'"), ce->name);

> +test_expect_success 'stash -u ignores sub-repository' '
> +	test_when_finished "rm -rf sub-repo" &&
> +	git init sub-repo &&
> +	git stash -u
> +'