All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] Allow attr magic for git-add, git-stash.
@ 2023-10-05 23:23 Joanna Wang
  2023-10-06  2:42 ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Joanna Wang @ 2023-10-05 23:23 UTC (permalink / raw)
  To: git; +Cc: Joanna Wang

This lets users limit added/stashed files or exclude files based on file
attributes. For example, the chromium project would like to use
this like "git add --all ':(exclude,attr:submodule)'", as submodules
are managed in a unique way and often results in submodule changes
that users do not want in their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add/stash, which was previously
blocked (for stash/add) by GUARD_PATHSPEC and (for add only) a pathspec
mask in parse_pathspec()).

With this patch, attr is supported for the two commands. It is possible
that when the attr pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13), 
"PATHSPEC_ATTR" was just unintentionally left out of a few
GUARD_PATHSPEC() invocations. Later, to get a more user-friendly error
message when attr was used with git-add, PATHSPEC_ATTR was added as a
mask to git-add's invocation of parse_pathspec() in
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).

Any bugs this may trigger will be fixed in follow-up patches.

Signed-off-by: Joanna Wang <jojwang@google.com>
---

I have tested this thoroughly with different flags, non-root directories,
and other magic pathspecs, but may be unaware of some edge cases.

The GUARD_PATHSPEC() in dir.c is a potential part of the call stack of
fill_directory(), which is called by add, stash, grep, ls-files,
sparse-checkout, and status.
However, this patch only seems to add new behavior to git-add and git-stash.
grep, ls-files, and status already support attr pathspecs and running some of
these commands with the debugger confirms this GUARD_PATHSPEC() is never reached.
But again I am not aware of all edge cases. For sparse-checkout I'm not sure
if/how pathspec can be used, but either way did not see new behavior.

For context, attr magic support for git-add is part of work discussed here:
https://lore.kernel.org/git/xmqqfs2yjgvr.fsf@gitster.g/T/#t

 builtin/add.c                  |  5 +++--
 dir.c                          |  8 +-------
 t/t6135-pathspec-with-attrs.sh | 34 ++++++++++++++++++++++++++--------
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..ef0b8d55fd 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 8486e4d56f..55c9607b1a 100644
--- a/dir.c
+++ b/dir.c
@@ -2173,13 +2173,7 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 	if (!pathspec || !pathspec->nr)
 		return 0;
 
-	GUARD_PATHSPEC(pathspec,
-		       PATHSPEC_FROMTOP |
-		       PATHSPEC_MAXDEPTH |
-		       PATHSPEC_LITERAL |
-		       PATHSPEC_GLOB |
-		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+	GUARD_PATHSPEC(pathspec, PATHSPEC_ALL_MAGIC);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e11d8cb119 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,23 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +161,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +187,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +213,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,14 +253,18 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
-	# The main purpose of this test is to check that we actually fail
-	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
-	test_i18ngrep "magic not supported" actual
+test_expect_success 'check that attr magic works for git-add' '
+	# attr magic was previously blocked for git-add. With attr magic
+	# enabled for git-add, add a basic test to make sure it works as
+	# expected and add more tests if more bugs are discovered.
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	touch sub/newFileA-foo &&
+	touch sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'abort on giving invalid label on the command line' '
-- 
2.42.0.582.g8ccd20d70d-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] Allow attr magic for git-add, git-stash.
  2023-10-05 23:23 [PATCH 1/1] Allow attr magic for git-add, git-stash Joanna Wang
@ 2023-10-06  2:42 ` Junio C Hamano
  2023-10-06 17:05   ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-10-06  2:42 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Joanna Wang <jojwang@google.com> writes:

> This lets users limit added/stashed files or exclude files based on file
> attributes. For example, the chromium project would like to use
> this like "git add --all ':(exclude,attr:submodule)'", as submodules
> are managed in a unique way and often results in submodule changes
> that users do not want in their commits.
> ...

Everything else before the following line was written quite well
(perhaps except for the commit title on the Subject: header).  Very
pleasant to see in the first iteration of a new contributor.

> Any bugs this may trigger will be fixed in follow-up patches.

This is rather a poor statement to make, as it hints that there are
known breakages this change will reveal that you are not telling us,
although I suspect that it is not the case.

But in case there are such existing-but-not-revealed bugs, the above
is not how the world works around here.  Instead, any existing code
whose bug has been hidden behind the GUARD_PATHSPEC() and known to
be revealed with this change should be fixed with N preliminary
clean-up patches, and then finally this patch would become the last
[N+1/N+1] step of such a N+1 patch series.

If no bugs are known to be revealed by applying this code change,
then we should just drop the above comment.  It is common to see a
patch that changes behaviour without intending to break anything has
unintended and unfortunate side effects, and we cannot really avoid
it.  The fallouts from such a change will be fixed after they are
discovered, and that is not something we need or want to repeat at
the end of every commit message ;-)

> I have tested this thoroughly with different flags, non-root directories,
> and other magic pathspecs, but may be unaware of some edge cases.

These are good things to add to the new test this patch adds.  Your
one time testing will not protect the current code that works for
these undocumented cases from future breakages, but when made into
a part of the test suite, it will.

> diff --git a/builtin/add.c b/builtin/add.c
> index c27254a5cd..ef0b8d55fd 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	 * Check the "pathspec '%s' did not match any files" block
>  	 * below before enabling new magic.
>  	 */
> -	parse_pathspec(&pathspec, PATHSPEC_ATTR,
> +	parse_pathspec(&pathspec, 0,
>  		       PATHSPEC_PREFER_FULL |
>  		       PATHSPEC_SYMLINK_LEADING_PATH,
>  		       prefix, argv);
> @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			       PATHSPEC_LITERAL |
>  			       PATHSPEC_GLOB |
>  			       PATHSPEC_ICASE |
> -			       PATHSPEC_EXCLUDE);
> +			       PATHSPEC_EXCLUDE |
> +			       PATHSPEC_ATTR);

Both hunks look quite as expected.  Looking good.

> diff --git a/dir.c b/dir.c
> index 8486e4d56f..55c9607b1a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2173,13 +2173,7 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
>  	if (!pathspec || !pathspec->nr)
>  		return 0;
>  
> -	GUARD_PATHSPEC(pathspec,
> -		       PATHSPEC_FROMTOP |
> -		       PATHSPEC_MAXDEPTH |
> -		       PATHSPEC_LITERAL |
> -		       PATHSPEC_GLOB |
> -		       PATHSPEC_ICASE |
> -		       PATHSPEC_EXCLUDE);
> +	GUARD_PATHSPEC(pathspec, PATHSPEC_ALL_MAGIC);

Hmph, is it a good idea in general to use ALL_MAGIC in guard?  The
programmer who wrote this not just promises that everything in the
current set of PATHSPEC_FOO bits are supported in this codepath
right now, but also says any *new* PATHSPEC_FOO magic will be
automatically supported.  How does the updated code guarantee it?

> @@ -239,14 +253,18 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
>  	test_i18ngrep "Only one" actual
>  '
>  
> -test_expect_success 'fail if attr magic is used places not implemented' '
> -	# The main purpose of this test is to check that we actually fail
> -	# when you attempt to use attr magic in commands that do not implement
> -	# attr magic. This test does not advocate git-add to stay that way,
> -	# though, but git-add is convenient as it has its own internal pathspec
> -	# parsing.
> -	test_must_fail git add ":(attr:labelB)" 2>actual &&
> -	test_i18ngrep "magic not supported" actual

As the comment says, this test is primarily about making sure that
parse_pathspec() that limits the allowed pathspec magic and
GUARD_PATHSPEC() that forbids certain magic are working as expected.

It is strongly preferrable, instead of butchering this test that
guards these two mechanisms from being broken, to find a command
that still has some restriction on the magic it allows, and use it
to make sure they still trigger and give "magic not supported" error
message.  IOW, do not remove the above test, but find something
other than "add" that is suitable to still follow the original
intention of the test.

It is, of course, very much welcome to add a new test below to
protect the new feature.  IIRC, "add --all" and "add -u" used
somewhat different codepaths to find and update the changed paths.

Don't we need a test each for both new paths (which "--all" is about
and these two new files created in the test are) and also tracked
paths (which "-u" would try to enumerate and update the index with)?
For that matter, wouldn't "add . ':(exclude)...'" be also something
we need to check?  Or do these three all use pretty-much the same
codepath under the hood?

> +test_expect_success 'check that attr magic works for git-add' '
> +	# attr magic was previously blocked for git-add. With attr magic
> +	# enabled for git-add, add a basic test to make sure it works as
> +	# expected and add more tests if more bugs are discovered.

These three lines are unwanted.

"was X, now Y" and "do Z now" may belong to the log message where
previous state and new state are captured in the commit.  But not in
the tracked contents.  6 months down the road, when reading t6135 in
order to further polish the tests, nobody cares if "add" did not
support the attr magic in the past.  The title of the test already
says the test is about attr magic used in pathspec fed to "git add",
so "add a basic test" is unnecessary to say.

> +	cat <<-\EOF >expect &&
> +	sub/newFileA-foo
> +	EOF
> +	touch sub/newFileA-foo &&
> +	touch sub/newFileB-foo &&

Unless it matters that these files have recent timestamps, do not
use "touch", merely to ensure presence of a file.  We often use a
simple redirection

	>sub/newFileA-foo &&

for such purpose (I thought we had it somewhere in the coding
guidelines document, but if not, we should add it).

> +	git add --all ":(exclude,attr:labelB)" &&
> +	git diff --name-only --cached >actual &&
> +	test_cmp expect actual
>  '

So we have two new paths (I do not offhand know if there are
existing paths that are already tracked), and one is with label and
the other is without.  We tell "add" to grab all paths except for
the paths with the label and see that we do not see the one with the
label.  OK.

Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] Allow attr magic for git-add, git-stash.
  2023-10-06  2:42 ` Junio C Hamano
@ 2023-10-06 17:05   ` Junio C Hamano
  2023-10-07  0:28     ` [PATCH 1/1] add: Enable attr pathspec magic for git-add Joanna Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-10-06 17:05 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

>> -test_expect_success 'fail if attr magic is used places not implemented' '
>> -	# The main purpose of this test is to check that we actually fail
>> -	# when you attempt to use attr magic in commands that do not implement
>> -	# attr magic. This test does not advocate git-add to stay that way,
>> -	# though, but git-add is convenient as it has its own internal pathspec
>> -	# parsing.
>> -	test_must_fail git add ":(attr:labelB)" 2>actual &&
>> -	test_i18ngrep "magic not supported" actual
> ...
> IOW, do not remove the above test, but find something
> other than "add" that is suitable to still follow the original
> intention of the test.

We can do something like the attached, instead of discarding the
protection from future breakage.  You're welcome to find another
command and magic that are less likely to be implemented than
check-ignore with attr magic and replace them, of course.  I just
eyeballed the hits from

    $ git grep -A2 parse_pathspec\(

and picked one that has pathspec magic restriction (i.e., the second
parameter is not zero) and appears near the surface (i.e., the one
in blame.c for example is a bad target, because it is to ensure that
a pathspec that is internally created out of an end-user provided
pathname is treated only literally, and is impossible to pass a
pathspec with offending magic to that codepath from the outside) to
make it easier to pass offending magic from the command line.

A tangent (to the topic of testing, but relevant to the whole
patch).  I notice 'stash' is mentioned on the topic, but I do not
see changes to the codepath that is specific to 'stash', and changes
to the tests do not demonstrate existing breakage.  The lack of code
changes probably is because something shared, which is pretected
with magic guard lifted by the patch, is called from 'stash' as well
as 'add', or something?  Whatever the reason is, it would be better
to document it in the proposed log message, and have a test to make
sure that the command works with the attr magic.

Alternatively, leave the mention of 'stash' out of the patch, if it
is not affected, perhaps?

In any case, here is a replacement for the removed test.


diff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh
index f70c395e75..b450cb5b3a 100755
--- c/t/t6135-pathspec-with-attrs.sh
+++ w/t/t6135-pathspec-with-attrs.sh
@@ -242,10 +242,10 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 test_expect_success 'fail if attr magic is used places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate check-ignore to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another commnad to replace it for the test.
+	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 1/1] add: Enable attr pathspec magic for git-add.
  2023-10-06 17:05   ` Junio C Hamano
@ 2023-10-07  0:28     ` Joanna Wang
  2023-10-07  0:50       ` Joanna Wang
  2023-10-07 10:10       ` [PATCH " Junio C Hamano
  0 siblings, 2 replies; 19+ messages in thread
From: Joanna Wang @ 2023-10-07  0:28 UTC (permalink / raw)
  To: gitster; +Cc: git, jojwang

This lets users limit added files or exclude files based on file
attributes. For example, the chromium project would like to use
this like "git add --all ':(exclude,attr:submodule)'", as submodules
are managed in a unique way and often results in submodule changes
that users do not want in their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).

With this patch, attr is supported. It is possible that when the attr
pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13), 
"PATHSPEC_ATTR" was just unintentionally left out of a few
GUARD_PATHSPEC() invocations. Later, to get a more user-friendly error
message when attr was used with git-add, PATHSPEC_ATTR was added as a
mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).

git-stash which goes through the same GUARD_PATHSPEC(), currently
does not work with attr. So a PATHSPEC_ATTR mask has been added
to its parse_pathspec and parse_pathspec_file()

Signed-off-by: Joanna Wang <jojwang@google.com>
---

> (perhaps except for the commit title on the Subject: header).
Added 'add:' and specified it is 'patchpsec' magic, but lmk if i'm still missing something.

> This is rather a poor statement to make, as it hints that there are
> known breakages this change will reveal that you are not telling us,
>although I suspect that it is not the case.
Yes, sorry I did not mean to make this hint. I was trying to express that
if bugs did come up, we would not abandon this effort and expect someone
else to fix it. I have removed this line. I realized I also don't want
to say we will forever be on the hook for fixing every related bug that comes up.

> These are good things to add to the new test this patch adds.
Tests added.

>Hmph, is it a good idea in general to use ALL_MAGIC in guard?
My original reasoning was using ALL_MAGIC would prevent new magic pathspecs
from being unintentionally left out of commands and if a new magic does not
work with a command, whoever adds it should change ALL_MAGIC back to listing
them individually in the same patch. But yes, listing them individually seems
safer. Better to accidentally leave magic out that can be added with intention
later rather than make it easy to accidentally leave unsupported magic in.
I switched it back to listing individually.

>It is strongly preferrable, instead of butchering this test that
>guards these two mechanisms from being broken, to find a command
>that still has some restriction on the magic it allows, and use it
>to make sure they still trigger and give "magic not supported" error
>message.
I added the test back with git-stash, which, after more testing i discovered it
actually did not fully work with attr. More info below.

>Unless it matters that these files have recent timestamps, do not
>use "touch", merely to ensure presence of a file.  We often use a
>simple redirection.
Done

>A tangent (to the topic of testing, but relevant to the whole
>patch).  I notice 'stash' is mentioned on the topic, but I do not
>see changes to the codepath that is specific to 'stash', and changes
>to the tests do not demonstrate existing breakage.  The lack of code
>changes probably is because something shared, which is pretected
>with magic guard lifted by the patch, is called from 'stash' as well
>as 'add', or something?
Yes, the previous patch enabled attr with git-stash which was only blocked
at the shared dir.c GUARD_PATHSPEC level. I thought attr was working for git-stash
but I was wrong after a few more manual tests.
So I added PATHSPEC_ATTR as a mask to stash's parse_pathspec and parse_pathspec_file.

 builtin/add.c                  |   7 ++-
 builtin/stash.c                |   4 +-
 dir.c                          |  13 ++--
 t/t6135-pathspec-with-attrs.sh | 109 +++++++++++++++++++++++++++++++--
 4 files changed, 117 insertions(+), 16 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..2de83964a3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/builtin/stash.c b/builtin/stash.c
index 1ad496985a..9c77d3e4e4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1760,7 +1760,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 		}
 	}
 
-	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
+	parse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
 		       prefix, argv);
 
 	if (pathspec_from_file) {
@@ -1773,7 +1773,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 		if (ps.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&ps, 0,
+		parse_pathspec_file(&ps, PATHSPEC_ATTR,
 				    PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
 				    prefix, pathspec_from_file, pathspec_file_nul);
 	} else if (pathspec_file_nul) {
diff --git a/dir.c b/dir.c
index 8486e4d56f..9bf9b53ca5 100644
--- a/dir.c
+++ b/dir.c
@@ -2174,12 +2174,13 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		return 0;
 
 	GUARD_PATHSPEC(pathspec,
-		       PATHSPEC_FROMTOP |
-		       PATHSPEC_MAXDEPTH |
-		       PATHSPEC_LITERAL |
-		       PATHSPEC_GLOB |
-		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+                       PATHSPEC_FROMTOP |
+                       PATHSPEC_MAXDEPTH |
+                       PATHSPEC_LITERAL |
+                       PATHSPEC_GLOB |
+                       PATHSPEC_ICASE |
+                       PATHSPEC_EXCLUDE |
+                       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..531b4f4d5e 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,100 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate stash push to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another commnad to replace it for the test.
+	test_must_fail git stash push ":(attr:labelB)" 2>actual &&
+	test_i18ngrep "magic not supported" actual
+'
+
+test_expect_success 'fail if attr magic is used in --pathspec-from-file when not implemented' '
+	# This is like the test above but for attr magic pass in via --pathspec-from-file.
+	cat <<-\EOF >pathspec_file &&
+	:(attr:labelB)
+	EOF
+	test_must_fail git stash push --pathspec-from-file=pathspec_file 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git' '
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.42.0.609.gbb76f46606-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re:[PATCH 1/1] add: Enable attr pathspec magic for git-add.
  2023-10-07  0:28     ` [PATCH 1/1] add: Enable attr pathspec magic for git-add Joanna Wang
@ 2023-10-07  0:50       ` Joanna Wang
  2023-10-07 10:10       ` [PATCH " Junio C Hamano
  1 sibling, 0 replies; 19+ messages in thread
From: Joanna Wang @ 2023-10-07  0:50 UTC (permalink / raw)
  To: jojwang; +Cc: git, gitster

>+test_expect_success 'check that attr magic works for git' '
>+'
Sorry, i forgot to take this out. I can fix it in the next pathset after
the next round of review.



^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] add: Enable attr pathspec magic for git-add.
  2023-10-07  0:28     ` [PATCH 1/1] add: Enable attr pathspec magic for git-add Joanna Wang
  2023-10-07  0:50       ` Joanna Wang
@ 2023-10-07 10:10       ` Junio C Hamano
  2023-10-11 20:20         ` [PATCH 1/1] add: enable attr pathspec magic Joanna Wang
  1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-10-07 10:10 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Joanna Wang <jojwang@google.com> writes:

> Added 'add:' and specified it is 'patchpsec' magic, but lmk if i'm still missing something.

If you run

    $ git shortlog --no-merges v2.42.0..

or something on your branch to see how well its subject blends with
others, you'd start wanting to (1) downcase "Enable", and (2) omit
the full-stop after the title, to make it look similar to others.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1ad496985a..9c77d3e4e4 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1760,7 +1760,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
>  		}
>  	}
>  
> -	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
> +	parse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
>  		       prefix, argv);

This becomes necessary because "stash" later goes through other
places this patch touches (e.g., dir.c::exclude_matches_pathspec()?)
that happened to be serving as a guard for another code "stash" has
that is not prepared to handle attribute magic?  Do you know what
exactly is not ready, so that perhaps others can help figuring out
how to make it ready for the attr magic?

> diff --git a/dir.c b/dir.c
> index 8486e4d56f..9bf9b53ca5 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2174,12 +2174,13 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
>  		return 0;
>  
>  	GUARD_PATHSPEC(pathspec,
> -		       PATHSPEC_FROMTOP |
> -		       PATHSPEC_MAXDEPTH |
> -		       PATHSPEC_LITERAL |
> -		       PATHSPEC_GLOB |
> -		       PATHSPEC_ICASE |
> -		       PATHSPEC_EXCLUDE);
> +                       PATHSPEC_FROMTOP |
> +                       PATHSPEC_MAXDEPTH |
> +                       PATHSPEC_LITERAL |
> +                       PATHSPEC_GLOB |
> +                       PATHSPEC_ICASE |
> +                       PATHSPEC_EXCLUDE |
> +                       PATHSPEC_ATTR);

Why this reindent?

> @@ -239,16 +254,100 @@ test_expect_success 'fail on multiple attr specifie
>  	test_i18ngrep "Only one" actual
>  '
>  
> -test_expect_success 'fail if attr magic is used places not implemented' '
> +test_expect_success 'fail if attr magic is used in places not implemented' '
>  	# The main purpose of this test is to check that we actually fail
>  	# when you attempt to use attr magic in commands that do not implement
> -	# attr magic. This test does not advocate git-add to stay that way,
> -	# though, but git-add is convenient as it has its own internal pathspec
> -	# parsing.
> -	test_must_fail git add ":(attr:labelB)" 2>actual &&
> +	# attr magic. This test does not advocate stash push to stay that way.
> +	# When you teach the command to grok the pathspec, you need to find
> +	# another commnad to replace it for the test.

There is a typo here.  Please do not expect your reviewers to always
offer a perfect solution to your problem and blindly copy what they
fed you.  Instead, just like other project participants try to find
bugs and improvement opportunities in your patch, please lend them
sanity-checking eyeballs in return ;-)

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH 1/1] add: enable attr pathspec magic
  2023-10-07 10:10       ` [PATCH " Junio C Hamano
@ 2023-10-11 20:20         ` Joanna Wang
  2023-11-02  1:55           ` [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash Joanna Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Joanna Wang @ 2023-10-11 20:20 UTC (permalink / raw)
  To: gitster; +Cc: git, jojwang

This lets users limit added files or exclude files based on file
attributes. For example, the chromium project would like to use
this like "git add --all ':(exclude,attr:submodule)'", as submodules
are managed in a unique way and often results in submodule changes
that users do not want in their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).

With this patch, attr is supported. It is possible that when the attr
pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few
GUARD_PATHSPEC() invocations. Later, to get a more user-friendly error
message when attr was used with git-add, PATHSPEC_ATTR was added as a
mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).

git-stash which goes through the same GUARD_PATHSPEC(), currently
does not work with attr.
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/T/#u
So a PATHSPEC_ATTR mask has been added to its parse_pathspec and
parse_pathspec_file().

Signed-off-by: Joanna Wang <jojwang@google.com>
---

> Do you know what exactly is not ready, so that perhaps others can
> help figuring out how to make it ready for the attr magic?
I have filed a bug which describes what I have found:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/T/#u

> Why this reindent?
Fixed

> There is a typo here.
Fixed


 builtin/add.c                  |   7 ++-
 builtin/stash.c                |   7 ++-
 dir.c                          |   3 +-
 t/t6135-pathspec-with-attrs.sh | 106 +++++++++++++++++++++++++++++++--
 4 files changed, 112 insertions(+), 11 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..2de83964a3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/builtin/stash.c b/builtin/stash.c
index 1ad496985a..af1b3a7146 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1760,7 +1760,10 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 		}
 	}
 
-	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
+        // For parse_pathspec() and parse_pathspec_file() below, PATHSPEC_ATTR is blocked for git stash
+        // because the magic attr does not get properly parsed when the PATHSPEC_PREFIX_ORIGIN flag is
+        // used, resulting in incorrect file filtering for attr.
+	parse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
 		       prefix, argv);
 
 	if (pathspec_from_file) {
@@ -1773,7 +1776,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 		if (ps.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&ps, 0,
+		parse_pathspec_file(&ps, PATHSPEC_ATTR,
 				    PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
 				    prefix, pathspec_from_file, pathspec_file_nul);
 	} else if (pathspec_file_nul) {
diff --git a/dir.c b/dir.c
index 8486e4d56f..ee3f3777df 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..403ee5e6b6 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,97 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate stash push to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another command to replace it for the test.
+	test_must_fail git stash push ":(attr:labelB)" 2>actual &&
+	test_i18ngrep "magic not supported" actual
+'
+
+test_expect_success 'fail if attr magic is used in --pathspec-from-file when not implemented' '
+	# This is like the test above but for attr magic passed in via --pathspec-from-file.
+	cat <<-\EOF >pathspec_file &&
+	:(attr:labelB)
+	EOF
+	test_must_fail git stash push --pathspec-from-file=pathspec_file 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.42.0.609.gbb76f46606-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-10-11 20:20         ` [PATCH 1/1] add: enable attr pathspec magic Joanna Wang
@ 2023-11-02  1:55           ` Joanna Wang
  2023-11-02  5:00             ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Joanna Wang @ 2023-11-02  1:55 UTC (permalink / raw)
  To: jojwang; +Cc: git, gitster

This lets users limit files or exclude files based on file
attributes during git-add and git-stash.
For example, the chromium project would like to use this like
"git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
unique way and often results in submodule changes that users do not want in
their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add and git-stash, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
which was the case for git-stash. More details here:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/

It is possible that when the attr pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.

Later, to get a more user-friendly error message when attr was used with git-add,
PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
However, this user-friendly error message was never added for git-stash.

Signed-off-by: Joanna Wang <jojwang@google.com>

---

PTAL.
I've updated this to include the fix for git-stash. I was initially going to fix this bug [1]
separately, but I thought it would make more sense to put everything in one patch so attr
could be enabled for both git-add and git-stash and tested.

[1] https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/



 builtin/add.c                  |   7 ++-
 dir.c                          |   3 +-
 pathspec.c                     |  11 +++-
 t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
 4 files changed, 118 insertions(+), 11 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 5126d2ede3..d46e4d10e9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 16fdb03f2a..4d1cd039be 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/pathspec.c b/pathspec.c
index bb1efe1f39..40bd8a8819 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,7 +109,7 @@ static struct pathspec_magic {
 	{ PATHSPEC_ATTR,    '\0', "attr" },
 };
 
-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
 {
 	int i;
 	strbuf_addstr(sb, ":(");
@@ -118,6 +118,13 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
 			if (sb->buf[sb->len - 1] != '(')
 				strbuf_addch(sb, ',');
 			strbuf_addstr(sb, pathspec_magic[i].name);
+			if (pathspec_magic[i].bit & PATHSPEC_ATTR) {
+				/* extract and insert the attr magic value */
+				char* p = strstr(element, "attr:");
+				char buff[128];
+				sscanf(p, "attr%[^)|,]", buff);
+				strbuf_addstr(sb, buff);
+			}
 		}
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
@@ -493,7 +500,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		struct strbuf sb = STRBUF_INIT;
 
 		/* Preserve the actual prefix length of each pattern */
-		prefix_magic(&sb, prefixlen, element_magic);
+		prefix_magic(&sb, prefixlen, element_magic, elt);
 
 		strbuf_addstr(&sb, match);
 		item->original = strbuf_detach(&sb, NULL);
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e46fa176ed 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate check-ignore to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another command to replace it for the test.
+	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git stash push' '
+	cat <<-\EOF >expect &&
+	A	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
+	git stash show --include-untracked --name-status >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.42.0.820.g83a721a137-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-02  1:55           ` [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash Joanna Wang
@ 2023-11-02  5:00             ` Junio C Hamano
  2023-11-02 17:53               ` Joanna Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-11-02  5:00 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Joanna Wang <jojwang@google.com> writes:

> I've updated this to include the fix for git-stash. I was
> initially going to fix this bug [1] separately, but I thought it
> would make more sense to put everything in one patch so attr could
> be enabled for both git-add and git-stash and tested.

I think that is perfectly fine, as long as it is described well in
the proposed log message what is done in the patch, and how two
seemingly different things done in the patch are so closely linked
that it makes sense to do so in a single patch.

Will queue.

Thanks.

> diff --git a/pathspec.c b/pathspec.c
> index bb1efe1f39..40bd8a8819 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -109,7 +109,7 @@ static struct pathspec_magic {
>  	{ PATHSPEC_ATTR,    '\0', "attr" },
>  };
>  
> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
>  {
>  	int i;
>  	strbuf_addstr(sb, ":(");
> @@ -118,6 +118,13 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
>  			if (sb->buf[sb->len - 1] != '(')
>  				strbuf_addch(sb, ',');
>  			strbuf_addstr(sb, pathspec_magic[i].name);
> +			if (pathspec_magic[i].bit & PATHSPEC_ATTR) {
> +				/* extract and insert the attr magic value */
> +				char* p = strstr(element, "attr:");

In our codebase, the asterisk sticks to the variable, not to the
type, i.e.

				char *p = strstr(element, "attr:");

> +				char buff[128];

Will this fixed-size buffer make an inviting target for script
kiddies, as pathspec often come from the command line and under
control by whoever is making a request?

Aren't we going to copy what is in the elt literally from where
attr: appears up to the next delimiter like ',', ')'?  I am not sure
if we need a separate buffer.  Would something along this line work?

				strbuf_add(sb, p, strcspn(p, ",)");

I am unsure if this "unparsing" is the way we want to go.  For one,
just like %(attr:foo) can take an arbitrary end-user supplied string
in the "foo" part, we can have new pathspec magic in the future that
will take an arbitrary end-user supplied string as its value.  And the
above unparsing code will be utterly confused when that value
happens to contain "attr:" as its substring, e.g., 

    $ git add . ":(exclude,frotz:diattr:0,attr:submodule)"

Also, do we (and if not right now, then do we want to) support
giving more than one attribute?

    $ git add ":(attr:text,attr:small)*.py"

Not supporting multiple attributes would be OK for now, but when it
becomes needed, the "unparse using the bits in the element_magic"
does not look like the right approach.

Indeed, if you are going to pass the original "elt" string *anyway*,
you have all the info in there that you need.  I wonder if it makes
sense to get rid of the "unsigned magic" bitset from the parameter,
and discard the loop over the pathspec_magic[] array and do
something like:

    - if the original "elt" already has some magic (i.e., begins
      with ":(" and not in the literal mode), then copy them
      literally and textually from ":(" up to the closing ")", but
      also insert the necessary "prefix:<num>" magic;

    - otherwise, give ":(prefix:<num>)" magic.

without even attempting to unparse the bits in "element_magic"?

Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-02  5:00             ` Junio C Hamano
@ 2023-11-02 17:53               ` Joanna Wang
  2023-11-02 21:32                 ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Joanna Wang @ 2023-11-02 17:53 UTC (permalink / raw)
  To: gitster; +Cc: git, jojwang

This lets users limit files or exclude files based on file
attributes during git-add and git-stash.
For example, the chromium project would like to use this like
"git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
unique way and often results in submodule changes that users do not want in
their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add and git-stash, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
which was the case for git-stash. More details here:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/

It is possible that when the attr pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.

Later, to get a more user-friendly error message when attr was used with git-add,
PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
However, this user-friendly error message was never added for git-stash.

Signed-off-by: Joanna Wang <jojwang@google.com>

---

> Indeed, if you are going to pass the original "elt" string *anyway*,
> you have all the info in there that you need.  I wonder if it makes
> sense to get rid of the "unsigned magic" bitset from the parameter,
This was my initial strategy but ran into trouble when the magic was
in shorthand form. Upon closer look at how the shorthand works
(e.g. shorthand and longhand can never mix so
':!/(attr:chicken)file' would make <(attr:chicken)file> the match string)
I tried this again by processing the forms separately.
It would still need both the element and element_magic, but I think it
addresses the concerns with future changes where multiple magic match
values could be allowed and the match values could be any string. These
changes would be fine as long as there is no overlap between magic that
takes a user-supplied value and magic that can be expressed in shorthand.


 builtin/add.c                  |   7 ++-
 dir.c                          |   3 +-
 pathspec.c                     |  25 +++++---
 t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
 4 files changed, 125 insertions(+), 18 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 5126d2ede3..d46e4d10e9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 16fdb03f2a..4d1cd039be 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/pathspec.c b/pathspec.c
index bb1efe1f39..588a2cde4d 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,16 +109,23 @@ static struct pathspec_magic {
 	{ PATHSPEC_ATTR,    '\0', "attr" },
 };
 
-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
 {
-	int i;
-	strbuf_addstr(sb, ":(");
-	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
-		if (magic & pathspec_magic[i].bit) {
-			if (sb->buf[sb->len - 1] != '(')
-				strbuf_addch(sb, ',');
-			strbuf_addstr(sb, pathspec_magic[i].name);
+	if (element[1] != '(') {
+		/* Process an element in shorthand form (e.g. ":!/<match>") */
+		strbuf_addstr(sb, ":(");
+		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+			if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
+				if (sb->buf[sb->len - 1] != '(')
+					strbuf_addch(sb, ',');
+				strbuf_addstr(sb, pathspec_magic[i].name);
+			}
 		}
+	} else {
+		/* For an element in longhand form, we simply copy everything up to the final ')' */
+		int len = strchr(element, ')') - element;
+		strbuf_add(sb, element, len);
+	}
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
@@ -493,7 +500,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		struct strbuf sb = STRBUF_INIT;
 
 		/* Preserve the actual prefix length of each pattern */
-		prefix_magic(&sb, prefixlen, element_magic);
+		prefix_magic(&sb, prefixlen, element_magic, elt);
 
 		strbuf_addstr(&sb, match);
 		item->original = strbuf_detach(&sb, NULL);
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e46fa176ed 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate check-ignore to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another command to replace it for the test.
+	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git stash push' '
+	cat <<-\EOF >expect &&
+	A	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
+	git stash show --include-untracked --name-status >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.42.0.869.gea05f2083d-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-02 17:53               ` Joanna Wang
@ 2023-11-02 21:32                 ` Junio C Hamano
  2023-11-02 23:45                   ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-11-02 21:32 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Joanna Wang <jojwang@google.com> writes:

>> Indeed, if you are going to pass the original "elt" string *anyway*,
>> you have all the info in there that you need.  I wonder if it makes
>> sense to get rid of the "unsigned magic" bitset from the parameter,
> This was my initial strategy but ran into trouble when the magic was
> in shorthand form. Upon closer look at how the shorthand works
> (e.g. shorthand and longhand can never mix so
> ':!/(attr:chicken)file' would make <(attr:chicken)file> the match string)
> I tried this again by processing the forms separately.
> It would still need both the element and element_magic, but I think it
> addresses the concerns with future changes where multiple magic match
> values could be allowed and the match values could be any string.

The "bits" were acceptable for things like "exclude" and "icase"
because it does not matter how many times you gave them and they do
not take any additional parameters, but attr is different in that it
takes a value, and multiple instances with different values can be
given.  It is lucky that we did not allow mixing the short and long
forms ;-)

> These changes would be fine as long as there is no overlap between
> magic that takes a user-supplied value and magic that can be
> expressed in shorthand.

Indeed.  Thanks for thinking this through.

> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
>  {
> -	int i;
> -	strbuf_addstr(sb, ":(");
> -	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
> -		if (magic & pathspec_magic[i].bit) {
> -			if (sb->buf[sb->len - 1] != '(')
> -				strbuf_addch(sb, ',');
> -			strbuf_addstr(sb, pathspec_magic[i].name);

At this point in the code, is it guaranteed that element[0] is ':'
and never a NUL?  Also is it guaranteed that element has ')'
somewhere later if element[1] is '('?

"Otherwise the caller would have failed to parse the pathspec magic
into the magic bits, and this helper function would not have been
called" is the answer I am expecting, but I didn't check if the
caller refrains from calling us.  It would be better to have a brief
comment explaining why a seemingly loose parsing of element[] string
is OK to save future readers from wondering the same thing as I did
here.

> +	if (element[1] != '(') {
> +		/* Process an element in shorthand form (e.g. ":!/<match>") */
> +		strbuf_addstr(sb, ":(");
> +		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +			if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
> +				if (sb->buf[sb->len - 1] != '(')
> +					strbuf_addch(sb, ',');
> +				strbuf_addstr(sb, pathspec_magic[i].name);
> +			}
>  		}
> +	} else {
> +		/* For an element in longhand form, we simply copy everything up to the final ')' */

A comment that is a bit on the overly-long side.

> +		int len = strchr(element, ')') - element;
> +		strbuf_add(sb, element, len);
> +	}
>  	strbuf_addf(sb, ",prefix:%d)", prefixlen);
>  }

Come to think of it, this part of the change could stand on its own
as an independent bugfix.  I wonder if this existing bug caused by
failing to copy the value of "attr:" is triggerable from a codepath
that already allows PATHSPEC_ATTR magic.  Not absolutely required
when the rest of the patch is in reasonably close to the finish
line, but it would narrow the scope of the new feature proper to
treat this as a separate and independent fix, on which the new
fature depends on.

Thanks for working on fixing this rather old bug.  I think we should
have noticed when we added the support for the "attr" magic to the
pathspec API.


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-02 21:32                 ` Junio C Hamano
@ 2023-11-02 23:45                   ` Junio C Hamano
  2023-11-03 14:35                     ` Joanna Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2023-11-02 23:45 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

>> +	} else {
>> +		/* For an element in longhand form, we simply copy everything up to the final ')' */
>
> A comment that is a bit on the overly-long side.
>
>> +		int len = strchr(element, ')') - element;
>> +		strbuf_add(sb, element, len);

In practice, nobody sane would write a pathspec magic that is over
2GB in size, so this would not matter unless we are facing a
potential attacker, but as the third parameter strbuf_add() takes is
of type size_t, it would not hurt to define "len" as the same type
as well.

> Thanks for working on fixing this rather old bug.  I think we should
> have noticed when we added the support for the "attr" magic to the
> pathspec API.


^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-02 23:45                   ` Junio C Hamano
@ 2023-11-03 14:35                     ` Joanna Wang
  2023-11-03 15:31                       ` Ruben Safir
  2023-11-03 16:34                       ` Joanna Wang
  0 siblings, 2 replies; 19+ messages in thread
From: Joanna Wang @ 2023-11-03 14:35 UTC (permalink / raw)
  To: gitster; +Cc: git, jojwang

This lets users limit files or exclude files based on file
attributes during git-add and git-stash.
For example, the chromium project would like to use this like
"git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
unique way and often results in submodule changes that users do not want in
their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add and git-stash, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
which was the case for git-stash.
(But originally filed here:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)

Furthermore, while other commands hit this code path it did not result in unexpected
behavior because this bug only impacts the pathspec->items->original field which is
NOT used to filter paths. However, git-stash does use pathspec->items->original when
building args used to call other git commands.
(See add_pathspecs() usage and implementation in stash.c)

It is possible that when the attr pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.

Later, to get a more user-friendly error message when attr was used with git-add,
PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
However, this user-friendly error message was never added for git-stash.

Signed-off-by: Joanna Wang <jojwang@google.com>

---

> At this point in the code, is it guaranteed that element[0] is ':'
> and never a NUL?  Also is it guaranteed that element has ')'
> somewhere later if element[1] is '('?
No sorry, we can only assume this if there was element magic detected
by parse_element_magic(). So if element magic is not 0, we know the body
is in the expected form (either long or short).
I have added comments and a check for magic to guard against this.

>  I wonder if this existing bug caused by
> failing to copy the value of "attr:" is triggerable from a codepath
> that already allows PATHSPEC_ATTR magic.
While there are other commands that go through the prefix_magic path (like `git-add -i`),
AFAICT they are not actually impacted by the bug. The bug only impacts the value of
pathspec->items->original which is not used during pathspec matching. But git-stash
uses `original` to pass in as args to other commands it calls. I've included this
detail in the description above.

> but as the third parameter strbuf_add() takes is of type size_t, it would not
> hurt to define "len" as the same type as well.
Thanks for spotting. fixed.

 builtin/add.c                  |   7 ++-
 dir.c                          |   3 +-
 pathspec.c                     |  36 ++++++++---
 t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
 4 files changed, 136 insertions(+), 18 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 5126d2ede3..d46e4d10e9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 16fdb03f2a..4d1cd039be 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/pathspec.c b/pathspec.c
index bb1efe1f39..2f8721cc15 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,16 +109,34 @@ static struct pathspec_magic {
 	{ PATHSPEC_ATTR,    '\0', "attr" },
 };
 
-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
 {
-	int i;
-	strbuf_addstr(sb, ":(");
-	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
-		if (magic & pathspec_magic[i].bit) {
-			if (sb->buf[sb->len - 1] != '(')
-				strbuf_addch(sb, ',');
-			strbuf_addstr(sb, pathspec_magic[i].name);
+	/* No magic was found in element, just add prefix magic */
+	if (magic == 0) {
+		strbuf_addf(sb, ":(prefix:%d)", prefixlen);
+		return;
+	}
+
+	/*
+	 * At this point we known that parse_element_magic() was able to extract some pathspec
+	 * magic from element. So we know element is correctly formatted in either shorthand
+	 * or longhand form
+	 */
+	if (element[1] != '(') {
+		/* Process an element in shorthand form (e.g. ":!/<match>") */
+		strbuf_addstr(sb, ":(");
+		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+			if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
+				if (sb->buf[sb->len - 1] != '(')
+					strbuf_addch(sb, ',');
+				strbuf_addstr(sb, pathspec_magic[i].name);
+			}
 		}
+	} else {
+		/* For the longhand form, we copy everything up to the final ')' */
+		size_t len = strchr(element, ')') - element;
+		strbuf_add(sb, element, len);
+	}
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
@@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		struct strbuf sb = STRBUF_INIT;
 
 		/* Preserve the actual prefix length of each pattern */
-		prefix_magic(&sb, prefixlen, element_magic);
+		prefix_magic(&sb, prefixlen, element_magic, elt);
 
 		strbuf_addstr(&sb, match);
 		item->original = strbuf_detach(&sb, NULL);
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e46fa176ed 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate check-ignore to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another command to replace it for the test.
+	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git stash push' '
+	cat <<-\EOF >expect &&
+	A	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
+	git stash show --include-untracked --name-status >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.42.0.869.gea05f2083d-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-03 14:35                     ` Joanna Wang
@ 2023-11-03 15:31                       ` Ruben Safir
  2023-11-03 16:34                       ` Joanna Wang
  1 sibling, 0 replies; 19+ messages in thread
From: Ruben Safir @ 2023-11-03 15:31 UTC (permalink / raw)
  To: Joanna Wang; +Cc: gitster, git

Is that really you name or is it a bad joke?


On Fri, Nov 03, 2023 at 02:35:07PM +0000, Joanna Wang wrote:
> This lets users limit files or exclude files based on file
> attributes during git-add and git-stash.
> For example, the chromium project would like to use this like
> "git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
> unique way and often results in submodule changes that users do not want in
> their commits.
> 
> This does not change any attr magic implementation. It is only adding
> attr as an allowed pathspec in git-add and git-stash, which was previously
> blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
> However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
> This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
> which was the case for git-stash.
> (But originally filed here:
> https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)
> 
> Furthermore, while other commands hit this code path it did not result in unexpected
> behavior because this bug only impacts the pathspec->items->original field which is
> NOT used to filter paths. However, git-stash does use pathspec->items->original when
> building args used to call other git commands.
> (See add_pathspecs() usage and implementation in stash.c)
> 
> It is possible that when the attr pathspec feature was first added in
> b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
> "PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.
> 
> Later, to get a more user-friendly error message when attr was used with git-add,
> PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
> 84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
> However, this user-friendly error message was never added for git-stash.
> 
> Signed-off-by: Joanna Wang <jojwang@google.com>
> 
> ---
> 
> > At this point in the code, is it guaranteed that element[0] is ':'
> > and never a NUL?  Also is it guaranteed that element has ')'
> > somewhere later if element[1] is '('?
> No sorry, we can only assume this if there was element magic detected
> by parse_element_magic(). So if element magic is not 0, we know the body
> is in the expected form (either long or short).
> I have added comments and a check for magic to guard against this.
> 
> >  I wonder if this existing bug caused by
> > failing to copy the value of "attr:" is triggerable from a codepath
> > that already allows PATHSPEC_ATTR magic.
> While there are other commands that go through the prefix_magic path (like `git-add -i`),
> AFAICT they are not actually impacted by the bug. The bug only impacts the value of
> pathspec->items->original which is not used during pathspec matching. But git-stash
> uses `original` to pass in as args to other commands it calls. I've included this
> detail in the description above.
> 
> > but as the third parameter strbuf_add() takes is of type size_t, it would not
> > hurt to define "len" as the same type as well.
> Thanks for spotting. fixed.
> 
>  builtin/add.c                  |   7 ++-
>  dir.c                          |   3 +-
>  pathspec.c                     |  36 ++++++++---
>  t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
>  4 files changed, 136 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 5126d2ede3..d46e4d10e9 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	 * Check the "pathspec '%s' did not match any files" block
>  	 * below before enabling new magic.
>  	 */
> -	parse_pathspec(&pathspec, PATHSPEC_ATTR,
> +	parse_pathspec(&pathspec, 0,
>  		       PATHSPEC_PREFER_FULL |
>  		       PATHSPEC_SYMLINK_LEADING_PATH,
>  		       prefix, argv);
> @@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		if (pathspec.nr)
>  			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
>  
> -		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
> +		parse_pathspec_file(&pathspec, 0,
>  				    PATHSPEC_PREFER_FULL |
>  				    PATHSPEC_SYMLINK_LEADING_PATH,
>  				    prefix, pathspec_from_file, pathspec_file_nul);
> @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			       PATHSPEC_LITERAL |
>  			       PATHSPEC_GLOB |
>  			       PATHSPEC_ICASE |
> -			       PATHSPEC_EXCLUDE);
> +			       PATHSPEC_EXCLUDE |
> +			       PATHSPEC_ATTR);
>  
>  		for (i = 0; i < pathspec.nr; i++) {
>  			const char *path = pathspec.items[i].match;
> diff --git a/dir.c b/dir.c
> index 16fdb03f2a..4d1cd039be 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
>  		       PATHSPEC_LITERAL |
>  		       PATHSPEC_GLOB |
>  		       PATHSPEC_ICASE |
> -		       PATHSPEC_EXCLUDE);
> +		       PATHSPEC_EXCLUDE |
> +		       PATHSPEC_ATTR);
>  
>  	for (i = 0; i < pathspec->nr; i++) {
>  		const struct pathspec_item *item = &pathspec->items[i];
> diff --git a/pathspec.c b/pathspec.c
> index bb1efe1f39..2f8721cc15 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -109,16 +109,34 @@ static struct pathspec_magic {
>  	{ PATHSPEC_ATTR,    '\0', "attr" },
>  };
>  
> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
>  {
> -	int i;
> -	strbuf_addstr(sb, ":(");
> -	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
> -		if (magic & pathspec_magic[i].bit) {
> -			if (sb->buf[sb->len - 1] != '(')
> -				strbuf_addch(sb, ',');
> -			strbuf_addstr(sb, pathspec_magic[i].name);
> +	/* No magic was found in element, just add prefix magic */
> +	if (magic == 0) {
> +		strbuf_addf(sb, ":(prefix:%d)", prefixlen);
> +		return;
> +	}
> +
> +	/*
> +	 * At this point we known that parse_element_magic() was able to extract some pathspec
> +	 * magic from element. So we know element is correctly formatted in either shorthand
> +	 * or longhand form
> +	 */
> +	if (element[1] != '(') {
> +		/* Process an element in shorthand form (e.g. ":!/<match>") */
> +		strbuf_addstr(sb, ":(");
> +		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +			if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
> +				if (sb->buf[sb->len - 1] != '(')
> +					strbuf_addch(sb, ',');
> +				strbuf_addstr(sb, pathspec_magic[i].name);
> +			}
>  		}
> +	} else {
> +		/* For the longhand form, we copy everything up to the final ')' */
> +		size_t len = strchr(element, ')') - element;
> +		strbuf_add(sb, element, len);
> +	}
>  	strbuf_addf(sb, ",prefix:%d)", prefixlen);
>  }
>  
> @@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
>  		struct strbuf sb = STRBUF_INIT;
>  
>  		/* Preserve the actual prefix length of each pattern */
> -		prefix_magic(&sb, prefixlen, element_magic);
> +		prefix_magic(&sb, prefixlen, element_magic, elt);
>  
>  		strbuf_addstr(&sb, match);
>  		item->original = strbuf_detach(&sb, NULL);
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index f70c395e75..e46fa176ed 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
>  	fileSetLabel label
>  	fileValue label=foo
>  	fileWrongLabel label☺
> +	newFileA* labelA
> +	newFileB* labelB
>  	EOF
>  	echo fileSetLabel label1 >sub/.gitattributes &&
>  	git add .gitattributes sub/.gitattributes &&
>  	git commit -m "add attributes"
>  '
>  
> +test_expect_success 'setup .gitignore' '
> +	cat <<-\EOF >.gitignore &&
> +	actual
> +	expect
> +	pathspec_file
> +	EOF
> +	git add .gitignore &&
> +	git commit -m "add gitignore"
> +'
> +
>  test_expect_success 'check specific set attr' '
>  	cat <<-\EOF >expect &&
>  	fileSetLabel
> @@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
>  test_expect_success 'check unspecified attr' '
>  	cat <<-\EOF >expect &&
>  	.gitattributes
> +	.gitignore
>  	fileA
>  	fileAB
>  	fileAC
> @@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
>  test_expect_success 'check unspecified attr (2)' '
>  	cat <<-\EOF >expect &&
>  	HEAD:.gitattributes
> +	HEAD:.gitignore
>  	HEAD:fileA
>  	HEAD:fileAB
>  	HEAD:fileAC
> @@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
>  test_expect_success 'check multiple unspecified attr' '
>  	cat <<-\EOF >expect &&
>  	.gitattributes
> +	.gitignore
>  	fileC
>  	fileNoLabel
>  	fileWrongLabel
> @@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
>  	test_i18ngrep "Only one" actual
>  '
>  
> -test_expect_success 'fail if attr magic is used places not implemented' '
> +test_expect_success 'fail if attr magic is used in places not implemented' '
>  	# The main purpose of this test is to check that we actually fail
>  	# when you attempt to use attr magic in commands that do not implement
> -	# attr magic. This test does not advocate git-add to stay that way,
> -	# though, but git-add is convenient as it has its own internal pathspec
> -	# parsing.
> -	test_must_fail git add ":(attr:labelB)" 2>actual &&
> +	# attr magic. This test does not advocate check-ignore to stay that way.
> +	# When you teach the command to grok the pathspec, you need to find
> +	# another command to replace it for the test.
> +	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
>  	test_i18ngrep "magic not supported" actual
>  '
>  
> +test_expect_success 'check that attr magic works for git stash push' '
> +	cat <<-\EOF >expect &&
> +	A	sub/newFileA-foo
> +	EOF
> +	>sub/newFileA-foo &&
> +	>sub/newFileB-foo &&
> +	git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
> +	git stash show --include-untracked --name-status >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add --all' '
> +	cat <<-\EOF >expect &&
> +	sub/newFileA-foo
> +	EOF
> +	>sub/newFileA-foo &&
> +	>sub/newFileB-foo &&
> +	git add --all ":(exclude,attr:labelB)" &&
> +	git diff --name-only --cached >actual &&
> +	git restore -W -S . &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add -u' '
> +	cat <<-\EOF >expect &&
> +	sub/fileA
> +	EOF
> +	>sub/newFileA-foo &&
> +	>sub/newFileB-foo &&
> +	>sub/fileA &&
> +	>sub/fileB &&
> +	git add -u ":(exclude,attr:labelB)" &&
> +	git diff --name-only --cached  >actual &&
> +	git restore -S -W . && rm sub/new* &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add <path>' '
> +	cat <<-\EOF >expect &&
> +	fileA
> +	fileB
> +	sub/fileA
> +	EOF
> +	>fileA &&
> +	>fileB &&
> +	>sub/fileA &&
> +	>sub/fileB &&
> +	git add ":(exclude,attr:labelB)sub/*" &&
> +	git diff --name-only --cached >actual &&
> +	git restore -S -W . &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git -add .' '
> +	cat <<-\EOF >expect &&
> +	sub/fileA
> +	EOF
> +	>fileA &&
> +	>fileB &&
> +	>sub/fileA &&
> +	>sub/fileB &&
> +	cd sub &&
> +	git add . ":(exclude,attr:labelB)" &&
> +	cd .. &&
> +	git diff --name-only --cached >actual &&
> +	git restore -S -W . &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
> +	cat <<-\EOF >pathspec_file &&
> +	:(exclude,attr:labelB)
> +	EOF
> +	cat <<-\EOF >expect &&
> +	sub/newFileA-foo
> +	EOF
> +	>sub/newFileA-foo &&
> +	>sub/newFileB-foo &&
> +	git add --all --pathspec-from-file=pathspec_file &&
> +	git diff --name-only --cached >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'abort on giving invalid label on the command line' '
>  	test_must_fail git ls-files . ":(attr:☺)"
>  '
> -- 
> 2.42.0.869.gea05f2083d-goog
> 

-- 
So many immigrant groups have swept through our town
that Brooklyn, like Atlantis, reaches mythological
proportions in the mind of the world - RI Safir 1998
http://www.mrbrklyn.com 

DRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002
http://www.nylxs.com - Leadership Development in Free Software
http://www2.mrbrklyn.com/resources - Unpublished Archive 
http://www.coinhangout.com - coins!
http://www.brooklyn-living.com 

Being so tracked is for FARM ANIMALS and extermination camps, 
but incompatible with living as a free human being. -RI Safir 2013


^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-03 14:35                     ` Joanna Wang
  2023-11-03 15:31                       ` Ruben Safir
@ 2023-11-03 16:34                       ` Joanna Wang
  2023-11-04  5:11                         ` Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: Joanna Wang @ 2023-11-03 16:34 UTC (permalink / raw)
  To: jojwang; +Cc: git, gitster

This lets users limit files or exclude files based on file
attributes during git-add and git-stash.
For example, the chromium project would like to use this like
"git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
unique way and often results in submodule changes that users do not want in
their commits.

This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add and git-stash, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
which was the case for git-stash.
(Bug originally filed here:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)

Furthermore, while other commands hit this code path it did not result in unexpected
behavior because this bug only impacts the pathspec->items->original field which is
NOT used to filter paths. However, git-stash does use pathspec->items->original when
building args used to call other git commands.
(See add_pathspecs() usage and implementation in stash.c)

It is possible that when the attr pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.

Later, to get a more user-friendly error message when attr was used with git-add,
PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
However, this user-friendly error message was never added for git-stash.

Signed-off-by: Joanna Wang <jojwang@google.com>

---

I fixed a typo above. "(But originally" -> "(Bug originally"

> So if element magic is not 0, we know the body is in the expected
> form (either long or short).
Also while this is still true AFAICT, I found an existing bug in parse_short_magic()
where it does not handle paths that start with ":" where ":" is part of the
file name. e.g. "git stash push -- ':chicken'" is not handled correcly. But I feel
this is a seperate topic from this patch. So I'll file a bug to address it seperately.

 builtin/add.c                  |   7 ++-
 dir.c                          |   3 +-
 pathspec.c                     |  36 ++++++++---
 t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
 4 files changed, 136 insertions(+), 18 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 5126d2ede3..d46e4d10e9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 16fdb03f2a..4d1cd039be 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/pathspec.c b/pathspec.c
index bb1efe1f39..2f8721cc15 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,16 +109,34 @@ static struct pathspec_magic {
 	{ PATHSPEC_ATTR,    '\0', "attr" },
 };
 
-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
 {
-	int i;
-	strbuf_addstr(sb, ":(");
-	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
-		if (magic & pathspec_magic[i].bit) {
-			if (sb->buf[sb->len - 1] != '(')
-				strbuf_addch(sb, ',');
-			strbuf_addstr(sb, pathspec_magic[i].name);
+	/* No magic was found in element, just add prefix magic */
+	if (magic == 0) {
+		strbuf_addf(sb, ":(prefix:%d)", prefixlen);
+		return;
+	}
+
+	/*
+	 * At this point we known that parse_element_magic() was able to extract some pathspec
+	 * magic from element. So we know element is correctly formatted in either shorthand
+	 * or longhand form
+	 */
+	if (element[1] != '(') {
+		/* Process an element in shorthand form (e.g. ":!/<match>") */
+		strbuf_addstr(sb, ":(");
+		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+			if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
+				if (sb->buf[sb->len - 1] != '(')
+					strbuf_addch(sb, ',');
+				strbuf_addstr(sb, pathspec_magic[i].name);
+			}
 		}
+	} else {
+		/* For the longhand form, we copy everything up to the final ')' */
+		size_t len = strchr(element, ')') - element;
+		strbuf_add(sb, element, len);
+	}
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
@@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		struct strbuf sb = STRBUF_INIT;
 
 		/* Preserve the actual prefix length of each pattern */
-		prefix_magic(&sb, prefixlen, element_magic);
+		prefix_magic(&sb, prefixlen, element_magic, elt);
 
 		strbuf_addstr(&sb, match);
 		item->original = strbuf_detach(&sb, NULL);
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e46fa176ed 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate check-ignore to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another command to replace it for the test.
+	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git stash push' '
+	cat <<-\EOF >expect &&
+	A	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
+	git stash show --include-untracked --name-status >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.42.0.869.gea05f2083d-goog


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-03 16:34                       ` Joanna Wang
@ 2023-11-04  5:11                         ` Junio C Hamano
  2023-11-04  6:28                           ` Eric Sunshine
  2023-11-09 23:27                           ` Junio C Hamano
  0 siblings, 2 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-11-04  5:11 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Thanks.  The compiled result from this version looks quite good.  As
you started a new round with typofix, let me start the final
nitpicking.

Joanna Wang <jojwang@google.com> writes:

> This lets users limit files or exclude files based on file
> attributes during git-add and git-stash.
> For example, the chromium project would like to use this like
> "git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
> unique way and often results in submodule changes that users do not want in
> their commits.

I would say

    Allow users to limit or exclude files based on file attributes
    during git-add and git-stash.

    For example, the chromium project would like to use 

        $ git add . ':(exclude,attr:submodule)'

    as submodules are managed in a unique way and often results in
    submodule changes that users do not want in their commits.

except that I would prefer to see "in a unique way" rephrased.
Would "are managed outside Git using external tools" be a more
accurate description?

It may be true that the chromium project handles submodules unlike
all the other projects, but you guys being alone is not an important
part of the reason why we might want this new feature to help users.
When another project adopts the way how chromium manages the
submodules, forbidding its end-users from running "git add" to
record the changes to their submodules, you guys will no longer be
"unique", but the need for this feature will still exist (and the
demand may even be stronger).  The reason why this feature helps
should be stated here.

> This does not change any attr magic implementation. It is only adding
> attr as an allowed pathspec in git-add and git-stash, which was previously
> blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).

Add blank line here between paragraphs.

> However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
> This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
> which was the case for git-stash.
> (Bug originally filed here:

Wrap overlong lines here and in the rest of the proposed log message.

> diff --git a/pathspec.c b/pathspec.c
> index bb1efe1f39..2f8721cc15 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -109,16 +109,34 @@ static struct pathspec_magic {
>  	{ PATHSPEC_ATTR,    '\0', "attr" },
>  };
>  
> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
>  {

This is overly long.

> +	/* No magic was found in element, just add prefix magic */
> +	if (magic == 0) {

Style:

	if (!magic) {

> +		strbuf_addf(sb, ":(prefix:%d)", prefixlen);
> +		return;
> +	}
> +

> +	/*
> +	 * At this point we known that parse_element_magic() was able to extract some pathspec
> +	 * magic from element. So we know element is correctly formatted in either shorthand
> +	 * or longhand form
> +	 */

Overly long, with a typo.  "At this point, we know that ...".

> +	if (element[1] != '(') {
> +		/* Process an element in shorthand form (e.g. ":!/<match>") */
> +		strbuf_addstr(sb, ":(");
> +		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +			if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {

Overly long, with a stylo.

> +				if (sb->buf[sb->len - 1] != '(')
> +					strbuf_addch(sb, ',');
> +				strbuf_addstr(sb, pathspec_magic[i].name);
> +			}
>  		}
> +	} else {
> +		/* For the longhand form, we copy everything up to the final ')' */
> +		size_t len = strchr(element, ')') - element;
> +		strbuf_add(sb, element, len);
> +	}
>  	strbuf_addf(sb, ",prefix:%d)", prefixlen);
>  }

Here is what I ended up queuing.

Thanks.

---- >8 -------- >8 -------- >8 -------- >8 ----
From: Joanna Wang <jojwang@google.com>
Date: Fri, 3 Nov 2023 16:34:48 +0000
Subject: [PATCH] attr: enable attr pathspec magic for git-add and git-stash

Allow users to limit or exclude files based on file attributes
during git-add and git-stash.

For example, the chromium project would like to use

    $ git add . ':(exclude,attr:submodule)'

as submodules are managed by an external tool, forbidding end users
to record changes with "git add".  Allowing "git add" to often
records changes that users do not want in their commits.

This commit does not change any attr magic implementation. It is
only adding attr as an allowed pathspec in git-add and git-stash,
which was previously blocked by GUARD_PATHSPEC and a pathspec mask
in parse_pathspec()).

However, we fix a bug in prefix_magic() where attr values were
unintentionally removed.  This was triggerable when parse_pathspec()
is called with PATHSPEC_PREFIX_ORIGIN as a flag, which was the case
for git-stash (Bug originally filed here [*])

Furthermore, while other commands hit this code path it did not
result in unexpected behavior because this bug only impacts the
pathspec->items->original field which is NOT used to filter
paths. However, git-stash does use pathspec->items->original when
building args used to call other git commands.  (See add_pathspecs()
usage and implementation in stash.c)

It is possible that when the attr pathspec feature was first added
in b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few
GUARD_PATHSPEC() invocations.

Later, to get a more user-friendly error message when attr was used
with git-add, PATHSPEC_ATTR was added as a mask to git-add's
invocation of parse_pathspec() 84d938b732 (add: do not accept
pathspec magic 'attr', 2018-09-18).  However, this user-friendly
error message was never added for git-stash.

[Reference]
 * https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)

Signed-off-by: Joanna Wang <jojwang@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c                  |   7 ++-
 dir.c                          |   3 +-
 pathspec.c                     |  39 +++++++++---
 t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
 4 files changed, 139 insertions(+), 18 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 5126d2ede3..d46e4d10e9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ATTR,
+	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		if (pathspec.nr)
 			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
 
-		parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+		parse_pathspec_file(&pathspec, 0,
 				    PATHSPEC_PREFER_FULL |
 				    PATHSPEC_SYMLINK_LEADING_PATH,
 				    prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
 			       PATHSPEC_ICASE |
-			       PATHSPEC_EXCLUDE);
+			       PATHSPEC_EXCLUDE |
+			       PATHSPEC_ATTR);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 16fdb03f2a..4d1cd039be 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	for (i = 0; i < pathspec->nr; i++) {
 		const struct pathspec_item *item = &pathspec->items[i];
diff --git a/pathspec.c b/pathspec.c
index bb1efe1f39..d132eefac2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,16 +109,37 @@ static struct pathspec_magic {
 	{ PATHSPEC_ATTR,    '\0', "attr" },
 };
 
-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen,
+			 unsigned magic, const char *element)
 {
-	int i;
-	strbuf_addstr(sb, ":(");
-	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
-		if (magic & pathspec_magic[i].bit) {
-			if (sb->buf[sb->len - 1] != '(')
-				strbuf_addch(sb, ',');
-			strbuf_addstr(sb, pathspec_magic[i].name);
+	/* No magic was found in element, just add prefix magic */
+	if (!magic) {
+		strbuf_addf(sb, ":(prefix:%d)", prefixlen);
+		return;
+	}
+
+	/*
+	 * At this point, we know that parse_element_magic() was able
+	 * to extract some pathspec magic from element. So we know
+	 * element is correctly formatted in either shorthand or
+	 * longhand form
+	 */
+	if (element[1] != '(') {
+		/* Process an element in shorthand form (e.g. ":!/<match>") */
+		strbuf_addstr(sb, ":(");
+		for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+			if ((magic & pathspec_magic[i].bit) &&
+			    (pathspec_magic[i].mnemonic)) {
+				if (sb->buf[sb->len - 1] != '(')
+					strbuf_addch(sb, ',');
+				strbuf_addstr(sb, pathspec_magic[i].name);
+			}
 		}
+	} else {
+		/* For the longhand form, we copy everything up to the final ')' */
+		size_t len = strchr(element, ')') - element;
+		strbuf_add(sb, element, len);
+	}
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
@@ -493,7 +514,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		struct strbuf sb = STRBUF_INIT;
 
 		/* Preserve the actual prefix length of each pattern */
-		prefix_magic(&sb, prefixlen, element_magic);
+		prefix_magic(&sb, prefixlen, element_magic, elt);
 
 		strbuf_addstr(&sb, match);
 		item->original = strbuf_detach(&sb, NULL);
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e46fa176ed 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
 	fileSetLabel label
 	fileValue label=foo
 	fileWrongLabel label☺
+	newFileA* labelA
+	newFileB* labelB
 	EOF
 	echo fileSetLabel label1 >sub/.gitattributes &&
 	git add .gitattributes sub/.gitattributes &&
 	git commit -m "add attributes"
 '
 
+test_expect_success 'setup .gitignore' '
+	cat <<-\EOF >.gitignore &&
+	actual
+	expect
+	pathspec_file
+	EOF
+	git add .gitignore &&
+	git commit -m "add gitignore"
+'
+
 test_expect_success 'check specific set attr' '
 	cat <<-\EOF >expect &&
 	fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileA
 	fileAB
 	fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
 test_expect_success 'check unspecified attr (2)' '
 	cat <<-\EOF >expect &&
 	HEAD:.gitattributes
+	HEAD:.gitignore
 	HEAD:fileA
 	HEAD:fileAB
 	HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
+	.gitignore
 	fileC
 	fileNoLabel
 	fileWrongLabel
@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
 	test_i18ngrep "Only one" actual
 '
 
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
 	# The main purpose of this test is to check that we actually fail
 	# when you attempt to use attr magic in commands that do not implement
-	# attr magic. This test does not advocate git-add to stay that way,
-	# though, but git-add is convenient as it has its own internal pathspec
-	# parsing.
-	test_must_fail git add ":(attr:labelB)" 2>actual &&
+	# attr magic. This test does not advocate check-ignore to stay that way.
+	# When you teach the command to grok the pathspec, you need to find
+	# another command to replace it for the test.
+	test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
 	test_i18ngrep "magic not supported" actual
 '
 
+test_expect_success 'check that attr magic works for git stash push' '
+	cat <<-\EOF >expect &&
+	A	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
+	git stash show --include-untracked --name-status >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --all' '
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached >actual &&
+	git restore -W -S . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add -u ":(exclude,attr:labelB)" &&
+	git diff --name-only --cached  >actual &&
+	git restore -S -W . && rm sub/new* &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+	cat <<-\EOF >expect &&
+	fileA
+	fileB
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	git add ":(exclude,attr:labelB)sub/*" &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+	cat <<-\EOF >expect &&
+	sub/fileA
+	EOF
+	>fileA &&
+	>fileB &&
+	>sub/fileA &&
+	>sub/fileB &&
+	cd sub &&
+	git add . ":(exclude,attr:labelB)" &&
+	cd .. &&
+	git diff --name-only --cached >actual &&
+	git restore -S -W . &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+	cat <<-\EOF >pathspec_file &&
+	:(exclude,attr:labelB)
+	EOF
+	cat <<-\EOF >expect &&
+	sub/newFileA-foo
+	EOF
+	>sub/newFileA-foo &&
+	>sub/newFileB-foo &&
+	git add --all --pathspec-from-file=pathspec_file &&
+	git diff --name-only --cached >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.43.0-rc0


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-04  5:11                         ` Junio C Hamano
@ 2023-11-04  6:28                           ` Eric Sunshine
  2023-11-04  7:14                             ` Junio C Hamano
  2023-11-09 23:27                           ` Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: Eric Sunshine @ 2023-11-04  6:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Joanna Wang, git

On Sat, Nov 4, 2023 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:
> > +                     if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
>
> Overly long, with a stylo.
>
> Here is what I ended up queuing.
>
> +               for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +                       if ((magic & pathspec_magic[i].bit) &&
> +                           (pathspec_magic[i].mnemonic)) {

Nit: The no-value-added parentheses around
`pathspec_magic[i].mnemonic` can also be dropped.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-04  6:28                           ` Eric Sunshine
@ 2023-11-04  7:14                             ` Junio C Hamano
  0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-11-04  7:14 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Joanna Wang, git

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Nov 4, 2023 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:
>> > +                     if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
>>
>> Overly long, with a stylo.
>>
>> Here is what I ended up queuing.
>>
>> +               for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>> +                       if ((magic & pathspec_magic[i].bit) &&
>> +                           (pathspec_magic[i].mnemonic)) {
>
> Nit: The no-value-added parentheses around
> `pathspec_magic[i].mnemonic` can also be dropped.

Yeah, thanks for spotting.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
  2023-11-04  5:11                         ` Junio C Hamano
  2023-11-04  6:28                           ` Eric Sunshine
@ 2023-11-09 23:27                           ` Junio C Hamano
  1 sibling, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2023-11-09 23:27 UTC (permalink / raw)
  To: Joanna Wang; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> Thanks.  The compiled result from this version looks quite good.  As
> you started a new round with typofix, let me start the final
> nitpicking.
> ...
> Here is what I ended up queuing.
>
> Thanks.

Any comments on the "counterproposal to fix nitpicks"?  I'd like to
make sure the original author is either OK with the update (in which
case I can just move what I have in my tree forward) or unhappy with
it and plan to send in an update on their own (in which case I can
wait for the next iteration), in a case like this.

Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-11-09 23:27 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-05 23:23 [PATCH 1/1] Allow attr magic for git-add, git-stash Joanna Wang
2023-10-06  2:42 ` Junio C Hamano
2023-10-06 17:05   ` Junio C Hamano
2023-10-07  0:28     ` [PATCH 1/1] add: Enable attr pathspec magic for git-add Joanna Wang
2023-10-07  0:50       ` Joanna Wang
2023-10-07 10:10       ` [PATCH " Junio C Hamano
2023-10-11 20:20         ` [PATCH 1/1] add: enable attr pathspec magic Joanna Wang
2023-11-02  1:55           ` [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash Joanna Wang
2023-11-02  5:00             ` Junio C Hamano
2023-11-02 17:53               ` Joanna Wang
2023-11-02 21:32                 ` Junio C Hamano
2023-11-02 23:45                   ` Junio C Hamano
2023-11-03 14:35                     ` Joanna Wang
2023-11-03 15:31                       ` Ruben Safir
2023-11-03 16:34                       ` Joanna Wang
2023-11-04  5:11                         ` Junio C Hamano
2023-11-04  6:28                           ` Eric Sunshine
2023-11-04  7:14                             ` Junio C Hamano
2023-11-09 23:27                           ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.