threads / discuss / 59955

Expected behaviour for pathspecs matching attributes in subdirectories

Subject: Expected behaviour for pathspecs matching attributes in subdirectories

## tl;dr

9 messages between Jul 6, 2023 and Jul 8, 2023.

replies: 8people: 4as markdown or json

Matthew Hughes· Jul 6, 2023, 10:33 UTC · lore
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· Jul 6, 2023, 17:35 UTC · re: Matthew Hughes · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

Matthew Hughes <mhughes@uw.co.uk> 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· Jul 6, 2023, 20:54 UTC · re: Junio C Hamano · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

On Thu, 6 Jul 2023 at 18:35, Junio C Hamano <gitster@pobox.com> 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· Jul 6, 2023, 21:00 UTC · re: Matthew Hughes · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

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· Jul 6, 2023, 21:01 UTC · re: Matthew Hughes · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

Matthew Hughes <mhughes@uw.co.uk> 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· Jul 7, 2023, 08:45 UTC · re: Junio C Hamano · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

On Thu, 6 Jul 2023 at 22:01, Junio C Hamano <gitster@pobox.com> 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· Jul 7, 2023, 17:23 UTC · re: Matthew Hughes · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

Matthew Hughes <mhughes@uw.co.uk> writes:
Show 8 quoted lines
> 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· Jul 7, 2023, 19:28 UTC · re: Junio C Hamano · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

On Fri, Jul 7, 2023 at 1:26 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> +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· Jul 8, 2023, 12:42 UTC · re: Junio C Hamano · lore

Re: Expected behaviour for pathspecs matching attributes in subdirectories

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.

← back to recent threads