Re: [RFC/PATCH] tag: add --points-at list option
- From
Jeff King <peff@peff.net>
- Date
- Feb 6, 2012, 00:04 UTC
- Message-ID
- <20120206000420.GC28735@sigill.intra.peff.net>
- In-Reply-To
- <1328480887-27463-1-git-send-email-tmgrennan@gmail.com>
On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:
Show 6 quoted lines
> 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.
Show 10 quoted lines
> + 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