Re: [RFC/PATCH] tag: add --points-at list option

[Jeff King <peff@peff.net>]

On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:

> This filters the list for annotated|signed tags of the given object.
> Example,
> 
>    john$ git tag -s v1.0-john v1.0
>    john$ git tag -l --points-at v1.0
>    v1.0-john

I really like this approach. One big question, and a few small comments:

> +--points-at <object>::
> +	Only list annotated or signed tags of the given object.
> +

It is unclear to me from this documentation if we will only peel a
single level, or if we will peel indefinitely. E.g., what will this
show:

  $ git tag one v1.0
  $ git tag two one
  $ git tag --points-at=v1.0

It will clearly show "one", but will it also show "two" (from reading
the code, I think the answer is "no")? If not, should it?

> +		buf = read_sha1_file(sha1, &type, &size);
> +		if (!buf || !size)
> +			return 0;

Before your patch, a tag whose sha1 could not be read would get its name
printed, and then we would later return without printing anything more.
Now it won't get even the first bit printed.

However, I'm not sure the old behavior wasn't buggy; it would print part
of the line, but never actually print the newline.

> +		if (filter->points_at) {
> +			unsigned char tagged_sha1[20];
> +			if (memcmp("object ", buf, 7) \
> +			    || buf[47] != '\n' \
> +			    || get_sha1_hex(buf + 7, tagged_sha1) \
> +			    || memcmp(filter->points_at, tagged_sha1, 20)) {
> +				free(buf);
> +				return 0;
> +			}
> +		}

Hmm, I would have expected to use parse_tag_buffer instead of doing it
by hand. This is probably a tiny bit more efficient, but I wonder if the
code complexity is worth it.

>  static int list_tags(const char **patterns, int lines,
> -			struct commit_list *with_commit)
> +			struct commit_list *with_commit,
> +			unsigned char *points_at)

Like Junio, I was surprised this did not allow a list.

-Peff