[2/2alt] dir: do not feed path suffix to pathspec match

Commit Message

Junio C Hamano July 7, 2023, 11:45 p.m. UTC

Junio C Hamano <[e-mail]> writes:

> ...
>  * do_match_pathspec() calls match_pathspec_item() _after_ stripping
>    the common prefix "sub/" from the path, giving it "file", plus
>    the length of the common prefix (4-bytes), so that the pathspec
>    element "(attr:label)sub/" can be treated as if it were "(attr:label)".
>
> The last one is what breaks the match, as the pathspec subsystem
> ends up asking the attribute subsystem to find the attribute
> attached to the path "file".
> ...
> Update do_match_pathspec() so that it does not strip the prefix from
> the path, and always feeding the full pathname to match_pathspec_item().

Here is an alternative approach with a lot smaller code footprint.
Instead of teaching do_match_pathspec() not to strip the common
prefix from the pathname, we teach match_pathspec_item() how to
recover the original pathname before stripping, and use that when
calling match_pathspec_attrs() function.  The same trick is already
used in an earlier part of the same function, so even though it
looks somewhat dirty, it is unlikely that it would introduce
more breakage.

As the test part is the same, I'll just show the code change
relative to the 'master' branch.

I am undecided which one is better.

 dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

René Scharfe July 8, 2023, 7:16 a.m. UTC | #1

Am 08.07.23 um 01:45 schrieb Junio C Hamano:
> Junio C Hamano <[e-mail]> writes:
>
>> ...
>>  * do_match_pathspec() calls match_pathspec_item() _after_ stripping
>>    the common prefix "sub/" from the path, giving it "file", plus
>>    the length of the common prefix (4-bytes), so that the pathspec
>>    element "(attr:label)sub/" can be treated as if it were "(attr:label)".
>>
>> The last one is what breaks the match, as the pathspec subsystem
>> ends up asking the attribute subsystem to find the attribute
>> attached to the path "file".
>> ...
>> Update do_match_pathspec() so that it does not strip the prefix from
>> the path, and always feeding the full pathname to match_pathspec_item().
>
> Here is an alternative approach with a lot smaller code footprint.
> Instead of teaching do_match_pathspec() not to strip the common
> prefix from the pathname, we teach match_pathspec_item() how to
> recover the original pathname before stripping, and use that when
> calling match_pathspec_attrs() function.  The same trick is already
> used in an earlier part of the same function, so even though it
> looks somewhat dirty, it is unlikely that it would introduce
> more breakage.
>
> As the test part is the same, I'll just show the code change
> relative to the 'master' branch.
>
> I am undecided which one is better.
>
>  dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git c/dir.c w/dir.c
> index a7469df3ac..635d1b058c 100644
> --- c/dir.c
> +++ w/dir.c
> @@ -374,7 +374,7 @@ static int match_pathspec_item(struct index_state *istate,
>  		return 0;
>
>  	if (item->attr_match_nr &&
> -	    !match_pathspec_attrs(istate, name, namelen, item))
> +	    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))

match_pathspec_item() has only one caller, and it did the opposite, so
this is safe.  And a minimal fix like that is less likely to have side
effects.  Removing the trick will surely improve the code, though.  If
match_pathspec_item() needs the full name then we should pass it on,
and if the "prefix" offset needs to be added then it can happen right
there in that function.

>  		return 0;
>
>  	/* If the match was just the prefix, we matched */

Patch

diff --git c/dir.c w/dir.c
index a7469df3ac..635d1b058c 100644
--- c/dir.c
+++ w/dir.c
@@ -374,7 +374,7 @@  static int match_pathspec_item(struct index_state *istate,
 		return 0;
 
 	if (item->attr_match_nr &&
-	    !match_pathspec_attrs(istate, name, namelen, item))
+	    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))
 		return 0;
 
 	/* If the match was just the prefix, we matched */