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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



santiago@xxxxxxx writes:

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

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]