# Expected behaviour for pathspecs matching attributes in subdirectories 9 messages from 2023-07-06 to 2023-07-08. Participants: Matthew Hughes, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/59955 ## Matthew Hughes, 2023-07-06 10:33 Subject: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/CAEzX-aD1wfgp8AvNNfCXVM3jAaAjK%2BuFTqS2XP4CJbVvFr2BtQ%40mail.gmail.com ``` Hi, I'm working with pathspecs matching on attributes, here's a basic repo setup: $ git init . $ mkdir -p sub/sub $ touch fileA sub/fileA sub/sub/fileA $ echo 'fileA labelA' > sub/.gitattributes $ git add . $ git commit -m 'Build the repo' What I want to do: list all files under 'sub/sub' that have 'labelA' as an attribute. Working from the top level I get the output I would expect: $ git ls-files -- ':(attr:labelA)' sub/fileA sub/sub/fileA $ git ls-files -- ':(attr:labelA)sub' sub/fileA sub/sub/fileA But as soon as a directory is included in the pathspec's path no files are returned: $ git ls-files -- ':(attr:labelA)sub/' $ git ls-files -- ':(attr:labelA)sub/sub' Without the attribtue magic I can use a directory to list files there: $ git ls-files -- sub/sub sub/sub/fileA Is this the expected behaviour? I looked in t/t6135-pathspec-with-attrs.sh and didn't see a test case covering something similar. If this is expected, is there another way to achieve what I'm looking for? Thanks for your time, Matt ``` ## Junio C Hamano, 2023-07-06 17:35 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/xmqq7crddjtq.fsf%40gitster.g In-Reply-To: ``` Matthew Hughes writes: > Is this the expected behaviour? I looked in t/t6135-pathspec-with-attrs.sh and > didn't see a test case covering something similar. If this is expected, is > there another way to achieve what I'm looking for? I wonder if this serves a good addition to the tests? t/t6135-pathspec-with-attrs.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh index 457cc167c7..83e6bac8e5 100755 --- c/t/t6135-pathspec-with-attrs.sh +++ w/t/t6135-pathspec-with-attrs.sh @@ -75,6 +75,14 @@ test_expect_success 'check specific set attr' ' sub/fileSetLabel EOF git ls-files ":(attr:label)" >actual && + test_cmp expect actual && + + git ls-files ":(attr:label)sub/" >actual && + test_write_lines sub/fileSetLabel >expect && + test_cmp expect actual && + + git ls-files ":(attr:label)sub" >actual && + test_write_lines sub/fileSetLabel >expect && test_cmp expect actual ' ``` ## Matthew Hughes, 2023-07-06 20:54 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/CAEzX-aC%3DUDkf6nevLbN0bNyGXujZVDuqtCp3YcYhAPD6zvYZiQ%40mail.gmail.com In-Reply-To: ``` On Thu, 6 Jul 2023 at 18:35, Junio C Hamano wrote: > I wonder if this serves a good addition to the tests? Good idea, that would help clear up what's expected. What about some tests for the case of attributes defined in a subdirectory? I'm still trying to understand what's expected in that case, specifically the test_must_be_empty case (the 'rm' at the end is just to avoid polluting any other tests with extra attributes, not sure if there's a more standard way of doing this): diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh index 457cc167c7..7a7502a6eb 100755 --- a/t/t6135-pathspec-with-attrs.sh +++ b/t/t6135-pathspec-with-attrs.sh @@ -87,6 +87,23 @@ test_expect_success 'check specific set attr (2)' ' test_cmp expect actual ' +test_expect_success 'check specific set attr nested .gitattributes' ' + cat <<-\EOF >sub/.gitattributes && + fileSetLabel otherLabel + EOF + test_write_lines sub/fileSetLabel >expect && + git ls-files ":(attr:otherLabel)" >actual && + test_cmp expect actual && + + git ls-files ":(attr:otherLabel)sub" >actual && + test_cmp expect actual && + + git ls-files ":(attr:otherLabel)sub/" >actual && + test_must_be_empty actual && + + rm sub/.gitattributes +' + test_expect_success 'check specific unset attr' ' cat <<-\EOF >expect && fileUnsetLabel ``` ## Matthew Hughes, 2023-07-06 21:00 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/CAEzX-aBUyKivBYTFaLh0_irEH5ToVTVBUsSSU03-3ZMdPbjOow%40mail.gmail.com In-Reply-To: ``` Apologies, re-sending the patch as an attachment instead, since Gmail decided to strip the tab characters diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh index 457cc167c7..7a7502a6eb 100755 --- a/t/t6135-pathspec-with-attrs.sh +++ b/t/t6135-pathspec-with-attrs.sh @@ -87,6 +87,23 @@ test_expect_success 'check specific set attr (2)' ' test_cmp expect actual ' +test_expect_success 'check specific set attr nested .gitattributes' ' + cat <<-\EOF >sub/.gitattributes && + fileSetLabel otherLabel + EOF + test_write_lines sub/fileSetLabel >expect && + git ls-files ":(attr:otherLabel)" >actual && + test_cmp expect actual && + + git ls-files ":(attr:otherLabel)sub" >actual && + test_cmp expect actual && + + git ls-files ":(attr:otherLabel)sub/" >actual && + test_must_be_empty actual && + + rm sub/.gitattributes +' + test_expect_success 'check specific unset attr' ' cat <<-\EOF >expect && fileUnsetLabel ``` ## Junio C Hamano, 2023-07-06 21:01 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/xmqqjzvcbvqd.fsf%40gitster.g In-Reply-To: ``` Matthew Hughes writes: > ... (the 'rm' at the end is just to avoid polluting any > other tests with extra attributes, not sure if there's a more standard way of > doing this): Study the use of test_when_finished in other tests, perhaps? ``` ## Matthew Hughes, 2023-07-07 08:45 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/CAEzX-aCnp0avSbMdyFQz%3D3s4-hjdeVwnndR5b7UeZo4oNMnv7A%40mail.gmail.com In-Reply-To: ``` On Thu, 6 Jul 2023 at 22:01, Junio C Hamano wrote: > Study the use of test_when_finished in other tests, perhaps? Thanks for the suggestion, that appears to be what I was looking for. It's my first time looking around Git's test framework, so I had a scan of the docs but managed to miss that one. I'd be happy to submit a patch adding those tests if you'd like. Though I would like to just confirm that in the patch I shared it is not a bug that: git ls-files ":(attr:otherLabel)sub/" >actual && test_must_be_empty actual I.e. that no files are listed here even tough `sub/fileSetLabel` has the attribute `otherLabel`? ``` ## Junio C Hamano, 2023-07-07 17:23 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/xmqq1qhjbpoz.fsf%40gitster.g In-Reply-To: ``` Matthew Hughes writes: > I'd be happy to submit a patch adding those tests if you'd like. Though I would > like to just confirm that in the patch I shared it is not a bug that: > > git ls-files ":(attr:otherLabel)sub/" >actual && > test_must_be_empty actual > > I.e. that no files are listed here even tough `sub/fileSetLabel` has the > attribute `otherLabel`? I do not think it is a good idea to cast in stone the behaviour, which we do not know if it is sensible, with a new test, before knowning what behaviour we want. I think in this case the common prefix optimization in "ls-files.c" is broken. If we disable it like the attached illustration patch, we will see that pathspecs that end with "sub" or "sub/" behave the same way, which is what I think people would expect. The code change in this illustration is not a "fix", of course ;-). Thanks. builtin/ls-files.c | 2 +- t/t6135-pathspec-with-attrs.sh | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git c/builtin/ls-files.c w/builtin/ls-files.c index a0229c3277..17baed30ca 100644 --- c/builtin/ls-files.c +++ w/builtin/ls-files.c @@ -724,7 +724,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) * submodule entry because the pathspec may match something inside the * submodule. */ - if (recurse_submodules) + if (!!"disable common prefix optimization" || recurse_submodules) max_prefix = NULL; else max_prefix = common_prefix(&pathspec); diff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh index 457cc167c7..a805fa132b 100755 --- c/t/t6135-pathspec-with-attrs.sh +++ w/t/t6135-pathspec-with-attrs.sh @@ -253,4 +253,21 @@ test_expect_success 'backslash cannot be used as a value' ' test_i18ngrep "for value matching" actual ' +test_expect_success 'reading from .gitattributes in a subdirectory' ' + test_when_finished "rm -f sub/.gitattributes" && + test_write_lines "fileSetLabel label1" >sub/.gitattributes && + + git ls-files ":(attr:label1)" >actual && + test_write_lines "sub/fileSetLabel" >expect && + test_cmp expect actual && + + git ls-files ":(attr:label1)sub" >actual && + test_write_lines "sub/fileSetLabel" >expect && + test_cmp expect actual && + + git ls-files ":(attr:label1)sub/" >actual && + test_write_lines "sub/fileSetLabel" >expect && + test_cmp expect actual +' + test_done ``` ## Eric Sunshine, 2023-07-07 19:28 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSo8%2Bbh7vHBchwLfpdxehHmRFnbsOKOCpKoYGd%3DbzO_ig%40mail.gmail.com In-Reply-To: ``` On Fri, Jul 7, 2023 at 1:26 PM Junio C Hamano wrote: > +test_expect_success 'reading from .gitattributes in a subdirectory' ' > + test_when_finished "rm -f sub/.gitattributes" && > + test_write_lines "fileSetLabel label1" >sub/.gitattributes && > + > + git ls-files ":(attr:label1)" >actual && > + test_write_lines "sub/fileSetLabel" >expect && > + test_cmp expect actual && > + > + git ls-files ":(attr:label1)sub" >actual && > + test_write_lines "sub/fileSetLabel" >expect && > + test_cmp expect actual && > + > + git ls-files ":(attr:label1)sub/" >actual && > + test_write_lines "sub/fileSetLabel" >expect && > + test_cmp expect actual > +' Perhaps shave off a bit of cognitive overhead by using `echo` for these rather than `test_write_lines`. For instance: echo "fileSetLabel label1" >sub/.gitattributes && ... echo "sub/fileSetLabel" >expect && and so on. ``` ## Matthew Hughes, 2023-07-08 12:42 Subject: Re: Expected behaviour for pathspecs matching attributes in subdirectories Message-ID: <20230708124253.6g2sl2diipqfma7g@archP14s> URL: https://gitlist.dev/e/20230708124253.6g2sl2diipqfma7g%40archP14s In-Reply-To: ``` On Fri, Jul 07, 2023 at 10:23:56AM -0700, Junio C Hamano wrote: > I think in this case the common prefix optimization in "ls-files.c" > is broken. If we disable it like the attached illustration patch, > we will see that pathspecs that end with "sub" or "sub/" behave the > same way, which is what I think people would expect. Thanks, this also revealed a temporary workaround for me, just add a pathspec that ensures there's no common prefix but also matches nothing: $ git ls-files -- ':(attr:labelA)sub/sub/' 'not/a/real/path' sub/sub/fileA I might have a look into that optimization and see what changes might be made, but given I am unfamiliar with the code-base this might not be the most productive search. ```