Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 2, 2023, 05:00 UTC
Message-ID: <xmqqfs1ooil2.fsf@gitster.g>
In-Reply-To: <20231102015554.34089-1-jojwang@google.com>

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.

Show 20 quoted lines

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