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

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:

quoted

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:

quoted

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

quoted

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

quoted

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

quoted

 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