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

[Tom Grennan <tmgrennan@gmail.com>]

On Tue, Feb 07, 2012 at 12:35:19AM -0800, Junio C Hamano wrote:
>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?

Yep

>> +static void free_points_at (struct points_at *points_at)
>
>Please lose the SP before (.

Oops

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

I like to compact multiple conditions to a common exit but also appreciate
the fear and loathing of comma's.

While rearranging this I finally understand how to include lightweight tags.

	struct points_at *pa;
	const unsigned char *tagged_sha1 = (const unsigned char *)"";

	/* 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;
	buf = read_sha1_file(sha1, &type, &size);
	if (buf) {
		if (type == OBJ_TAG) {
			tag = lookup_tag(sha1);
			if (parse_tag_buffer(tag, buf, size) >= 0)
				tagged_sha1 = tag->tagged->sha1;
		}
		free(buf);
	}
	while (points_at && hashcmp(points_at->sha1, tagged_sha1))
		points_at = points_at->next;
	return points_at;

For example,
$ ./git-tag tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -a tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-README HEAD:README
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README
tomg-README
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README \*v1.7.9
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9

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

Cool

>Other than that, looks cleanly done.
>
>Thanks. I'll take another look after I wake up in the morning.

Thanks, I'll send v3 later today.

-- 
TomG