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

[Junio C Hamano <gitster@pobox.com>]

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?

> +static void free_points_at (struct points_at *points_at)

Please lose the SP before (.

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

Other than that, looks cleanly done.

Thanks. I'll take another look after I wake up in the morning.