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

[Tom Grennan <tmgrennan@gmail.com>]

On Sun, Feb 05, 2012 at 03:31:17PM -0800, Junio C Hamano wrote:
>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?

No, just an old habit. Thanks.

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

OK, I'll implement multiple args to be consistent with contains and patterns.

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

Yes, the usage, "--points-at <object>..." implies that there is no
default. So, I suppose that NULL more appropriate than "HEAD".

Should I make the "contains" usage indicate that "commit" is optional
like this?
	"[--contains [<commit>...]] [--points-at <object>..]"

>Other than that, thanks for a pleasant read.

Thanks,
TomG