{"thread":{"id":"59955","subject":"Expected behaviour for pathspecs matching attributes in subdirectories","startedAt":"2023-07-06T10:33:48Z","lastAt":"2023-07-08T12:43:00Z","messageCount":9,"participants":["Matthew Hughes","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"479256","messageId":"CAEzX-aD1wfgp8AvNNfCXVM3jAaAjK+uFTqS2XP4CJbVvFr2BtQ@mail.gmail.com","threadId":"59955","inReplyTo":null,"subject":"Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Matthew Hughes","fromEmail":"mhughes@uw.co.uk","sentAt":"2023-07-06T10:33:29Z","receivedAt":"2023-07-06T10:33:48Z","isPatch":false,"sender":{"key":"mhughes@uw.co.uk","avatar":null},"body":"Hi,\n\nI'm working with pathspecs matching on attributes, here's a basic repo setup:\n\n $ git init .\n $ mkdir -p sub/sub\n $ touch fileA sub/fileA sub/sub/fileA\n $ echo 'fileA labelA' > sub/.gitattributes\n $ git add .\n $ git commit -m 'Build the repo'\n\nWhat I want to do: list all files under 'sub/sub' that have 'labelA' as an\nattribute.\n\nWorking from the top level I get the output I would expect:\n\n $ git ls-files -- ':(attr:labelA)'\n sub/fileA\n sub/sub/fileA\n $ git ls-files -- ':(attr:labelA)sub'\n sub/fileA\n sub/sub/fileA\n\nBut as soon as a directory is included in the pathspec's path no files are\nreturned:\n\n $ git ls-files -- ':(attr:labelA)sub/'\n $ git ls-files -- ':(attr:labelA)sub/sub'\n\nWithout the attribtue magic I can use a directory to list files there:\n\n $ git ls-files -- sub/sub\n sub/sub/fileA\n\nIs this the expected behaviour? I looked in t/t6135-pathspec-with-attrs.sh and\ndidn't see a test case covering something similar. If this is expected, is\nthere another way to achieve what I'm looking for?\n\nThanks for your time,\nMatt\n"},{"id":"479258","messageId":"xmqq7crddjtq.fsf@gitster.g","threadId":"59955","inReplyTo":"CAEzX-aD1wfgp8AvNNfCXVM3jAaAjK+uFTqS2XP4CJbVvFr2BtQ@mail.gmail.com","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-06T17:35:29Z","receivedAt":"2023-07-06T17:35:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Hughes writes:\n\n> Is this the expected behaviour? I looked in t/t6135-pathspec-with-attrs.sh and\n> didn't see a test case covering something similar. If this is expected, is\n> there another way to achieve what I'm looking for?\n\nI wonder if this serves a good addition to the tests?\n\n t/t6135-pathspec-with-attrs.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh\nindex 457cc167c7..83e6bac8e5 100755\n--- c/t/t6135-pathspec-with-attrs.sh\n+++ w/t/t6135-pathspec-with-attrs.sh\n@@ -75,6 +75,14 @@ test_expect_success 'check specific set attr' '\n \tsub/fileSetLabel\n \tEOF\n \tgit ls-files \":(attr:label)\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit ls-files \":(attr:label)sub/\" >actual &&\n+\ttest_write_lines sub/fileSetLabel >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit ls-files \":(attr:label)sub\" >actual &&\n+\ttest_write_lines sub/fileSetLabel >expect &&\n \ttest_cmp expect actual\n '\n \n"},{"id":"479264","messageId":"CAEzX-aC=UDkf6nevLbN0bNyGXujZVDuqtCp3YcYhAPD6zvYZiQ@mail.gmail.com","threadId":"59955","inReplyTo":"xmqq7crddjtq.fsf@gitster.g","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Matthew Hughes","fromEmail":"mhughes@uw.co.uk","sentAt":"2023-07-06T20:54:10Z","receivedAt":"2023-07-06T20:54:44Z","isPatch":false,"sender":{"key":"mhughes@uw.co.uk","avatar":null},"body":"On Thu, 6 Jul 2023 at 18:35, Junio C Hamano wrote:\n\n> I wonder if this serves a good addition to the tests?\n\nGood idea, that would help clear up what's expected.\n\nWhat about some tests for the case of attributes defined in a subdirectory? I'm\nstill trying to understand what's expected in that case, specifically the\ntest_must_be_empty case (the 'rm' at the end is just to avoid polluting any\nother tests with extra attributes, not sure if there's a more standard way of\ndoing this):\n\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 457cc167c7..7a7502a6eb 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -87,6 +87,23 @@ test_expect_success 'check specific set attr (2)' '\n test_cmp expect actual\n '\n\n+test_expect_success 'check specific set attr nested .gitattributes' '\n+ cat <<-\\EOF >sub/.gitattributes &&\n+ fileSetLabel otherLabel\n+ EOF\n+ test_write_lines sub/fileSetLabel >expect &&\n+ git ls-files \":(attr:otherLabel)\" >actual &&\n+ test_cmp expect actual &&\n+\n+ git ls-files \":(attr:otherLabel)sub\" >actual &&\n+ test_cmp expect actual &&\n+\n+ git ls-files \":(attr:otherLabel)sub/\" >actual &&\n+ test_must_be_empty actual &&\n+\n+ rm sub/.gitattributes\n+'\n+\n test_expect_success 'check specific unset attr' '\n cat <<-\\EOF >expect &&\n fileUnsetLabel\n"},{"id":"479265","messageId":"CAEzX-aBUyKivBYTFaLh0_irEH5ToVTVBUsSSU03-3ZMdPbjOow@mail.gmail.com","threadId":"59955","inReplyTo":"CAEzX-aC=UDkf6nevLbN0bNyGXujZVDuqtCp3YcYhAPD6zvYZiQ@mail.gmail.com","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Matthew Hughes","fromEmail":"mhughes@uw.co.uk","sentAt":"2023-07-06T21:00:01Z","receivedAt":"2023-07-06T21:00:17Z","isPatch":false,"sender":{"key":"mhughes@uw.co.uk","avatar":null},"body":"Apologies, re-sending the patch as an attachment instead, since Gmail\ndecided to strip the tab characters\n\n\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 457cc167c7..7a7502a6eb 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -87,6 +87,23 @@ test_expect_success 'check specific set attr (2)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check specific set attr nested .gitattributes' '\n+\tcat <<-\\EOF >sub/.gitattributes &&\n+\tfileSetLabel otherLabel\n+\tEOF\n+\ttest_write_lines sub/fileSetLabel >expect &&\n+\tgit ls-files \":(attr:otherLabel)\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit ls-files \":(attr:otherLabel)sub\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit ls-files \":(attr:otherLabel)sub/\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\trm sub/.gitattributes\n+'\n+\n test_expect_success 'check specific unset attr' '\n \tcat <<-\\EOF >expect &&\n \tfileUnsetLabel\n"},{"id":"479266","messageId":"xmqqjzvcbvqd.fsf@gitster.g","threadId":"59955","inReplyTo":"CAEzX-aC=UDkf6nevLbN0bNyGXujZVDuqtCp3YcYhAPD6zvYZiQ@mail.gmail.com","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-06T21:01:14Z","receivedAt":"2023-07-06T21:01:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Hughes writes:\n\n> ... (the 'rm' at the end is just to avoid polluting any\n> other tests with extra attributes, not sure if there's a more standard way of\n> doing this):\n\nStudy the use of test_when_finished in other tests, perhaps?\n\n"},{"id":"479273","messageId":"CAEzX-aCnp0avSbMdyFQz=3s4-hjdeVwnndR5b7UeZo4oNMnv7A@mail.gmail.com","threadId":"59955","inReplyTo":"xmqqjzvcbvqd.fsf@gitster.g","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Matthew Hughes","fromEmail":"mhughes@uw.co.uk","sentAt":"2023-07-07T08:45:03Z","receivedAt":"2023-07-07T08:45:19Z","isPatch":false,"sender":{"key":"mhughes@uw.co.uk","avatar":null},"body":"On Thu, 6 Jul 2023 at 22:01, Junio C Hamano wrote:\n\n> Study the use of test_when_finished in other tests, perhaps?\n\nThanks for the suggestion, that appears to be what I was looking for. It's my\nfirst time looking around Git's test framework, so I had a scan of the docs but\nmanaged to miss that one.\n\nI'd be happy to submit a patch adding those tests if you'd like. Though I would\nlike to just confirm that in the patch I shared it is not a bug that:\n\n git ls-files \":(attr:otherLabel)sub/\" >actual &&\n test_must_be_empty actual\n\nI.e. that no files are listed here even tough `sub/fileSetLabel` has the\nattribute `otherLabel`?\n"},{"id":"479284","messageId":"xmqq1qhjbpoz.fsf@gitster.g","threadId":"59955","inReplyTo":"CAEzX-aCnp0avSbMdyFQz=3s4-hjdeVwnndR5b7UeZo4oNMnv7A@mail.gmail.com","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-07T17:23:56Z","receivedAt":"2023-07-07T17:24:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Hughes writes:\n\n> I'd be happy to submit a patch adding those tests if you'd like. Though I would\n> like to just confirm that in the patch I shared it is not a bug that:\n>\n> git ls-files \":(attr:otherLabel)sub/\" >actual &&\n> test_must_be_empty actual\n>\n> I.e. that no files are listed here even tough `sub/fileSetLabel` has the\n> attribute `otherLabel`?\n\nI do not think it is a good idea to cast in stone the behaviour,\nwhich we do not know if it is sensible, with a new test, before\nknowning what behaviour we want.\n\nI think in this case the common prefix optimization in \"ls-files.c\"\nis broken. If we disable it like the attached illustration patch,\nwe will see that pathspecs that end with \"sub\" or \"sub/\" behave the\nsame way, which is what I think people would expect.\n\nThe code change in this illustration is not a \"fix\", of course ;-).\n\nThanks.\n\n builtin/ls-files.c | 2 +-\n t/t6135-pathspec-with-attrs.sh | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/ls-files.c w/builtin/ls-files.c\nindex a0229c3277..17baed30ca 100644\n--- c/builtin/ls-files.c\n+++ w/builtin/ls-files.c\n@@ -724,7 +724,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t * submodule entry because the pathspec may match something inside the\n \t * submodule.\n \t */\n-\tif (recurse_submodules)\n+\tif (!!\"disable common prefix optimization\" || recurse_submodules)\n \t\tmax_prefix = NULL;\n \telse\n \t\tmax_prefix = common_prefix(&pathspec);\ndiff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh\nindex 457cc167c7..a805fa132b 100755\n--- c/t/t6135-pathspec-with-attrs.sh\n+++ w/t/t6135-pathspec-with-attrs.sh\n@@ -253,4 +253,21 @@ 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' '\n+\ttest_when_finished \"rm -f sub/.gitattributes\" &&\n+\ttest_write_lines \"fileSetLabel label1\" >sub/.gitattributes &&\n+\n+\tgit ls-files \":(attr:label1)\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit ls-files \":(attr:label1)sub\" >actual &&\n+\ttest_write_lines \"sub/fileSetLabel\" >expect &&\n+\ttest_cmp expect actual &&\n+\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\n"},{"id":"479293","messageId":"CAPig+cSo8+bh7vHBchwLfpdxehHmRFnbsOKOCpKoYGd=bzO_ig@mail.gmail.com","threadId":"59955","inReplyTo":"xmqq1qhjbpoz.fsf@gitster.g","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-07-07T19:28:42Z","receivedAt":"2023-07-07T19:31:05Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 7, 2023 at 1:26 PM Junio C Hamano wrote:\n> +test_expect_success 'reading from .gitattributes in a subdirectory' '\n> + test_when_finished \"rm -f sub/.gitattributes\" &&\n> + test_write_lines \"fileSetLabel label1\" >sub/.gitattributes &&\n> +\n> + git ls-files \":(attr:label1)\" >actual &&\n> + test_write_lines \"sub/fileSetLabel\" >expect &&\n> + test_cmp expect actual &&\n> +\n> + git ls-files \":(attr:label1)sub\" >actual &&\n> + test_write_lines \"sub/fileSetLabel\" >expect &&\n> + test_cmp expect actual &&\n> +\n> + git ls-files \":(attr:label1)sub/\" >actual &&\n> + test_write_lines \"sub/fileSetLabel\" >expect &&\n> + test_cmp expect actual\n> +'\n\nPerhaps shave off a bit of cognitive overhead by using `echo` for\nthese rather than `test_write_lines`. For instance:\n\n echo \"fileSetLabel label1\" >sub/.gitattributes &&\n ...\n echo \"sub/fileSetLabel\" >expect &&\n\nand so on.\n"},{"id":"479331","messageId":"20230708124253.6g2sl2diipqfma7g@archP14s","threadId":"59955","inReplyTo":"xmqq1qhjbpoz.fsf@gitster.g","subject":"Re: Expected behaviour for pathspecs matching attributes in subdirectories","fromName":"Matthew Hughes","fromEmail":"matthewhughes934@gmail.com","sentAt":"2023-07-08T12:42:53Z","receivedAt":"2023-07-08T12:43:00Z","isPatch":false,"sender":{"key":"matthewhughes934@gmail.com","avatar":"https://avatars.githubusercontent.com/u/34972397?v=4"},"body":"On Fri, Jul 07, 2023 at 10:23:56AM -0700, Junio C Hamano wrote:\n\n> I think in this case the common prefix optimization in \"ls-files.c\"\n> is broken. If we disable it like the attached illustration patch,\n> we will see that pathspecs that end with \"sub\" or \"sub/\" behave the\n> same way, which is what I think people would expect.\n\nThanks, this also revealed a temporary workaround for me, just add a pathspec\nthat ensures there's no common prefix but also matches nothing:\n\n $ git ls-files -- ':(attr:labelA)sub/sub/' 'not/a/real/path'\n sub/sub/fileA\n\nI might have a look into that optimization and see what changes might be made,\nbut given I am unfamiliar with the code-base this might not be the most\nproductive search.\n"}]}