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:

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