RFC patchtag: add --points-at list option

> @@ -105,16 +107,28 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>  				return 0;
>  		}
>  
> +		buf = read_sha1_file(sha1, &type, &size);
> +		if (!buf || !size)
> +			return 0;
> +
> +		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)) {

> @@ -143,16 +157,20 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>  }
>  
>  static int list_tags(const char **patterns, int lines,
> -			struct commit_list *with_commit)
> +			struct commit_list *with_commit,
> +			unsigned char *points_at)
>  {

> @@ -375,12 +393,28 @@ 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, const char *arg, int unset)
> +{
> +	unsigned char *sha1;
> +
> +	if (!arg)
> +		return -1;
> +	sha1 = xmalloc(20);
> +	if (get_sha1(arg, sha1)) {
> +		free(sha1);
> +		return error("malformed object name %s", arg);
> +	}
> +	*(unsigned char **)opt->value = sha1;
> +	return 0;
> +}

> @@ -417,6 +451,12 @@ 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", &points_at, "object",
> +			"print only annotated|signed tags of the object",
> +			PARSE_OPT_LASTARG_DEFAULT,
> +			parse_opt_points_at, (intptr_t)"HEAD",
> +		},

>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> @@ -105,16 +107,28 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>>  				return 0;
>>  		}
>>  
>> +		buf = read_sha1_file(sha1, &type, &size);
>> +		if (!buf || !size)
>> +			return 0;
>> +
>> +		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)) {
>
>Do we need these backslashes at the end of these lines?

>> @@ -143,16 +157,20 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>>  }
>>  
>>  static int list_tags(const char **patterns, int lines,
>> -			struct commit_list *with_commit)
>> +			struct commit_list *with_commit,
>> +			unsigned char *points_at)
>>  {
>
>It strikes me somewhat odd that you can give a list of commits to filter
>when using "--contains" (e.g. "--contains v1.7.9 --contains 1.7.8.4"), but
>you can only ask for a single object with "--points-at" from the UI point
>of view.
>
>> @@ -375,12 +393,28 @@ 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, const char *arg, int unset)
>> +{
>> +	unsigned char *sha1;
>> +
>> +	if (!arg)
>> +		return -1;
>> +	sha1 = xmalloc(20);
>> +	if (get_sha1(arg, sha1)) {
>> +		free(sha1);
>> +		return error("malformed object name %s", arg);
>> +	}
>> +	*(unsigned char **)opt->value = sha1;
>> +	return 0;
>> +}
>
>We are ignoring earlier --points-at argument without telling the user that
>we do not support more than one.
>
>Would it become too much unnecessary addition of new code if you supported
>multiple --points-at on the command line for the sake of consistency?

>> @@ -417,6 +451,12 @@ 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", &points_at, "object",
>> +			"print only annotated|signed tags of the object",
>> +			PARSE_OPT_LASTARG_DEFAULT,
>> +			parse_opt_points_at, (intptr_t)"HEAD",
>> +		},
>
>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>are chasing a bug and checked out an old version that originally had
>problem, "git tag --contains" that defaults to HEAD does have a value. It
>tells us what releases are potentially contaminated with the buggy commit.
>
>But does a similar use case support points-at that defaults to HEAD?

>>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>>are chasing a bug and checked out an old version that originally had
>>problem, "git tag --contains" that defaults to HEAD does have a value. It
>>tells us what releases are potentially contaminated with the buggy commit.
>>
>>But does a similar use case support points-at that defaults to HEAD?
>
> Yes, the usage, "--points-at <object>..." implies that there is no
> default. So, I suppose that NULL more appropriate than "HEAD".

>Tom Grennan <tmgrennan@gmail.com> writes:
>
>>>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>>>are chasing a bug and checked out an old version that originally had
>>>problem, "git tag --contains" that defaults to HEAD does have a value. It
>>>tells us what releases are potentially contaminated with the buggy commit.
>>>
>>>But does a similar use case support points-at that defaults to HEAD?
>>
>> Yes, the usage, "--points-at <object>..." implies that there is no
>> default. So, I suppose that NULL more appropriate than "HEAD".
>
>That's a circular logic.
>
>The usage could very well say "--points-at <object>" and forbid missing
><object>.  I think that would make a lot _more_ sense, because I did not
>think of offhand any good reason that --points-at should default to HEAD
>to support some common usage, and you also seem to be unable to.

> 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

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

>On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:
>
>> 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.

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

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

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

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

> > >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.
> 
> Thinking on it more, your behavior is at least as good as the old. And
> it only comes up in a broken repo, anyway, so trying to come up with
> some kind of useful outcome is pointless.

> BTW, writing that helped me notice two bugs in your patch:
> 
>   1. You read up to 47 bytes into the buffer without ever checking
>      whether size >= 47.
> 
>   2. You never check whether the object you read from read_sha1_file is
>      actually a tag.

> > BTW, writing that helped me notice two bugs in your patch:
> > 
> >   1. You read up to 47 bytes into the buffer without ever checking
> >      whether size >= 47.
> > 
> >   2. You never check whether the object you read from read_sha1_file is
> >      actually a tag.
> 
> Hmm, the "filter->lines" code for "git tag -n" makes a similar error. It
> should probably print nothing for objects that are not tags.

> > Hmm, the "filter->lines" code for "git tag -n" makes a similar error. It
> > should probably print nothing for objects that are not tags.
> 
> Ugh, this part of builtin/tag.c is riddled with small bugs. I'm
> preparing a series that will fix them, and hopefully it should make
> building your points-at patch on top much more pleasant.

> The first case is an indication of a broken or corrupt repo,
> and we should notify the user of the error.
>
> The second case is OK to silently ignore; however, the
> existing code leaked the buffer returned by read_sha1_file.
> ...  
>  	buf = read_sha1_file(sha1, &type, &size);
> -	if (!buf || !size)
> +	if (!buf)
> +		die_errno("unable to read object %s", sha1_to_hex(sha1));
> +	if (!size) {
> +		free(buf);
>  		return;
> +	}
>  
>  	/* skip header */
>  	sp = strstr(buf, "\n\n");

> Jeff King <peff@peff.net> writes:
> 
> > The first case is an indication of a broken or corrupt repo,
> > and we should notify the user of the error.
> >
> > The second case is OK to silently ignore; however, the
> > existing code leaked the buffer returned by read_sha1_file.
> > ...  
> >  	buf = read_sha1_file(sha1, &type, &size);
> > -	if (!buf || !size)
> > +	if (!buf)
> > +		die_errno("unable to read object %s", sha1_to_hex(sha1));
> > +	if (!size) {
> > +		free(buf);
> >  		return;
> > +	}
> >  
> >  	/* skip header */
> >  	sp = strstr(buf, "\n\n");
> 
> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.

> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.
>
> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> because the definition of "first N lines" is compatible between tag and
> commit for the purpose of the -n option.

> > For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> > make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> > because the definition of "first N lines" is compatible between tag and
> > commit for the purpose of the -n option.
> 
> Ahh, Ok, your 3/3 addresses this exact issue.
> 
> I do not object to silently return when the object is not OBJ_TAG (even
> though I slightly prefer showing the first N lines of commit log contents
> for OBJ_COMMIT lightweight tag), but I still think it should be warned
> just like a corruption when we see (type == OBJ_TAG && !size).

> Jeff King <peff@peff.net> writes:
> 
> > OK, that's easy enough to do. Should we show lightweight tags to commits
> > for backwards compatibility (and just drop the parse_signature junk in
> > that case)? The showing of blobs or trees is the really bad thing, I
> > think.
> 
> For now, dropping 3/3 and queuing this instead...
> 
> ---
> Subject: tag: do not show non-tag contents with "-n"

>> Subject: tag: do not show non-tag contents with "-n"
>
> Looks perfect. Thanks.
>
> -Peff

> Jeff King <peff@peff.net> writes:
> 
> > OK, that's easy enough to do. Should we show lightweight tags to commits
> > for backwards compatibility (and just drop the parse_signature junk in
> > that case)? The showing of blobs or trees is the really bad thing, I
> > think.
> 
> For now, dropping 3/3 and queuing this instead...
> 
> ---
> Subject: tag: do not show non-tag contents with "-n"

> @@ -432,6 +500,12 @@ 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", &points_at, "object",
> +			"print only annotated|signed tags of the object",
> +			PARSE_OPT_LASTARG_DEFAULT,
> +			parse_opt_points_at, (intptr_t)NULL,
> +		},

>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +struct points_at {
>> +	struct points_at *next;
>> +	unsigned char *sha1;
>> +};
>
>struct points_at {
>	struct points_at *next;
>        unsigned char sha1[20];
>};
>
>would save you from having to allocate and free always in pairs, no?

>> +	if (type != OBJ_TAG
>> +	    || (tag = lookup_tag(sha1), !tag)
>> +	    || parse_tag_buffer(tag, buf, size) < 0) {
>
>Even though I personally prefer to cascade a long expression like this, so
>that you see a parse tree when you tilt your head 90-degrees to the left,
>I think the prevalent style in Git codebase is
>
>	if (A-long-long-expression ||
>            B-long-long-expression ||
>            C-long-long-expression) {
>
>Also we try to avoid assignment in the conditional.

>> @@ -432,6 +500,12 @@ 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", &points_at, "object",
>> +			"print only annotated|signed tags of the object",
>> +			PARSE_OPT_LASTARG_DEFAULT,
>> +			parse_opt_points_at, (intptr_t)NULL,
>> +		},
>
>If you are going to reject NULL anyway, do you still need to mark this as
>lastarg-default?
>
>Looking for example in parse-options.h, I found this:
>
>        #define OPT_STRING_LIST(s, l, v, a, h) \
>                    { OPTION_CALLBACK, (s), (l), (v), (a), \
>                      (h), 0, &parse_opt_string_list }
>
>which is used by "git clone" to mark its -c option.
>
>Running "git clone -c" gives me
>
>	error: switch 'c' requires a value
>
>without any extra code in the caller of parse_options().

> +static void free_points_at (struct points_at *points_at)
> +{
> +	while (points_at) {
> +		struct points_at *next = points_at->next;
> +		free(points_at->sha1);
> +		free(points_at);
> +		points_at = next;
> +	}
> +}

> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
> +{
> +	struct points_at *new, **opt_value = (struct points_at **)opt->value;
> +	unsigned char *sha1;
> +
> +	if (!arg)
> +		return error(_("missing <object>"));
> +	new = xmalloc(sizeof(struct points_at));
> +	sha1 = xmalloc(20);
> +	if (get_sha1(arg, sha1)) {
> +		free(new);
> +		free(sha1);
> +		return error(_("malformed object name '%s'"), arg);
> +	}
> +	new->sha1 = sha1;
> +	new->next = *opt_value;
> +	*opt_value = new;
> +	return 0;
> +}

> +static struct points_at *match_points_at(struct points_at *points_at,
> +					 const unsigned char *sha1)
> +{
> +	char *buf;
> +	struct tag *tag;
> +	unsigned long size;
> +	enum object_type type;
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return NULL;
> +	if (type != OBJ_TAG
> +	    || (tag = lookup_tag(sha1), !tag)
> +	    || parse_tag_buffer(tag, buf, size) < 0) {
> +		free(buf);
> +		return NULL;
> +	}
> +	while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
> +		points_at = points_at->next;
> +	free(buf);
> +	return points_at;
> +}

> +		{
> +			OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> +			"print only annotated|signed tags of the object",
> +			PARSE_OPT_LASTARG_DEFAULT,
> +			parse_opt_points_at, (intptr_t)NULL,
> +		},

>On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:
>
>> +struct points_at {
>> +	struct points_at *next;
>> +	unsigned char *sha1;
>> +};
>
>Would using sha1_array save us from having to create our own data
>structure? As a bonus, it can do O(lg n) lookups, though I seriously
>doubt anyone will provide a large number of "--points-at".

>> +static void free_points_at (struct points_at *points_at)
>> +{
>> +	while (points_at) {
>> +		struct points_at *next = points_at->next;
>> +		free(points_at->sha1);
>> +		free(points_at);
>> +		points_at = next;
>> +	}
>> +}
>
>Then this could go away in favor of sha1_array_clear.
>
>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
>> +{
>> +	struct points_at *new, **opt_value = (struct points_at **)opt->value;
>> +	unsigned char *sha1;
>> +
>> +	if (!arg)
>> +		return error(_("missing <object>"));
>> +	new = xmalloc(sizeof(struct points_at));
>> +	sha1 = xmalloc(20);
>> +	if (get_sha1(arg, sha1)) {
>> +		free(new);
>> +		free(sha1);
>> +		return error(_("malformed object name '%s'"), arg);
>> +	}
>> +	new->sha1 = sha1;
>> +	new->next = *opt_value;
>> +	*opt_value = new;
>> +	return 0;
>> +}
>
>And this can drop all of the memory management bits, like:
>
>  unsigned char sha1[20];
>
>  if (!arg)
>          return error(_("missing <object>"));
>  if (get_sha1(arg, sha1))
>          return error(_("malformed object name '%s'"), arg);
>  sha1_array_append(opt->value, sha1);
>  return 0;
>
>Also, should you check "unset"? When we have options that build a list,
>usually doing "--no-foo" will clear the list. E.g., this:
>
>  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>
>should look only for "baz".

>> +static struct points_at *match_points_at(struct points_at *points_at,
>> +					 const unsigned char *sha1)
>> +{
>> +	char *buf;
>> +	struct tag *tag;
>> +	unsigned long size;
>> +	enum object_type type;
>> +
>> +	buf = read_sha1_file(sha1, &type, &size);
>> +	if (!buf)
>> +		return NULL;
>> +	if (type != OBJ_TAG
>> +	    || (tag = lookup_tag(sha1), !tag)
>> +	    || parse_tag_buffer(tag, buf, size) < 0) {
>> +		free(buf);
>> +		return NULL;
>> +	}
>> +	while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
>> +		points_at = points_at->next;
>> +	free(buf);
>> +	return points_at;
>> +}
>
>Sorry, I threw a lot of object lookup code at you last time, so I think
>my point may have been lost in the noise. But I think this is slightly
>nicer as:
>
>  static int tag_points_at(struct sha1_array *sa,
>                           const unsigned char *sha1)
>  {
>          struct object *obj = parse_object(sha1);
>          if (!obj)
>                  return 0; /* or probably we should even just die() */
>          if (obj->type != OBJ_TAG)
>                  return 0;
>          if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
>                  return 0;
>          return 1;
>  }
>
>I.e., using parse_object lets you avoid dealing with memory management
>yourself. And as a bonus, it will reuse the cached information if you
>happen to have already parsed that object (not likely in typical
>repositories, but a huge win in certain pathological cases, like repos
>storing shared objects and refs for a large number of forks).

>> +		{
>> +			OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> +			"print only annotated|signed tags of the object",
>> +			PARSE_OPT_LASTARG_DEFAULT,
>> +			parse_opt_points_at, (intptr_t)NULL,
>> +		},
>
>I think you can drop the LASTARG_DEFAULT here, as it is no longer
>optional, no?

> >Would using sha1_array save us from having to create our own data
> >structure? As a bonus, it can do O(lg n) lookups, though I seriously
> >doubt anyone will provide a large number of "--points-at".
> 
> Thanks, but I now realize that I also need to save the pointed at
> refname to detect lightweight tags that have matching sha's but
> different names.

> >Also, should you check "unset"? When we have options that build a list,
> >usually doing "--no-foo" will clear the list. E.g., this:
> >
> >  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
> >
> >should look only for "baz".
> 
> Ahh, so I just need to:
> 	if (unset) {
> 		if (*opt_value)
> 			free_points_at(*opt_value);
> 		*opt_value = NULL;
> 		return 0;
> 	}

> >> +		{
> >> +			OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> >> +			"print only annotated|signed tags of the object",
> >> +			PARSE_OPT_LASTARG_DEFAULT,
> >> +			parse_opt_points_at, (intptr_t)NULL,
> >> +		},
> >
> >I think you can drop the LASTARG_DEFAULT here, as it is no longer
> >optional, no?
> 
> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?

>On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:
>
>> >Would using sha1_array save us from having to create our own data
>> >structure? As a bonus, it can do O(lg n) lookups, though I seriously
>> >doubt anyone will provide a large number of "--points-at".
>> 
>> Thanks, but I now realize that I also need to save the pointed at
>> refname to detect lightweight tags that have matching sha's but
>> different names.
>
>I'm not sure I understand. Wouldn't you match lightweight tags by the
>sha1 they point at? Something like:

>  static int tag_points_at(struct sha1_array *sa,
>                           const unsigned char *sha1)
>  {
>          struct object *obj;
>
>          /* Lightweight tag of an interesting sha1? */
>          if (sha1_array_lookup(sa, sha1) >= 0)
>                  return 1;
>
>          /* Otherwise, maybe a tag object pointing to an interesting sha1 */
>          obj = parse_object(sha1);
>          if (!obj)
>                 return 0; /* or probably we should even just die() */
>          if (obj->type != OBJ_TAG)
>                 return 0;
>          if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
>                 return 0;
>          return 1;
> }
>
>> >Also, should you check "unset"? When we have options that build a list,
>> >usually doing "--no-foo" will clear the list. E.g., this:
>> >
>> >  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>> >
>> >should look only for "baz".
>> 
>> Ahh, so I just need to:
>> 	if (unset) {
>> 		if (*opt_value)
>> 			free_points_at(*opt_value);
>> 		*opt_value = NULL;
>> 		return 0;
>> 	}
>
>Yes, exactly.
>
>> >> +		{
>> >> +			OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> >> +			"print only annotated|signed tags of the object",
>> >> +			PARSE_OPT_LASTARG_DEFAULT,
>> >> +			parse_opt_points_at, (intptr_t)NULL,
>> >> +		},
>> >
>> >I think you can drop the LASTARG_DEFAULT here, as it is no longer
>> >optional, no?
>> 
>> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
>
>Right. Though without flags, you can probably just use the OPT_CALLBACK
>wrapper, like:
>
>  OPT_CALLBACK(0, "points-at", &points_at, "object",
>               "print only annotated|signed tags of the object",
>               parse_opt_points_at)
>
>Note that if you are going to handle lightweight tags, that description
>should probably be updated.
>
>-Peff

> >> Thanks, but I now realize that I also need to save the pointed at
> >> refname to detect lightweight tags that have matching sha's but
> >> different names.
> >
> >I'm not sure I understand. Wouldn't you match lightweight tags by the
> >sha1 they point at? Something like:
> 
> 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.

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

> Jeff King <peff@peff.net> writes:
> 
> >> 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 <peff@peff.net> writes:
>> 
>> >> 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

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

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

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

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

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

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

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

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

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

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

> >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()

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

> 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

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

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

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

> 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 <tmgrennan@gmail.com>

>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:
>>
>>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()