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

[Tom Grennan <tmgrennan@gmail.com>]

On Sun, Feb 05, 2012 at 07:04:21PM -0500, Jeff King wrote:
>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?

Actually, neither one nor two would be listed as these are lightweight
tags.  In the modified example,

  $ git tag -a -m One one v1.0
  $ git tag -a -m Two two one
  $ git tag --points-at v1.0
  one
  $ git tag --points-at one
  two

one's object is v1.0 whereas two's object is one

>> +		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 you prefer, I can restore the old behavior just moving the
condition/return back below the refname print; then add "buf" qualifier
to the following fragment and at each intermediate free.

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

I didn't see how to get the object sha out of parse_tag_buffer() to
compare with "point_at". The inline conditions seem simple enough.

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

I agree and will change it.

Thanks,
TomG