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

[Junio C Hamano <gitster@pobox.com>]

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.