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

On Tue, Feb 07, 2012 at 12:35:19AM -0800, Junio C Hamano wrote:

Tom Grennan [off-list ref] writes:

quoted

+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

quoted

+static void free_points_at (struct points_at *points_at)

Please lose the SP before (.

Oops

quoted

+	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

quoted

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