Re: [PATCH 1/1] add: Enable attr pathspec magic for git-add.

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2023, 10:10 UTC
Message-ID: <xmqqbkda4ubp.fsf@gitster.g>
In-Reply-To: <20231007002811.2337315-1-jojwang@google.com>

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.

Show 11 quoted lines

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

Show 21 quoted lines

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

Show 15 quoted lines

> @@ -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 ;-)