Expected behaviour for pathspecs matching attributes in subdirectories

Expected behaviour for pathspecs matching attributes in subdirectories

[Matthew Hughes]

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

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Junio C Hamano]

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
 '
 

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Matthew Hughes]

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

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Matthew Hughes]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Apologies, re-sending the patch as an attachment instead, since Gmail
decided to strip the tab characters

[-- Attachment #2: attributes_test.patch --]
[-- Type: text/x-patch, Size: 838 bytes --]

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

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Junio C Hamano]

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?

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Matthew Hughes]

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`?

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Junio C Hamano]

Matthew Hughes <mhughes@uw.co.uk> 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

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Eric Sunshine]

On Fri, Jul 7, 2023 at 1:26 PM Junio C Hamano <gitster@pobox.com> 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.

Re: Expected behaviour for pathspecs matching attributes in subdirectories

[Matthew Hughes]

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.