Subject: Re: [PATCH 1/1] Allow attr magic for git-add, git-stash.

From: Junio C Hamano <[email-masked]>
Date: Thu, 05 Oct 2023 19:42:26 -0700

Joanna Wang <[email-masked]> 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.