Re: [PATCH 6/6] builtin/tag: add --format argument for tag -v

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2016, 21:23 UTC
Message-ID: <xmqqeg4b39ye.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160922185317.349-7-santiago@nyu.edu>

santiago@nyu.edu writes:

Show 10 quoted lines

> @@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		die(_("--merged and --no-merged option are only allowed with -l"));
>  	if (cmdmode == 'd')
>  		return for_each_tag_name(argv, delete_tag);
> -	if (cmdmode == 'v')
> +	if (cmdmode == 'v') {
> +		if (fmt_pretty)
> +			verify_ref_format(fmt_pretty);
>  		return for_each_tag_name(argv, verify_tag);
> +	}

OK, you said something about for_each_ref() in an earlier commit, but what you meant was this one, which takes each_tag_name_fn.

The function for_each_tag_name(), the type each_tag_name_fn, and the function of that type verify_tag(), are ALL file-scope static in this single file, builtin/tag.c. It seems to me that it is not necessary to make the format string global at all.

Show 10 quoted lines

> @@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		die(_("--merged and --no-merged option are only allowed with -l"));
>  	if (cmdmode == 'd')
>  		return for_each_tag_name(argv, delete_tag);
> -	if (cmdmode == 'v')
> +	if (cmdmode == 'v') {
> +		if (fmt_pretty)
> +			verify_ref_format(fmt_pretty);
>  		return for_each_tag_name(argv, verify_tag);
> +	}

There are minor implementation and design issues I spotted, but overall I think the feature the series attempts to add may be a good thing to have.

Thanks.