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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 5, 2016, 02:00 UTC
Message-ID: <CAPig+cQ4i86JpLFe0tqA-tjFj6Y3DvxDz3nFL8XAMVDBLwLUPg@mail.gmail.com>
In-Reply-To: <1459808535-8895-7-git-send-email-santiago@nyu.edu>

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

Show 10 quoted lines

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

Show 11 quoted lines

>  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

Show 25 quoted lines

> +               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?

Show 5 quoted lines

>
>         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);