Tom Grennan <tmgrennan@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html