Re: [PATCHv2] tag: add --points-at list option

[Tom Grennan <tmgrennan@gmail.com>]

On Tue, Feb 07, 2012 at 07:25:54PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:
>
>> v1 and v2 wouldn't list lightweight tags of the points-at objects.
>> Both versions behave like this:
>>   $ git tag my-lw-v1.7.9 v1.7.9
>>   $ git tag my-a-v1.7.9 v1.7.9
>>   $ git tag my-s-v1.7.9 v1.7.9
>>   $ git tag -l --points-at v1.7.9
>>   my-a-v1.7.9
>>   my-s-v1.7.9
>
>I assume the 2nd and 3rd line should be:
>
>  $ git tag -a my-a-v1.7.9 v1.7.9
>  $ git tag -s my-s-v1.7.9 v1.7.9

Yes

>> static struct points_at *match_points_at(struct points_at *points_at,
>> 					 const char *refname,
>> 					 const unsigned char *sha1)
>> {
>> 	struct object *obj;
>> 	struct points_at *pa;
>> 	const unsigned char *tagged_sha1;
>> 
>> 	/* First look for lightweight tags - those with matching sha's
>> 	 * but different names */
>> 	for (pa = points_at; pa; pa = pa->next)
>> 		if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
>> 			return pa;
>
>OK, I see what you are trying to accomplish here. But I really don't
>like it. Two complaints:
>
>  1. Why is the name of the tag relevant? That is, if you are interested
>     in lightweight tags, and you have two tag refs, "refs/tags/a" and
>     "refs/tags/b", both pointing to the same tag object, then in what
>     situation is it useful to show "a" but not "b"?

Yes, I suppose this is more "tags or aliases of <object>" rather than
"tags that point at <object>".

>     It seems to me you would either want lightweight tags or not. And I
>     thought not, because the point of this was to reveal signatures or
>     annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we
>     want to show it?

Initially I didn't care about listing these lightweight tags (aliases)
but now I see that this could be useful to find turds in refs/tags.
  $ git tag my-v.1.7.9 v1.7.9
  ...
  $ git tag -l --points-at v1.7.9
  my-v.1.7.9
Oops

>     Also, it's not symmetric. What if I say "git tag
>     --points-at=my-lw-v1.7.9"? Then I would get your signed and
>     annotated tags (even though they're _not_ saying anything about
>     ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
>     anything about it either; in fact, it's the opposite!).

Huh?  As you noted, the lightweight tag is just an alternate reference,
so why wouldn't want to see the annotated and signed tags of that common
object?

  $ ./git-tag -l --points-at tomg-lw-v1.7.9 
  tomg-annotate-v1.7.9
  tomg-signed-v1.7.9
  v1.7.9
  $ ./git-tag -l --points-at v1.7.9 
  tomg-annotate-v1.7.9
  tomg-lw-v1.7.9
  tomg-signed-v1.7.9

>  2. I thought --points-at was about providing an object name. But it's
>     not. It's about providing a particular string. So with this code,
>     "git tag --points-at=v1.7.9" and "git tag --points-at=$(git
>     rev-parse v1.7.9)" are two different things. Which seems odd and
>     un-git-like to me.

Yep,
  $ ./git-tag -l --points-at $(git rev-parse v1.7.9)
  tomg-annotate-v1.7.9
  tomg-lw-v1.7.9
  tomg-signed-v1.7.9
  v1.7.9

>     Your documentation says "Only list annotated or signed tags of the
>     given object", which implies to me that --points-at is an arbitrary
>     object specifier, not a specific tagname.

Yes, I changed that in the patch that I've prepared but will revert this
if you'd rather not list these lightweight tags.

>It seems like your rationale is just avoiding a mention of v1.7.9
>because, hey, it was obviously on the command line and the user isn't
>interested in it.

Yes, exactly.

>But I don't think that's true. The user asked for every tag pointing to
>v1.7.9's object, and v1.7.9 is such a tag. It is no more or less true
>for v1.7.9 than it is for my-lw-v1.7.9.

My reaction when I tested this was, "don't tell me what I already know."
But consistency with $(git rev-parse ...) seems more important.
And as you noted, a sha1_array would save code and to me, less code is
always better.

Thanks,
TomG