git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Tom Grennan <tmgrennan@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, jasampler@gmail.com
Subject: Re: [PATCHv2] tag: add --points-at list option
Date: Tue, 7 Feb 2012 19:25:54 -0500	[thread overview]
Message-ID: <20120208002554.GA6035@sigill.intra.peff.net> (raw)
In-Reply-To: <20120207220806.GD6264@tgrennan-laptop>

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

> 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"?

     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?

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

  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.

     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.

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

-Peff

  reply	other threads:[~2012-02-08  0:26 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-05 22:28 [RFC/PATCH] tag: add --points-at list option Tom Grennan
2012-02-05 23:31 ` Junio C Hamano
2012-02-06  5:48   ` Tom Grennan
2012-02-06  6:25     ` Junio C Hamano
2012-02-06  6:45       ` Tom Grennan
2012-02-06  0:04 ` Jeff King
2012-02-06  6:32   ` Tom Grennan
2012-02-06  7:04     ` Jeff King
2012-02-06  7:13       ` Jeff King
2012-02-06  7:45         ` Jeff King
2012-02-06  8:11           ` Jeff King
2012-02-06  8:13             ` [PATCH 1/3] tag: fix output of "tag -n" when errors occur Jeff King
2012-02-06  8:13             ` [PATCH 2/3] tag: die when listing missing or corrupt objects Jeff King
2012-02-06  8:32               ` Junio C Hamano
2012-02-06  8:34                 ` Jeff King
2012-02-06  8:36                 ` Junio C Hamano
2012-02-06  8:38                   ` Jeff King
2012-02-06 18:04                     ` Junio C Hamano
2012-02-06 18:13                     ` Junio C Hamano
2012-02-06 20:12                       ` Jeff King
2012-02-06 23:34                         ` Junio C Hamano
2012-02-08 21:01                       ` Jeff King
2012-02-09  4:33                         ` Junio C Hamano
2012-02-06  8:14             ` [PATCH 3/3] tag: don't show non-tag contents with "-n" Jeff King
2012-02-07  7:01             ` [PATCHv2] tag: add --points-at list option Tom Grennan
2012-02-07  7:01             ` Tom Grennan
2012-02-07  8:35               ` Junio C Hamano
2012-02-07 18:05                 ` Tom Grennan
2012-02-07 16:05               ` Jeff King
2012-02-07 19:02                 ` Tom Grennan
2012-02-07 19:12                   ` Jeff King
2012-02-07 19:22                     ` Tom Grennan
2012-02-07 19:36                       ` Jeff King
2012-02-07 20:20                         ` Junio C Hamano
2012-02-07 21:30                           ` Jeff King
2012-02-07 22:08                             ` Tom Grennan
2012-02-08  0:25                               ` Jeff King [this message]
2012-02-08  1:45                                 ` Tom Grennan
2012-02-08 15:31                                   ` Jeff King
2012-02-08  6:21                                 ` [PATCHv3] " Tom Grennan
2012-02-08  6:21                                 ` Tom Grennan
2012-02-08 15:44                                   ` Jeff King
2012-02-08 18:43                                     ` Tom Grennan
2012-02-08 18:57                                       ` Jeff King
2012-02-08 20:12                                         ` [PATCHv4] " Tom Grennan
2012-02-08 20:12                                         ` Tom Grennan
2012-02-08 20:58                                           ` Jeff King
2012-02-08 22:15                                             ` Tom Grennan
2012-02-08 23:03                                             ` [PATCH-master] " Tom Grennan
2012-02-08 23:03                                             ` Tom Grennan
2012-02-09  1:44                                               ` Jeff King
2012-02-09  4:29                                                 ` Junio C Hamano
2012-02-08 18:58                                       ` [PATCHv3] " Tom Grennan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120208002554.GA6035@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jasampler@gmail.com \
    --cc=tmgrennan@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).