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

Jeff King [off-list ref] writes:

quoted

I think the following would show the pointed at tag too.
  $ git tag my-v1.7.9 v1.7.9
  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9
  v1.7.9

vs.

  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9

I found that I had to filter matching refnames.

Ah, so you are trying _not_ to show lightweight tags (I thought you
meant you also wanted to show them)? But I still don't see why the code
I posted before wouldn't work in that case. The "object" field of v1.7.9
is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
would not match.

I think he is trying to avoid saying "v1.7.9 points at itself", and wants
to know not just the value of $(rev-parse v1.7.9) but the refname.

On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

quoted

I think the following would show the pointed at tag too.
  $ git tag my-v1.7.9 v1.7.9
  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9
  v1.7.9

vs.

  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9

I found that I had to filter matching refnames.

Ah, so you are trying _not_ to show lightweight tags (I thought you
meant you also wanted to show them)? But I still don't see why the code
I posted before wouldn't work in that case. The "object" field of v1.7.9
is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
would not match.

I think he is trying to avoid saying "v1.7.9 points at itself", and wants
to know not just the value of $(rev-parse v1.7.9) but the refname.

Hmm. I read his example again, and now I'm even more confused.

If I give an object name to --points-at, should or should not a
lightweight tag pointing to that object be found?

If not, then I don't see how "git tag --points-at v1.7.9" would find
v1.7.9. Because we would use get_sha1 to parse "v1.7.9", returning the
sha1 of the tag object. And then when trying to match, we would look at
each tag object, find its "object" line, and compare that. In the case
of considering whether to show the v1.7.9 tag, we would be comparing the
sha1 of the commit that it points to to the actual tag sha1 itself, and
not match.

But in that case, nor would we match "my-v1.7.9" above, as it is a
lightweight tag that also points to v1.7.9's tag object.

If we _do_ want to match lightweight tags, then in the matching phase we
look for both the sha1 contained in the tag ref, as well as the sha1 of
the thing the tag points to (_if_ it is a tag object). In that case, we
would find both v1.7.9 and my-v1.7.9.

So I am not sure which is preferable. But I don't see how you could or
would want to distinguish the two tags above. They are functionally
identical, in that they are both refs pointing to the exact same tag
object. If the example had started with "git tag -s my-v1.7.9 v1.7.9"
then it would make more sense to me.

-Peff

On Tue, Feb 07, 2012 at 04:30:12PM -0500, Jeff King wrote:

On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

quoted

I think the following would show the pointed at tag too.
  $ git tag my-v1.7.9 v1.7.9
  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9
  v1.7.9

vs.

  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9

I found that I had to filter matching refnames.

Ah, so you are trying _not_ to show lightweight tags (I thought you
meant you also wanted to show them)? But I still don't see why the code
I posted before wouldn't work in that case. The "object" field of v1.7.9
is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
would not match.

I think he is trying to avoid saying "v1.7.9 points at itself", and wants
to know not just the value of $(rev-parse v1.7.9) but the refname.

Hmm. I read his example again, and now I'm even more confused.

If I give an object name to --points-at, should or should not a
lightweight tag pointing to that object be found?

If not, then I don't see how "git tag --points-at v1.7.9" would find
v1.7.9. Because we would use get_sha1 to parse "v1.7.9", returning the
sha1 of the tag object. And then when trying to match, we would look at
each tag object, find its "object" line, and compare that. In the case
of considering whether to show the v1.7.9 tag, we would be comparing the
sha1 of the commit that it points to to the actual tag sha1 itself, and
not match.

But in that case, nor would we match "my-v1.7.9" above, as it is a
lightweight tag that also points to v1.7.9's tag object.

If we _do_ want to match lightweight tags, then in the matching phase we
look for both the sha1 contained in the tag ref, as well as the sha1 of
the thing the tag points to (_if_ it is a tag object). In that case, we
would find both v1.7.9 and my-v1.7.9.

So I am not sure which is preferable. But I don't see how you could or
would want to distinguish the two tags above. They are functionally
identical, in that they are both refs pointing to the exact same tag
object. If the example had started with "git tag -s my-v1.7.9 v1.7.9"
then it would make more sense to me.

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

While addressing Junio's comments I realized that by first matching the
sha's and not refnames like the following will show LW tags too.
So, v3 will act 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-lw-v1.7.9
  my-a-v1.7.9
  my-s-v1.7.9

Note, w/o strcmp(pa->refname, refname), this shows the points-at too:

  $ 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-lw-v1.7.9
  my-a-v1.7.9
  my-s-v1.7.9
  v1.7.9

Which I don't think we'd want.

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;
	obj = parse_object(sha1);
	if (!obj || obj->type != OBJ_TAG)
		return 0;
	tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
	while (points_at && hashcmp(points_at->sha1, tagged_sha1))
		points_at = points_at->next;
	return points_at;
}

-- 
TomG

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

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:

quoted

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

quoted

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

Please see version 3 of the "points-at" feature.  In addition to addressing
the comments on v2, this now lists lightweight tags to the given object.

Tom Grennan (1):
  tag: add --points-at list option

 Documentation/git-tag.txt |    5 +++-
 builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 4 deletions(-)

-- 
1.7.8

This filters the list for tags of the given object.
Example,

   john$ git tag v1.0-john v1.0
   john$ git tag -l --points-at v1.0
   v1.0-john

Signed-off-by: Tom Grennan <redacted>
---
 Documentation/git-tag.txt |    5 +++-
 builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..124ed36 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt

@@ -12,7 +12,7 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [<pattern>...]
 'git tag' -v <tagname>...
 

@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
 --contains <commit>::
 	Only list tags which contain the specified commit.
 
+--points-at <object>::
+	Only list tags of the given object.
+
 -m <msg>::
 --message=<msg>::
 	Use the given tag message (instead of prompting).

diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..c5da622 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -16,11 +16,13 @@
 #include "revision.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sha1-array.h"
 
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
 	"git tag -d <tagname>...",
-	"git tag -l [-n[<num>]] [<pattern>...]",
+	"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+		"\n\t\t[<pattern>...]",
 	"git tag -v <tagname>...",
 	NULL
 };

@@ -31,6 +33,7 @@ struct tag_filter {
 	struct commit_list *with_commit;
 };
 
+static struct sha1_array points_at;
 static unsigned int colopts;
 
 static int match_pattern(const char **patterns, const char *ref)

@@ -44,6 +47,22 @@ static int match_pattern(const char **patterns, const char *ref)
 	return 0;
 }
 
+static const unsigned char *match_points_at(const unsigned char *sha1)
+{
+	int i;
+	const unsigned char *tagged_sha1 = (unsigned char*)"";
+	struct object *obj = parse_object(sha1);
+
+	if (obj && obj->type == OBJ_TAG)
+		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+	for (i = 0; i < points_at.nr; i++)
+		if (!hashcmp(points_at.sha1[i], sha1))
+			return sha1;
+		else if (!hashcmp(points_at.sha1[i], tagged_sha1))
+			return tagged_sha1;
+	return NULL;
+}
+
 static int in_commit_list(const struct commit_list *want, struct commit *c)
 {
 	for (; want; want = want->next)

@@ -141,6 +160,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 				return 0;
 		}
 
+		if (points_at.nr && !match_points_at(sha1))
+			return 0;
+
 		if (!filter->lines) {
 			printf("%s\n", refname);
 			return 0;

@@ -389,6 +411,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+			const char *arg, int unset)
+{
+	unsigned char sha1[20];
+
+	if (unset) {
+		sha1_array_clear(&points_at);
+		return 0;
+	}
+	if (!arg)
+		return error(_("switch 'points-at' requires an object"));
+	if (get_sha1(arg, sha1))
+		return error(_("malformed object name '%s'"), arg);
+	sha1_array_append(&points_at, sha1);
+	return 0;
+}
+
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;

@@ -432,6 +471,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_LASTARG_DEFAULT,
 			parse_opt_with_commit, (intptr_t)"HEAD",
 		},
+		{
+			OPTION_CALLBACK, 0, "points-at", NULL, "object",
+			"print only tags of the object", 0, parse_opt_points_at
+		},
 		OPT_END()
 	};
 

@@ -478,8 +521,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	if (lines != -1)
 		die(_("-n option is only allowed with -l."));
-	if (with_commit)
-		die(_("--contains option is only allowed with -l."));
+	if (with_commit || points_at.nr)
+		die(_("--contains and --points-at options "
+		      "are only allowed with -l."));
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)

-- 
1.7.8

On Tue, Feb 07, 2012 at 05:45:15PM -0800, Tom Grennan wrote:

quoted

    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

Sorry, I should have been more clear here (the word symmetric isn't
right; it _is_ symmetric). My understanding of the point of your
original feature was to mention things that talk about a tag (because
you wanted to know what signatures were made around it).

With tag objects this is easy, because they contain a pointer. But when
it comes to lightweight tags, you cannot tell in which direction the
"talking about" occurred[1]. That is, a lightweight tag of another tag
is just creating a new ref, which looks the same as the old ref. So
something like --points-at cannot say "X talks about Y", because it
might as well have been "Y talks about X".

So I think you are better off to mention both X and Y (or to mention
neither).

quoted

    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.

I'm OK with not mentioning lightweight tags. I just feel it should be
all-or-nothing. It was specifically the "given object" that I took issue
with, since in your examples v1.7.9 was treated differently from its
sha1.

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. Either I've convinced you, or I've made you so sick of the
discussion that you're agreeing. The system works. :)

-Peff

[1] Actually, a tag object embeds the name of the ref under which it was
originally created (so the refs/tags/v1.7.9 tag has a "tag v1.7.9"
header in it). So in some cases, you _can_ determine the "original" ref
of a lightweight tag versus other refs made about it later. I'm still
not sure --points-at is a good place to try to make that distinction,
though.

On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:

+static const unsigned char *match_points_at(const unsigned char *sha1)
+{
+	int i;
+	const unsigned char *tagged_sha1 = (unsigned char*)"";
+	struct object *obj = parse_object(sha1);
+
+	if (obj && obj->type == OBJ_TAG)
+		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;

This is not safe. A sha1 is not NUL-terminated, but is rather _always_
20 bytes. So when the object is not a tag, you do the hashcmp against
your single-byte string literal above, and we end up comparing whatever
garbage is in the data segment after the string literal.

What you want instead is the all-zeros sha1, like:

  const unsigned char null_sha1[20] = { 0 };

Though we provide a null_sha1 global already. So doing:

  const unsigned char *tagged_sha1 = null_sha1;

would be sufficient.

That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).

Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.

+	for (i = 0; i < points_at.nr; i++)
+		if (!hashcmp(points_at.sha1[i], sha1))
+			return sha1;
+		else if (!hashcmp(points_at.sha1[i], tagged_sha1))
+			return tagged_sha1;
+	return NULL;

Why write your own linear search? sha1_array_lookup will do a binary
search for you.

Other than that, the patch looks OK to me.

-Peff

On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:

On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:

quoted

+static const unsigned char *match_points_at(const unsigned char *sha1)
+{
+	int i;
+	const unsigned char *tagged_sha1 = (unsigned char*)"";
+	struct object *obj = parse_object(sha1);
+
+	if (obj && obj->type == OBJ_TAG)
+		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;

This is not safe. A sha1 is not NUL-terminated, but is rather _always_
20 bytes. So when the object is not a tag, you do the hashcmp against
your single-byte string literal above, and we end up comparing whatever
garbage is in the data segment after the string literal.

Yikes! That was dumb.

What you want instead is the all-zeros sha1, like:

 const unsigned char null_sha1[20] = { 0 };

Though we provide a null_sha1 global already. So doing:

 const unsigned char *tagged_sha1 = null_sha1;

would be sufficient.

Or just initialize at test tagged_sha1 with NULL.

static const unsigned char *match_points_at(const unsigned char *sha1)
{
	int i;
	const unsigned char *tagged_sha1 = NULL;
	struct object *obj = parse_object(sha1);

	if (obj && obj->type == OBJ_TAG)
		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
	for (i = 0; i < points_at.nr; i++)
		if (!hashcmp(points_at.sha1[i], sha1))
			return sha1;
		else if (tagged_sha1 &&
			 !hashcmp(points_at.sha1[i], tagged_sha1))
			return tagged_sha1;
	return NULL;
}

That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).

Yes, I think your saying that the lightweight search could go before the
tag object search like this.

static const unsigned char *match_points_at(const unsigned char *sha1)
{
	const unsigned char *tagged_sha1 = NULL;
	struct object *obj = parse_object(sha1);

	if (sha1_array_lookup(&points_at, sha1) >= 0)
		return sha1;
	if (obj && obj->type == OBJ_TAG)
		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
	if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
		return tagged_sha1;
	return NULL;
}

Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.

I think the test of (obj) is redundant as this should be caught
by get_sha1() in parse_opt_points_at()

int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
			const char *arg, int unset)
{
	unsigned char sha1[20];

	if (unset) {
		sha1_array_clear(&points_at);
		return 0;
	}
	if (!arg)
		return error(_("switch 'points-at' requires an object"));
	if (get_sha1(arg, sha1))
		return error(_("malformed object name '%s'"), arg);
	sha1_array_append(&points_at, sha1);
	return 0;
}

quoted

+	for (i = 0; i < points_at.nr; i++)
+		if (!hashcmp(points_at.sha1[i], sha1))
+			return sha1;
+		else if (!hashcmp(points_at.sha1[i], tagged_sha1))
+			return tagged_sha1;
+	return NULL;

Why write your own linear search? sha1_array_lookup will do a binary
search for you.

Well, it's only a linear search of the points_at command arguments.
But by that reasoning, might as well do two sha1_array_lookups like
above and save some code b/c "less code is always better"(TM).

Other than that, the patch looks OK to me.

Thanks, I'll send what I hope to be the final version later today.

-- 
TomG

On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:

quoted

Though we provide a null_sha1 global already. So doing:

 const unsigned char *tagged_sha1 = null_sha1;

would be sufficient.

Or just initialize at test tagged_sha1 with NULL.

Oh yeah, that is even better.

quoted

That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).

Yes, I think your saying that the lightweight search could go before the
tag object search like this.

Exactly, though:

static const unsigned char *match_points_at(const unsigned char *sha1)
{
	const unsigned char *tagged_sha1 = NULL;
	struct object *obj = parse_object(sha1);

	if (sha1_array_lookup(&points_at, sha1) >= 0)
		return sha1;
	if (obj && obj->type == OBJ_TAG)
		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;

You can delay the relatively expensive parse_object until you find the
results of the first lookup (though like I said earlier, it is unlikely to
matter, as it only helps in the positive-match case. Out of N tags, you
will likely end up parsing N-1 of them anyway).

quoted

Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.

I think the test of (obj) is redundant as this should be caught
by get_sha1() in parse_opt_points_at()

No, it's not redundant. get_sha1 is purely about looking up the name and
finding a sha1. parse_object is about looking up the object represented
by that sha1 in the object db. get_sha1 can sometimes involve parsing
objects (e.g., looking for "foo^1" will need to parse the commit object
at "foo"),  but does not have to.

Besides which, you are not calling parse_object on the sha1 from
--points-at, but rather the sha1 for each tag ref given to us by
for_each_tag_ref.

quoted

Why write your own linear search? sha1_array_lookup will do a binary
search for you.

Well, it's only a linear search of the points_at command arguments.
But by that reasoning, might as well do two sha1_array_lookups like
above and save some code b/c "less code is always better"(TM).

Right. I expect the N to be small in this case, so I doubt it matters.
But two sha1_array_lookups is still asymptotically smaller, because the
expensive operation is hashcmp(). So two binary searches is O(2*lg n),
whereas a linear walk with 2 hashcmps per item is O(2*n).

quoted

Other than that, the patch looks OK to me.

Thanks, I'll send what I hope to be the final version later today.

Thanks for working on this and being so responsive to review.

-Peff

On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:

On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:

quoted

On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:

Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.

I think the test of (obj) is redundant as this should be caught
by get_sha1() in parse_opt_points_at()

I'm wrong. That tests the sha of the point-at argument, not the
sha/objects of the refs/tags entry.  I'll add...

	if (!obj)
		die(_("invalid tag, 'refs/tags/%s'"), refname);

-- 
TomG

Please see the following patch that I hoped is the last version of the
"points-at" feature.  Thank you for your patience.

Tom Grennan (1):
  tag: add --points-at list option

 Documentation/git-tag.txt |    5 +++-
 builtin/tag.c             |   52 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 4 deletions(-)

-- 
1.7.8

This filters the list for tags of the given object.
Example,

   john$ git tag v1.0-john v1.0
   john$ git tag -l --points-at v1.0
   v1.0-john

Signed-off-by: Tom Grennan <redacted>
---
 Documentation/git-tag.txt |    5 +++-
 builtin/tag.c             |   52 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..124ed36 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt

@@ -12,7 +12,7 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [<pattern>...]
 'git tag' -v <tagname>...
 

@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
 --contains <commit>::
 	Only list tags which contain the specified commit.
 
+--points-at <object>::
+	Only list tags of the given object.
+
 -m <msg>::
 --message=<msg>::
 	Use the given tag message (instead of prompting).

diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..f3051c7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -16,11 +16,13 @@
 #include "revision.h"
 #include "gpg-interface.h"
 #include "column.h"
+#include "sha1-array.h"
 
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
 	"git tag -d <tagname>...",
-	"git tag -l [-n[<num>]] [<pattern>...]",
+	"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+		"\n\t\t[<pattern>...]",
 	"git tag -v <tagname>...",
 	NULL
 };

@@ -31,6 +33,7 @@ struct tag_filter {
 	struct commit_list *with_commit;
 };
 
+static struct sha1_array points_at;
 static unsigned int colopts;
 
 static int match_pattern(const char **patterns, const char *ref)

@@ -44,6 +47,24 @@ static int match_pattern(const char **patterns, const char *ref)
 	return 0;
 }
 
+static const unsigned char *match_points_at(const char *refname,
+					    const unsigned char *sha1)
+{
+	const unsigned char *tagged_sha1 = NULL;
+	struct object *obj;
+
+	if (sha1_array_lookup(&points_at, sha1) >= 0)
+		return sha1;
+	obj = parse_object(sha1);
+	if (!obj)
+		die(_("malformed object at '%s'"), refname);
+	if (obj->type == OBJ_TAG)
+		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+	if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+		return tagged_sha1;
+	return NULL;
+}
+
 static int in_commit_list(const struct commit_list *want, struct commit *c)
 {
 	for (; want; want = want->next)

@@ -141,6 +162,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 				return 0;
 		}
 
+		if (points_at.nr && !match_points_at(refname, sha1))
+			return 0;
+
 		if (!filter->lines) {
 			printf("%s\n", refname);
 			return 0;

@@ -389,6 +413,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+			const char *arg, int unset)
+{
+	unsigned char sha1[20];
+
+	if (unset) {
+		sha1_array_clear(&points_at);
+		return 0;
+	}
+	if (!arg)
+		return error(_("switch 'points-at' requires an object"));
+	if (get_sha1(arg, sha1))
+		return error(_("malformed object name '%s'"), arg);
+	sha1_array_append(&points_at, sha1);
+	return 0;
+}
+
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;

@@ -432,6 +473,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_LASTARG_DEFAULT,
 			parse_opt_with_commit, (intptr_t)"HEAD",
 		},
+		{
+			OPTION_CALLBACK, 0, "points-at", NULL, "object",
+			"print only tags of the object", 0, parse_opt_points_at
+		},
 		OPT_END()
 	};
 

@@ -478,8 +523,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	if (lines != -1)
 		die(_("-n option is only allowed with -l."));
-	if (with_commit)
-		die(_("--contains option is only allowed with -l."));
+	if (with_commit || points_at.nr)
+		die(_("--contains and --points-at options "
+		      "are only allowed with -l."));
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)

-- 
1.7.8

On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:

This filters the list for tags of the given object.
Example,

   john$ git tag v1.0-john v1.0
   john$ git tag -l --points-at v1.0
   v1.0-john

And probably "v1.0", as well, in this iteration. :)

The patch content itself looks good to me, except:

What's this "column" stuff doing here? The nd/columns topic is still in
"next", isn't it? Did you base this on "next" or "pu"?

Usually topics should be based on master, so they can graduate
independently of each other. In this case, it might make sense to build
on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
even necessary here (my fixes ended up not being too closely related, I
think).

Other than that, I think the patch is fine. There are no tests, so
perhaps these should be squashed in:

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -v -s
 '
 
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+	test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+	echo v4.0 >expect &&
+	git tag --points-at v4.0 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+	git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+	echo annotated-v4.0 >expect &&
+	git tag -l --points-at v4.0 "annotated*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+	git tag -m "describing the v4.0 tag object" \
+		annotated-again-v4.0 annotated-v4.0 &&
+	cat >expect <<-\EOF &&
+	annotated-again-v4.0
+	annotated-v4.0
+	EOF
+	git tag --points-at=annotated-v4.0 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+	cat >expect <<-\EOF &&
+	v2.0
+	v3.0
+	EOF
+	git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.7.9.rc2.14.g3da2b

On Wed, Feb 08, 2012 at 03:58:57PM -0500, Jeff King wrote:

On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:

quoted

This filters the list for tags of the given object.
Example,

   john$ git tag v1.0-john v1.0
   john$ git tag -l --points-at v1.0
   v1.0-john

And probably "v1.0", as well, in this iteration. :)

Yep.

The patch content itself looks good to me, except:

quoted

--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt

@@ -12,7 +12,7 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [<pattern>...]

What's this "column" stuff doing here? The nd/columns topic is still in
"next", isn't it? Did you base this on "next" or "pu"?

Usually topics should be based on master, so they can graduate
independently of each other. In this case, it might make sense to build
on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
even necessary here (my fixes ended up not being too closely related, I
think).

Yes, it's no longer related to jk/maint-tag-show-fixes.
I've prepared a rebase patch to master and will add these tests.

Thanks,
TomG

The following applies the "points-at" feature to master and includes unit tests
from Jeff King [off-list ref]

Tom Grennan (1):
  tag: add --points-at list option

 Documentation/git-tag.txt |    6 ++++-
 builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh            |   39 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 2 deletions(-)

-- 
1.7.8

This filters the list for tags of the given object.
Example,

   john$ git tag v1.0-john v1.0
   john$ git tag -l --points-at v1.0
   v1.0-john
   v1.0

Signed-off-by: Tom Grennan <redacted>
---
 Documentation/git-tag.txt |    6 ++++-
 builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh            |   39 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 53ff5f6..8d32b9a 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt

@@ -12,7 +12,8 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>...]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
+	[<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION

@@ -86,6 +87,9 @@ OPTIONS
 --contains <commit>::
 	Only list tags which contain the specified commit.
 
+--points-at <object>::
+	Only list tags of the given object.
+
 -m <msg>::
 --message=<msg>::
 	Use the given tag message (instead of prompting).

diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..27c3557 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -15,11 +15,13 @@
 #include "diff.h"
 #include "revision.h"
 #include "gpg-interface.h"
+#include "sha1-array.h"
 
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
 	"git tag -d <tagname>...",
-	"git tag -l [-n[<num>]] [<pattern>...]",
+	"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] "
+		"\n\t\t[<pattern>...]",
 	"git tag -v <tagname>...",
 	NULL
 };

@@ -30,6 +32,8 @@ struct tag_filter {
 	struct commit_list *with_commit;
 };
 
+static struct sha1_array points_at;
+
 static int match_pattern(const char **patterns, const char *ref)
 {
 	/* no pattern means match everything */

@@ -41,6 +45,24 @@ static int match_pattern(const char **patterns, const char *ref)
 	return 0;
 }
 
+static const unsigned char *match_points_at(const char *refname,
+					    const unsigned char *sha1)
+{
+	const unsigned char *tagged_sha1 = NULL;
+	struct object *obj;
+
+	if (sha1_array_lookup(&points_at, sha1) >= 0)
+		return sha1;
+	obj = parse_object(sha1);
+	if (!obj)
+		die(_("malformed object at '%s'"), refname);
+	if (obj->type == OBJ_TAG)
+		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+	if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+		return tagged_sha1;
+	return NULL;
+}
+
 static int in_commit_list(const struct commit_list *want, struct commit *c)
 {
 	for (; want; want = want->next)

@@ -105,6 +127,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 				return 0;
 		}
 
+		if (points_at.nr && !match_points_at(refname, sha1))
+			return 0;
+
 		if (!filter->lines) {
 			printf("%s\n", refname);
 			return 0;

@@ -375,6 +400,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+			const char *arg, int unset)
+{
+	unsigned char sha1[20];
+
+	if (unset) {
+		sha1_array_clear(&points_at);
+		return 0;
+	}
+	if (!arg)
+		return error(_("switch 'points-at' requires an object"));
+	if (get_sha1(arg, sha1))
+		return error(_("malformed object name '%s'"), arg);
+	sha1_array_append(&points_at, sha1);
+	return 0;
+}
+
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;

@@ -417,6 +459,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_LASTARG_DEFAULT,
 			parse_opt_with_commit, (intptr_t)"HEAD",
 		},
+		{
+			OPTION_CALLBACK, 0, "points-at", NULL, "object",
+			"print only tags of the object", 0, parse_opt_points_at
+		},
 		OPT_END()
 	};
 

@@ -448,6 +494,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("-n option is only allowed with -l."));
 	if (with_commit)
 		die(_("--contains option is only allowed with -l."));
+	if (points_at.nr)
+		die(_("--points-at option is only allowed with -l."));
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -v -s
 '
 
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+	test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+	echo v4.0 >expect &&
+	git tag --points-at v4.0 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+	git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+	echo annotated-v4.0 >expect &&
+	git tag -l --points-at v4.0 "annotated*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+	git tag -m "describing the v4.0 tag object" \
+		annotated-again-v4.0 annotated-v4.0 &&
+	cat >expect <<-\EOF &&
+	annotated-again-v4.0
+	annotated-v4.0
+	EOF
+	git tag --points-at=annotated-v4.0 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+	cat >expect <<-\EOF &&
+	v2.0
+	v3.0
+	EOF
+	git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.7.8

On Wed, Feb 08, 2012 at 03:03:43PM -0800, Tom Grennan wrote:

This filters the list for tags of the given object.
Example,

   john$ git tag v1.0-john v1.0
   john$ git tag -l --points-at v1.0
   v1.0-john
   v1.0

Signed-off-by: Tom Grennan <redacted>

This version looks fine to me. Thanks.

-Peff

Jeff King [off-list ref] writes:

quoted

Signed-off-by: Tom Grennan <redacted>

This version looks fine to me. Thanks.

Thanks, both.  Will queue.