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

[Santiago Torres <santiago@nyu.edu>]

On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:
> 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/

Sorry I've been consistently missing these... 
> 
> > 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.

Yep, this is actually true, but something I didn't consider. I think
that, from what I could draw on [1] and [2], git tag -v is reserved to
tags only (refs/tags iirc). This patch makes it so that this behavior is
not lost. I'm not sure if it should be separate from 5/6 though. 
> 
> >  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.

This sounds better than what I had thought. I'll set had error and do
continue instead.
> 
 >  {
> >         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.

yes, I wasn't sure about how to move forward here. I'll replace the name
with the sha1 instead of just removing it.

Thanks!
-Santiago.


[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c
[2]
http://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-tp7652334p7652437.html