On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:
> On Mon, Apr 4, 2016 at 6:22 PM, <santiago@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html