Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> tag.c: Change gpg_verify_tag argument to sha1

s/Change/change/

> The gpg_verify_tag function resolves the ref for any existing object.
> However, git tag -v resolves to only tag-refs. We can provide support
> for sha1 by moving the refname resolution code out of gpg_verify_tag and
> allow for the object's sha1 as an argument.

This description leaves me fairly clueless about why this change is
being made since justification seems to be lacking. More about this
below...

> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>                                 const unsigned char *sha1)
>  {
> -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
> +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);
>  }

So, by this point, 'name' has already been resolved to 'sha1', thus
this change avoids a second resolution of 'name' inside
gpg_verify_tag(). Therefore, this is really an optimization, right?
Perhaps the intent of the patch would be clearer if the commit message
sold it as such. For instance, the commit message might start off:

    tag: avoid resolving tag name twice

and then go on to say that by hefting tag name resolution out of
gpg_verify_tag(), the extra resolution can be avoided.

>  static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>         if (verbose)
>                 flags |= GPG_VERIFY_VERBOSE;
>
> -       while (i < argc)
> -               if (gpg_verify_tag(argv[i++], flags))
> +       while (i < argc) {
> +               if (get_sha1(argv[i++], sha1))
> +                       return error("tag '%s' not found.", argv[i]);

Why does this 'return' after the first error, but the gpg_verify_tag()
call below merely sets a 'had_error' flag and continues? I would
expect this one to set the flag and continue, as well.

> +

Style: unnecessary blank line

> +               if (gpg_verify_tag(sha1, flags))
>                         had_error = 1;
> +       }
>         return had_error;
>  }
> diff --git a/tag.c b/tag.c
> @@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> -int gpg_verify_tag(const char *name, unsigned flags)
> +int gpg_verify_tag(const unsigned char *sha1, unsigned flags)
>  {
>         enum object_type type;
> -       unsigned char sha1[20];
>         char *buf;
>         unsigned long size;
>         int ret;
>
> -       if (get_sha1(name, sha1))
> -               return error("tag '%s' not found.", name);
> -
>         type = sha1_object_info(sha1, NULL);
>         if (type != OBJ_TAG)
> -               return error("%s: cannot verify a non-tag object of type %s.",
> -                               name, typename(type));
> +               return error("cannot verify a non-tag object of type %s.",
> +                               typename(type));

This error message becomes much less useful since it now only says
that there is a problem with *some* tag but doesn't give any
identifying information. How about including the sha1 in the error
message?

>
>         buf = read_sha1_file(sha1, &type, &size);
>         if (!buf)
> -               return error("%s: unable to read file.", name);
> +               return error("unable to read file.");

Ditto regarding making this more useful by including the sha1.

>         ret = run_gpg_verify(buf, size, flags);