Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

From: Jeff King <peff@peff.net>
Date: Apr 3, 2016, 04:56 UTC
Message-ID: <20160403045600.GD1519@sigill.intra.peff.net>
In-Reply-To: <1459638975-17705-5-git-send-email-santiago@nyu.edu>

On Sat, Apr 02, 2016 at 07:16:15PM -0400, santiago@nyu.edu wrote:

Show 10 quoted lines

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..3dffdff 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
>  }
>  
>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> -				const unsigned char *sha1);
> +				const unsigned char *sha1, unsigned flags);

I'm not sure it's a good idea to add a flags field here; most of the callbacks don't use it, and as you probably noticed, it makes the patch a lot noisier. It does let you directly use pgp_verify_tag like this:

>  	if (cmdmode == 'v')
> -		return for_each_tag_name(argv, verify_tag);
> +		return for_each_tag_name(argv, pgp_verify_tag,
> +				GPG_VERIFY_VERBOSE);

but I think that is coupling too closely. What happens later when the public, multi-file pgp_verify_tag function changes its interface? Or we want to change our interface here, and it no longer matches pgp_verify_tag? The results ripple a lot further than they should.

I think you probably want to keep a simple adapter callback in this file, like:

  int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
  {
	return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
  }

Show 30 quoted lines

> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f776778..8abc357 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  {
>  	int i = 1, verbose = 0, had_error = 0;
>  	unsigned flags = 0;
> +	unsigned char sha1[20];
> +	const char *name;
>  	const struct option verify_tag_options[] = {
>  		OPT__VERBOSE(&verbose, N_("print tag contents")),
>  		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
> @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  	if (verbose)
>  		flags |= GPG_VERIFY_VERBOSE;
>  
> -	while (i < argc)
> -		if (pgp_verify_tag(argv[i++], flags))
> +	while (i < argc) {
> +		name = argv[i++];
> +		if (get_sha1(name, sha1)) {
> +			error("tag '%s' not found.", name);
>  			had_error = 1;
> +		}
> +
> +		if (pgp_verify_tag(name, NULL, sha1, flags))
> +			had_error = 1;
> +
> +	}

So this is a good example of the rippling I mentioned earlier.

As a side note, it might actually be an improvement for pgp_verify_tag to take a sha1 (so that git-tag is sure that it is verifying the same object that it is printing), but that refactoring should probably come separately, I think.

-Peff