{"thread":{"id":"59963","subject":"[PATCH 2/2] dir: do not feed path suffix to pathspec match","startedAt":"2023-07-07T22:05:12Z","lastAt":"2023-07-09T09:30:04Z","messageCount":9,"participants":["Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"479297","messageId":"20230707220457.3655121-3-gitster@pobox.com","threadId":"59963","inReplyTo":"20230707220457.3655121-1-gitster@pobox.com","subject":"[PATCH 2/2] dir: do not feed path suffix to pathspec match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-07T22:04:57Z","receivedAt":"2023-07-07T22:05:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"match_pathspec_item() takes \"prefix\" value, assuming that it is OK\nfor a caller to chop off the common leading prefix of pathspec\nstrings from the path and only allow the remainder of the path to\nmatch the pathspec pattern (after chopping the same leading prefix\nof it, of course).\n\nThe \"common leading prefix\" optimization has two main features:\n\n * discard the entries in the in-core index that are outside of the\n   common leading prefix; if you are doing \"ls-files one/a one/b\",\n   we know all matches must be from \"one/\", so first the code\n   discards all entries outside the \"one/\" directory from the\n   in-core index.  This allows us to work on a smaller dataset.\n\n * allow skipping the comparison of a few leading bytes when\n   matching pathspec with path.  When \"ls-files\" finds the path\n   \"one/a/1\" in the in-core index given \"one/a\" and \"one/b\" as the\n   pathspec, knowing that common leading prefix \"one/\" was found\n   lets the pathspec matchiner not to bother comparing \"one/\" part,\n   and allows it to feed \"a/1\" down, as long as the pathspec element\n   \"one/a\" gets corresponding adjustment to \"a\".\n\nHowever, losing the full path in the repository too early in the\ncallchain (at dir.c:do_match_pathspec()) and feeding only the suffix\nto the low level code (i.e. dir.c:match_pathspec_item()) loses a\ncrucial piece of infomation that is needed by the pathspec matching\ncode, when the attribute magic is involved.  The attributes, other\nthan the ones that are built-in and the ones that come from the\n$GIT_DIR/info/attributes file and the top-level .gitattributes file,\nare lazily read on-demand, as we encounter each path and ask if it\nmatches the pathspec.\n\nFor example, if you say \"git ls-files \"(attr:label)sub/\" in a\nrepository with a file \"sub/file\" that is given the 'label'\nattribute in sub/.gitattributes:\n\n * The common prefix optimization finds that \"sub/\" is the common\n   prefix and prunes the in-core index so that it has only entries\n   inside that directory.  This is desirable.\n\n * The code then walks the in-core index, finds \"sub/file\", and\n   eventually asks do_match_pathspec() if it matches the given\n   pathspec.\n\n * do_match_pathspec() calls match_pathspec_item() _after_ stripping\n   the common prefix \"sub/\" from the path, giving it \"file\", plus\n   the length of the common prefix (4-bytes), so that the pathspec\n   element \"(attr:label)sub/\" can be treated as if it were \"(attr:label)\".\n\nThe last one is what breaks the match, as the pathspec subsystem\nends up asking the attribute subsystem to find the attribute\nattached to the path \"file\".\n\nUnfortunately this was not discovered so far because the code works\nwell if we do not trigger the common prefix optimization, e.g.\n\n\tgit ls-files \"(attr:label)sub\"\n\tgit ls-files \"(attr:label)sub/\" \"(attr:label)dir/\"\n\nwould have reported \"sub/file\" as a path with the 'label' attribute\njust fine.\n\nUpdate do_match_pathspec() so that it does not strip the prefix from\nthe path, and always feeding the full pathname to match_pathspec_item().\n\nReported-by: Matthew Hughes <mhughes@uw.co.uk>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n dir.c                          | 31 ++++++++-----------------------\n t/t6135-pathspec-with-attrs.sh | 24 +++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 24 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 3acac7beb1..6116022ae6 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -337,12 +337,11 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n  * [2] Only if DO_MATCH_LEADING_PATHSPEC is passed; otherwise, not a match.\n  */\n static int match_pathspec_item(struct index_state *istate,\n-\t\t\t       const struct pathspec_item *item, int prefix,\n+\t\t\t       const struct pathspec_item *item,\n \t\t\t       const char *name, int namelen, unsigned flags)\n {\n-\t/* name/namelen has prefix cut off by caller */\n-\tconst char *match = item->match + prefix;\n-\tint matchlen = item->len - prefix;\n+\tconst char *match = item->match;\n+\tint matchlen = item->len;\n \n \t/*\n \t * The normal call pattern is:\n@@ -362,19 +361,9 @@ static int match_pathspec_item(struct index_state *istate,\n \t * other words, we do not trust the caller on comparing the\n \t * prefix part when :(icase) is involved. We do exact\n \t * comparison ourselves.\n-\t *\n-\t * Normally the caller (common_prefix_len() in fact) does\n-\t * _exact_ matching on name[-prefix+1..-1] and we do not need\n-\t * to check that part. Be defensive and check it anyway, in\n-\t * case common_prefix_len is changed, or a new caller is\n-\t * introduced that does not use common_prefix_len.\n-\t *\n-\t * If the penalty turns out too high when prefix is really\n-\t * long, maybe change it to\n-\t * strncmp(match, name, item->prefix - prefix)\n \t */\n \tif (item->prefix && (item->magic & PATHSPEC_ICASE) &&\n-\t    strncmp(item->match, name - prefix, item->prefix))\n+\t    strncmp(item->match, name, item->prefix))\n \t\treturn 0;\n \n \tif (item->attr_match_nr &&\n@@ -399,7 +388,7 @@ static int match_pathspec_item(struct index_state *istate,\n \n \tif (item->nowildcard_len < item->len &&\n \t    !git_fnmatch(item, match, name,\n-\t\t\t item->nowildcard_len - prefix))\n+\t\t\t item->nowildcard_len))\n \t\treturn MATCHED_FNMATCH;\n \n \t/* Perform checks to see if \"name\" is a leading string of the pathspec */\n@@ -414,8 +403,7 @@ static int match_pathspec_item(struct index_state *istate,\n \n \t\t/* name doesn't match up to the first wild character */\n \t\tif (item->nowildcard_len < item->len &&\n-\t\t    ps_strncmp(item, match, name,\n-\t\t\t       item->nowildcard_len - prefix))\n+\t\t    ps_strncmp(item, match, name, item->nowildcard_len))\n \t\t\treturn 0;\n \n \t\t/*\n@@ -488,9 +476,6 @@ static int do_match_pathspec(struct index_state *istate,\n \t\t\treturn 0;\n \t}\n \n-\tname += prefix;\n-\tnamelen -= prefix;\n-\n \tfor (i = ps->nr - 1; i >= 0; i--) {\n \t\tint how;\n \n@@ -506,8 +491,8 @@ static int do_match_pathspec(struct index_state *istate,\n \t\t */\n \t\tif (seen && ps->items[i].magic & PATHSPEC_EXCLUDE)\n \t\t\tseen[i] = MATCHED_FNMATCH;\n-\t\thow = match_pathspec_item(istate, ps->items+i, prefix, name,\n-\t\t\t\t\t  namelen, flags);\n+\t\thow = match_pathspec_item(istate, ps->items+i,\n+\t\t\t\t\t  name, namelen, flags);\n \t\tif (ps->recursive &&\n \t\t    (ps->magic & PATHSPEC_MAXDEPTH) &&\n \t\t    ps->max_depth != -1 &&\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f63774094f..f70c395e75 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -65,7 +65,8 @@ test_expect_success 'setup .gitattributes' '\n \tfileValue label=foo\n \tfileWrongLabel label☺\n \tEOF\n-\tgit add .gitattributes &&\n+\techo fileSetLabel label1 >sub/.gitattributes &&\n+\tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n@@ -157,6 +158,7 @@ test_expect_success 'check unspecified attr' '\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n+\tsub/.gitattributes\n \tsub/fileA\n \tsub/fileAB\n \tsub/fileAC\n@@ -181,6 +183,7 @@ test_expect_success 'check unspecified attr (2)' '\n \tHEAD:fileC\n \tHEAD:fileNoLabel\n \tHEAD:fileWrongLabel\n+\tHEAD:sub/.gitattributes\n \tHEAD:sub/fileA\n \tHEAD:sub/fileAB\n \tHEAD:sub/fileAC\n@@ -200,6 +203,7 @@ test_expect_success 'check multiple unspecified attr' '\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n+\tsub/.gitattributes\n \tsub/fileC\n \tsub/fileNoLabel\n \tsub/fileWrongLabel\n@@ -273,4 +277,22 @@ test_expect_success 'backslash cannot be used as a value' '\n \ttest_i18ngrep \"for value matching\" actual\n '\n \n+test_expect_success 'reading from .gitattributes in a subdirectory (1)' '\n+\tgit ls-files \":(attr:label1)\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reading from .gitattributes in a subdirectory (2)' '\n+\tgit ls-files \":(attr:label1)sub\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n+\tgit ls-files \":(attr:label1)sub/\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.41.0-318-g061c58647e\n\n"},{"id":"479298","messageId":"20230707220457.3655121-1-gitster@pobox.com","threadId":"59963","inReplyTo":null,"subject":"[PATCH 0/2] Fix attr magic combined with pathspec prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-07T22:04:55Z","receivedAt":"2023-07-07T22:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Hughes noticed and reported that a pathspec that uses the\nattribute magic with pathspec pattern does not work correctly.\n\nAfter digging around, I found that\n\n    $ git ls-files \":(attr:label)\"\n\nnotices that the \"label\" attribute is set to path \"sub/file\" by\nlisting it, and combined with a pathspec pattern, i.e.\n\n    $ git ls-files \":(attr:label)sub\"\n\nit still correctly reports \"sub/file\" has the \"label\" attribute, be\nit defined in \".gitignore\" or \"sub/.gitignore\".  The case that it\ndoes not work is the command invocation is\n\n    $ git ls-files \":(attr:label)sub/\"\n\nand the attribute \"label\" is defined in \"sub/.gitattributes\" for\n\"sub/file\".\n\nIt turns out that the problematic invocation triggers the common\nprefix optimization, which is totally broken for this case.\n\nThe first patch enhances the test coverage, and the second patch\nfixes the broken common prefix optimization.\n\nJunio C Hamano (2):\n  t6135: attr magic with path pattern\n  dir: do not feed path suffix to pathspec match\n\n dir.c                          | 31 ++++++-----------------\n t/t6135-pathspec-with-attrs.sh | 46 ++++++++++++++++++++++++++++++++--\n 2 files changed, 52 insertions(+), 25 deletions(-)\n\n-- \n2.41.0-318-g061c58647e\n\n"},{"id":"479299","messageId":"20230707220457.3655121-2-gitster@pobox.com","threadId":"59963","inReplyTo":"20230707220457.3655121-1-gitster@pobox.com","subject":"[PATCH 1/2] t6135: attr magic with path pattern","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-07T22:04:56Z","receivedAt":"2023-07-07T22:10:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The test coverage on attribute magic combined with path pattern\nwas a bit thin.  Let's add a few and make sure \"(attr:X)sub\" and\n\"(attr:X)sub/\" behave the same.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t6135-pathspec-with-attrs.sh | 22 +++++++++++++++++++++-\n 1 file changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 457cc167c7..f63774094f 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -78,7 +78,17 @@ test_expect_success 'check specific set attr' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'check specific set attr (2)' '\n+test_expect_success 'check set attr with pathspec pattern' '\n+\techo sub/fileSetLabel >expect &&\n+\n+\tgit ls-files \":(attr:label)sub\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit ls-files \":(attr:label)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check specific set attr in tree-ish' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:fileSetLabel\n \tHEAD:sub/fileSetLabel\n@@ -87,6 +97,16 @@ test_expect_success 'check specific set attr (2)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check specific set attr with pathspec pattern in tree-ish' '\n+\techo HEAD:sub/fileSetLabel >expect &&\n+\n+\tgit grep -l content HEAD \":(attr:label)sub\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit grep -l content HEAD \":(attr:label)sub/\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'check specific unset attr' '\n \tcat <<-\\EOF >expect &&\n \tfileUnsetLabel\n-- \n2.41.0-318-g061c58647e\n\n"},{"id":"479304","messageId":"xmqqttuf70bn.fsf_-_@gitster.g","threadId":"59963","inReplyTo":"20230707220457.3655121-3-gitster@pobox.com","subject":"[PATCH 2/2alt] dir: do not feed path suffix to pathspec match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-07T23:45:32Z","receivedAt":"2023-07-07T23:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...\n>  * do_match_pathspec() calls match_pathspec_item() _after_ stripping\n>    the common prefix \"sub/\" from the path, giving it \"file\", plus\n>    the length of the common prefix (4-bytes), so that the pathspec\n>    element \"(attr:label)sub/\" can be treated as if it were \"(attr:label)\".\n>\n> The last one is what breaks the match, as the pathspec subsystem\n> ends up asking the attribute subsystem to find the attribute\n> attached to the path \"file\".\n> ...\n> Update do_match_pathspec() so that it does not strip the prefix from\n> the path, and always feeding the full pathname to match_pathspec_item().\n\nHere is an alternative approach with a lot smaller code footprint.\nInstead of teaching do_match_pathspec() not to strip the common\nprefix from the pathname, we teach match_pathspec_item() how to\nrecover the original pathname before stripping, and use that when\ncalling match_pathspec_attrs() function.  The same trick is already\nused in an earlier part of the same function, so even though it\nlooks somewhat dirty, it is unlikely that it would introduce\nmore breakage.\n\nAs the test part is the same, I'll just show the code change\nrelative to the 'master' branch.\n\nI am undecided which one is better.\n\n dir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/dir.c w/dir.c\nindex a7469df3ac..635d1b058c 100644\n--- c/dir.c\n+++ w/dir.c\n@@ -374,7 +374,7 @@ static int match_pathspec_item(struct index_state *istate,\n \t\treturn 0;\n \n \tif (item->attr_match_nr &&\n-\t    !match_pathspec_attrs(istate, name, namelen, item))\n+\t    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n \t\treturn 0;\n \n \t/* If the match was just the prefix, we matched */\n"},{"id":"479321","messageId":"f592e7c9-2a73-41f3-02a5-d77b7be574f1@web.de","threadId":"59963","inReplyTo":"20230707220457.3655121-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] dir: do not feed path suffix to pathspec match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-08T07:16:14Z","receivedAt":"2023-07-08T07:16:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.23 um 00:04 schrieb Junio C Hamano:\n> diff --git a/dir.c b/dir.c\n> index 3acac7beb1..6116022ae6 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -488,9 +476,6 @@ static int do_match_pathspec(struct index_state *istate,\n>  \t\t\treturn 0;\n>  \t}\n>\n> -\tname += prefix;\n> -\tnamelen -= prefix;\n> -\n>  \tfor (i = ps->nr - 1; i >= 0; i--) {\n>  \t\tint how;\n>\n> @@ -506,8 +491,8 @@ static int do_match_pathspec(struct index_state *istate,\n>  \t\t */\n>  \t\tif (seen && ps->items[i].magic & PATHSPEC_EXCLUDE)\n>  \t\t\tseen[i] = MATCHED_FNMATCH;\n> -\t\thow = match_pathspec_item(istate, ps->items+i, prefix, name,\n> -\t\t\t\t\t  namelen, flags);\n> +\t\thow = match_pathspec_item(istate, ps->items+i,\n> +\t\t\t\t\t  name, namelen, flags);\n\nWith that, the parameter \"prefix\" of do_match_pathspec() becomes unused\nand can be removed.  This cascades to match_pathspec_with_flags(),\nmatch_pathspec(), dir_path_match() and builtin/add.c::prune_directory(),\nand fill_directory() can lose its return value.\n\nThe code continues here like this, though:\n\n                if (ps->recursive &&\n                    (ps->magic & PATHSPEC_MAXDEPTH) &&\n                    ps->max_depth != -1 &&\n                    how && how != MATCHED_FNMATCH) {\n                        int len = ps->items[i].len;\n                        if (name[len] == '/')\n                                len++;\n                        if (within_depth(name+len, namelen-len, 0, ps->max_depth))\n                                how = MATCHED_EXACTLY;\n                        else\n                                how = 0;\n                }\n\nAnd \"name\" here would be affected by \"prefix\" no longer being added.\nDoes it fix or break --max-depth?  I think neither: builtin/grep.c --\nthe only user of PATHSPEC_MAXDEPTH AFAICS -- passes a prefix of 0.\n\nRené\n\n"},{"id":"479322","messageId":"26f81ed0-8d1f-e0be-9696-25ce3938ae09@web.de","threadId":"59963","inReplyTo":"xmqqttuf70bn.fsf_-_@gitster.g","subject":"Re: [PATCH 2/2alt] dir: do not feed path suffix to pathspec match","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-08T07:16:17Z","receivedAt":"2023-07-08T07:16:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.23 um 01:45 schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> ...\n>>  * do_match_pathspec() calls match_pathspec_item() _after_ stripping\n>>    the common prefix \"sub/\" from the path, giving it \"file\", plus\n>>    the length of the common prefix (4-bytes), so that the pathspec\n>>    element \"(attr:label)sub/\" can be treated as if it were \"(attr:label)\".\n>>\n>> The last one is what breaks the match, as the pathspec subsystem\n>> ends up asking the attribute subsystem to find the attribute\n>> attached to the path \"file\".\n>> ...\n>> Update do_match_pathspec() so that it does not strip the prefix from\n>> the path, and always feeding the full pathname to match_pathspec_item().\n>\n> Here is an alternative approach with a lot smaller code footprint.\n> Instead of teaching do_match_pathspec() not to strip the common\n> prefix from the pathname, we teach match_pathspec_item() how to\n> recover the original pathname before stripping, and use that when\n> calling match_pathspec_attrs() function.  The same trick is already\n> used in an earlier part of the same function, so even though it\n> looks somewhat dirty, it is unlikely that it would introduce\n> more breakage.\n>\n> As the test part is the same, I'll just show the code change\n> relative to the 'master' branch.\n>\n> I am undecided which one is better.\n>\n>  dir.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git c/dir.c w/dir.c\n> index a7469df3ac..635d1b058c 100644\n> --- c/dir.c\n> +++ w/dir.c\n> @@ -374,7 +374,7 @@ static int match_pathspec_item(struct index_state *istate,\n>  \t\treturn 0;\n>\n>  \tif (item->attr_match_nr &&\n> -\t    !match_pathspec_attrs(istate, name, namelen, item))\n> +\t    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n\nmatch_pathspec_item() has only one caller, and it did the opposite, so\nthis is safe.  And a minimal fix like that is less likely to have side\neffects.  Removing the trick will surely improve the code, though.  If\nmatch_pathspec_item() needs the full name then we should pass it on,\nand if the \"prefix\" offset needs to be added then it can happen right\nthere in that function.\n\n>  \t\treturn 0;\n>\n>  \t/* If the match was just the prefix, we matched */\n\n"},{"id":"479335","messageId":"xmqqh6qe5boa.fsf@gitster.g","threadId":"59963","inReplyTo":"26f81ed0-8d1f-e0be-9696-25ce3938ae09@web.de","subject":"[PATCH 2alt/2] dir: match \"attr\" pathspec magic with correct paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-08T21:35:33Z","receivedAt":"2023-07-08T21:35:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>>  \tif (item->attr_match_nr &&\n>> -\t    !match_pathspec_attrs(istate, name, namelen, item))\n>> +\t    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n>\n> match_pathspec_item() has only one caller, and it did the opposite, so\n> this is safe.  And a minimal fix like that is less likely to have side\n> effects.  Removing the trick will surely improve the code, though.  If\n> match_pathspec_item() needs the full name then we should pass it on,\n> and if the \"prefix\" offset needs to be added then it can happen right\n> there in that function.\n\nYup.  I am inclined to take this version and then update the\nproposed log message to put less blame on the \"common prefix\"\noptimization in general.\n\nThanks.\n\nJust for completeness, this is with an updated log message.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nThe match_pathspec_item() function takes \"prefix\" value, allowing a\ncaller to chop off the common leading prefix of pathspec pattern\nstrings from the path and only use the remainder of the path to\nmatch the pathspec patterns (after chopping the same leading prefix\nof them, of course).\n\nThis \"common leading prefix\" optimization has two main features:\n\n * discard the entries in the in-core index that are outside of the\n   common leading prefix; if you are doing \"ls-files one/a one/b\",\n   we know all matches must be from \"one/\", so first the code\n   discards all entries outside the \"one/\" directory from the\n   in-core index.  This allows us to work on a smaller dataset.\n\n * allow skipping the comparison of the leading bytes when matching\n   pathspec with path.  When \"ls-files\" finds the path \"one/a/1\" in\n   the in-core index given \"one/a\" and \"one/b\" as the pathspec,\n   knowing that common leading prefix \"one/\" was found lets the\n   pathspec matchinery not to bother comparing \"one/\" part, and\n   allows it to feed \"a/1\" down, as long as the pathspec element\n   \"one/a\" gets corresponding adjustment to \"a\".\n\nWhen the \"attr\" pathspec magic is in effect, however, the current\ncode breaks down.\n\nThe attributes, other than the ones that are built-in and the ones\nthat come from the $GIT_DIR/info/attributes file and the top-level\n.gitattributes file, are lazily read from the filesystem on-demand,\nas we encounter each path and ask if it matches the pathspec.  For\nexample, if you say \"git ls-files \"(attr:label)sub/\" in a repository\nwith a file \"sub/file\" that is given the 'label' attribute in\n\"sub/.gitattributes\":\n\n * The common prefix optimization finds that \"sub/\" is the common\n   prefix and prunes the in-core index so that it has only entries\n   inside that directory.  This is desirable.\n\n * The code then walks the in-core index, finds \"sub/file\", and\n   eventually asks do_match_pathspec() if it matches the given\n   pathspec.\n\n * do_match_pathspec() calls match_pathspec_item() _after_ stripping\n   the common prefix \"sub/\" from the path, giving it \"file\", plus\n   the length of the common prefix (4-bytes), so that the pathspec\n   element \"(attr:label)sub/\" can be treated as if it were \"(attr:label)\".\n\nThe last one is what breaks the match in the current code, as the\npathspec subsystem ends up asking the attribute subsystem to find\nthe attribute attached to the path \"file\".  We need to ask about the\nattributes on \"sub/file\" when calling match_pathspec_attrs(); this\ncan be done by looking at \"prefix\" bytes before the beginning of\n\"name\", which is the same trick already used by another piece of the\ncode in the same match_pathspec_item() function.\n\nUnfortunately this was not discovered so far because the code works\nwith slightly different arguments, e.g.\n\n $ git ls-files \"(attr:label)sub\"\n $ git ls-files \"(attr:label)sub/\" \"no/such/dir/\"\n\nwould have reported \"sub/file\" as a path with the 'label' attribute\njust fine, because neither would trigger the common prefix\noptimization.\n\nReported-by: Matthew Hughes <mhughes@uw.co.uk>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n dir.c                          |  2 +-\n t/t6135-pathspec-with-attrs.sh | 24 +++++++++++++++++++++++-\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex a7469df3ac..635d1b058c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -374,7 +374,7 @@ static int match_pathspec_item(struct index_state *istate,\n \t\treturn 0;\n \n \tif (item->attr_match_nr &&\n-\t    !match_pathspec_attrs(istate, name, namelen, item))\n+\t    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n \t\treturn 0;\n \n \t/* If the match was just the prefix, we matched */\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f63774094f..f70c395e75 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -65,7 +65,8 @@ test_expect_success 'setup .gitattributes' '\n \tfileValue label=foo\n \tfileWrongLabel label☺\n \tEOF\n-\tgit add .gitattributes &&\n+\techo fileSetLabel label1 >sub/.gitattributes &&\n+\tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n@@ -157,6 +158,7 @@ test_expect_success 'check unspecified attr' '\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n+\tsub/.gitattributes\n \tsub/fileA\n \tsub/fileAB\n \tsub/fileAC\n@@ -181,6 +183,7 @@ test_expect_success 'check unspecified attr (2)' '\n \tHEAD:fileC\n \tHEAD:fileNoLabel\n \tHEAD:fileWrongLabel\n+\tHEAD:sub/.gitattributes\n \tHEAD:sub/fileA\n \tHEAD:sub/fileAB\n \tHEAD:sub/fileAC\n@@ -200,6 +203,7 @@ test_expect_success 'check multiple unspecified attr' '\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n+\tsub/.gitattributes\n \tsub/fileC\n \tsub/fileNoLabel\n \tsub/fileWrongLabel\n@@ -273,4 +277,22 @@ test_expect_success 'backslash cannot be used as a value' '\n \ttest_i18ngrep \"for value matching\" actual\n '\n \n+test_expect_success 'reading from .gitattributes in a subdirectory (1)' '\n+\tgit ls-files \":(attr:label1)\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reading from .gitattributes in a subdirectory (2)' '\n+\tgit ls-files \":(attr:label1)sub\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reading from .gitattributes in a subdirectory (3)' '\n+\tgit ls-files \":(attr:label1)sub/\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.41.0-327-gaa9166bcc0\n\n\n"},{"id":"479338","messageId":"39d5e14d-f2e9-e619-c3e8-e63c7547370b@web.de","threadId":"59963","inReplyTo":"xmqqh6qe5boa.fsf@gitster.g","subject":"Re: [PATCH 2alt/2] dir: match \"attr\" pathspec magic with correct paths","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-09T05:35:59Z","receivedAt":"2023-07-09T05:36:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.23 um 23:35 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>>>  \tif (item->attr_match_nr &&\n>>> -\t    !match_pathspec_attrs(istate, name, namelen, item))\n>>> +\t    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n>>\n>> match_pathspec_item() has only one caller, and it did the opposite, so\n>> this is safe.  And a minimal fix like that is less likely to have side\n>> effects.  Removing the trick will surely improve the code, though.  If\n>> match_pathspec_item() needs the full name then we should pass it on,\n>> and if the \"prefix\" offset needs to be added then it can happen right\n>> there in that function.\n>\n> Yup.  I am inclined to take this version and then update the\n> proposed log message to put less blame on the \"common prefix\"\n> optimization in general.\n>\n> Thanks.\n>\n> Just for completeness, this is with an updated log message.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> The match_pathspec_item() function takes \"prefix\" value, allowing a\n> caller to chop off the common leading prefix of pathspec pattern\n> strings from the path and only use the remainder of the path to\n> match the pathspec patterns (after chopping the same leading prefix\n> of them, of course).\n>\n> This \"common leading prefix\" optimization has two main features:\n>\n>  * discard the entries in the in-core index that are outside of the\n>    common leading prefix; if you are doing \"ls-files one/a one/b\",\n>    we know all matches must be from \"one/\", so first the code\n>    discards all entries outside the \"one/\" directory from the\n>    in-core index.  This allows us to work on a smaller dataset.\n>\n>  * allow skipping the comparison of the leading bytes when matching\n>    pathspec with path.  When \"ls-files\" finds the path \"one/a/1\" in\n>    the in-core index given \"one/a\" and \"one/b\" as the pathspec,\n>    knowing that common leading prefix \"one/\" was found lets the\n>    pathspec matchinery not to bother comparing \"one/\" part, and\n>    allows it to feed \"a/1\" down, as long as the pathspec element\n>    \"one/a\" gets corresponding adjustment to \"a\".\n>\n> When the \"attr\" pathspec magic is in effect, however, the current\n> code breaks down.\n>\n> The attributes, other than the ones that are built-in and the ones\n> that come from the $GIT_DIR/info/attributes file and the top-level\n> .gitattributes file, are lazily read from the filesystem on-demand,\n> as we encounter each path and ask if it matches the pathspec.  For\n> example, if you say \"git ls-files \"(attr:label)sub/\" in a repository\n> with a file \"sub/file\" that is given the 'label' attribute in\n> \"sub/.gitattributes\":\n>\n>  * The common prefix optimization finds that \"sub/\" is the common\n>    prefix and prunes the in-core index so that it has only entries\n>    inside that directory.  This is desirable.\n>\n>  * The code then walks the in-core index, finds \"sub/file\", and\n>    eventually asks do_match_pathspec() if it matches the given\n>    pathspec.\n>\n>  * do_match_pathspec() calls match_pathspec_item() _after_ stripping\n>    the common prefix \"sub/\" from the path, giving it \"file\", plus\n>    the length of the common prefix (4-bytes), so that the pathspec\n>    element \"(attr:label)sub/\" can be treated as if it were \"(attr:label)\".\n>\n> The last one is what breaks the match in the current code, as the\n> pathspec subsystem ends up asking the attribute subsystem to find\n> the attribute attached to the path \"file\".  We need to ask about the\n> attributes on \"sub/file\" when calling match_pathspec_attrs(); this\n> can be done by looking at \"prefix\" bytes before the beginning of\n> \"name\", which is the same trick already used by another piece of the\n> code in the same match_pathspec_item() function.\n>\n> Unfortunately this was not discovered so far because the code works\n> with slightly different arguments, e.g.\n>\n>  $ git ls-files \"(attr:label)sub\"\n>  $ git ls-files \"(attr:label)sub/\" \"no/such/dir/\"\n>\n> would have reported \"sub/file\" as a path with the 'label' attribute\n> just fine, because neither would trigger the common prefix\n> optimization.\n\nMakes me wonder how important this optimization is, when this flaw went\nunnoticed for ten years.\n\nUsing the latest main against on an old Chromium repository, because\nit has lots of files:\n\n   Benchmark 1: ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform\n     Time (mean ± σ):      37.8 ms ±   0.2 ms    [User: 28.3 ms, System: 8.4 ms]\n     Range (min … max):    37.4 ms …  38.7 ms    74 runs\n\n   Benchmark 2: ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform missing\n     Time (mean ± σ):      48.4 ms ±   0.5 ms    [User: 38.5 ms, System: 8.7 ms]\n     Range (min … max):    47.8 ms …  51.9 ms    58 runs\n\n     Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.\n\n   Summary\n     ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform  ran\n       1.28 ± 0.02 times faster than ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform missing\n\nWe can see that the shared prefix optimization helps noticeably, even\nthough the measurements are noisy.\n\nWith your big patch 2:\n\n   Benchmark 1: ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform\n     Time (mean ± σ):      38.0 ms ±   0.4 ms    [User: 28.3 ms, System: 8.5 ms]\n     Range (min … max):    37.7 ms …  40.3 ms    72 runs\n\n     Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.\n\n   Benchmark 2: ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform missing\n     Time (mean ± σ):      48.5 ms ±   0.4 ms    [User: 38.5 ms, System: 8.8 ms]\n     Range (min … max):    47.9 ms …  50.6 ms    58 runs\n\n     Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.\n\n   Summary\n     ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform  ran\n       1.28 ± 0.02 times faster than ./git -C ../chromium/src ls-files third_party/blink/web_tests/external third_party/blink/web_tests/platform missing\n\nThe difference to main is small enough to get lost in the noise.\n\nThe one-line fix is nice and surgical, but I like the other one more.\nGets rid of crusty underutilized code that doesn't even seem to make\na measurable difference.\n\nRené\n"},{"id":"479340","messageId":"xmqqzg454eoc.fsf@gitster.g","threadId":"59963","inReplyTo":"39d5e14d-f2e9-e619-c3e8-e63c7547370b@web.de","subject":"Re: [PATCH 2alt/2] dir: match \"attr\" pathspec magic with correct paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-09T09:28:19Z","receivedAt":"2023-07-09T09:30:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> This \"common leading prefix\" optimization has two main features:\n>>\n>>  * discard the entries in the in-core index that are outside of the\n>>    common leading prefix; if you are doing \"ls-files one/a one/b\",\n>>    we know all matches must be from \"one/\", so first the code\n>>    discards all entries outside the \"one/\" directory from the\n>>    in-core index.  This allows us to work on a smaller dataset.\n>>\n>>  * allow skipping the comparison of the leading bytes when matching\n>>    pathspec with path.  When \"ls-files\" finds the path \"one/a/1\" in\n>>    the in-core index given \"one/a\" and \"one/b\" as the pathspec,\n>>    knowing that common leading prefix \"one/\" was found lets the\n>>    pathspec matchinery not to bother comparing \"one/\" part, and\n>>    allows it to feed \"a/1\" down, as long as the pathspec element\n>>    \"one/a\" gets corresponding adjustment to \"a\".\n>> ...\n> With your big patch 2:\n> ...\n> The difference to main is small enough to get lost in the noise.\n>\n> The one-line fix is nice and surgical, but I like the other one more.\n> Gets rid of crusty underutilized code that doesn't even seem to make\n> a measurable difference.\n\nYour benchmark matches my intuition, in that the main benefit of the\noptimization comes from discarding the in-core cache entries outside\nthe area covered by the common prefix, and not from being able to\nskip comparing a leading bytes.  The value in code simplification\nthe larger change has may want to be pursued later, but I'd rather\nsee us make a small \"fix\" that can be merged down to 'maint' first.\n\nThanks.\n"}]}