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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html