v4, 6 partstag.c: Replace varialbe name for readability

> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
> index 4608e71..f9161332 100755
> --- a/t/t7030-verify-tag.sh
> +++ b/t/t7030-verify-tag.sh
> @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' '
> +test_expect_success GPG 'verify multiple tags' '
> +       tags="fourth-signed sixth-signed seventh-signed" &&
> +       for i in $tags; do
> +               git verify-tag -v --raw $i || return 1
> +       done >expect.stdout 2>expect.stderr.1 &&
> +       grep GOODSIG <expect.stderr.1 >expect.stderr &&
> +       git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
> +       grep GOODSIG <actual.stderr.1 >actual.stderr &&

> +       test_cmp expect.stdout actual.stdout &&
> +       test_cmp expect.stderr actual.stderr
> +'
> +
>  test_done
> --
> 2.8.0

> 
> The bulk of this description is merely repeating what the patch itself
> already says, thus is redundant. The entire commit message could
> probably be collapsed to:
> 
>     t7030: test verify-tag with multiple tags
> 
>     git-verify-tag accepts multiple tags as arguments, however,
>     existing tests only ever invoke it with a single tag, so add a
>     test invoking it with multiple tags.

> > +test_expect_success GPG 'verify multiple tags' '
> > +       tags="fourth-signed sixth-signed seventh-signed" &&
> > +       for i in $tags; do
> > +               git verify-tag -v --raw $i || return 1
> > +       done >expect.stdout 2>expect.stderr.1 &&
> > +       grep GOODSIG <expect.stderr.1 >expect.stderr &&
> > +       git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
> > +       grep GOODSIG <actual.stderr.1 >actual.stderr &&
> 
> Hmm, I had expected you to adopt Peff's suggestion[1] for the greps:
> 
>     grep '^.GNUPG:.' ...
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/290691

> Eric Sunshine wrote:
>> > +test_expect_success GPG 'verify multiple tags' '
>> > +       tags="fourth-signed sixth-signed seventh-signed" &&
>> > +       for i in $tags; do
>> > +               git verify-tag -v --raw $i || return 1
>> > +       done >expect.stdout 2>expect.stderr.1 &&
>> > +       grep GOODSIG <expect.stderr.1 >expect.stderr &&
>> > +       git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
>> > +       grep GOODSIG <actual.stderr.1 >actual.stderr &&
>>
>> Hmm, I had expected you to adopt Peff's suggestion[1] for the greps:
>>
>>     grep '^.GNUPG:.' ...
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290691
>
> I thought this was an stylistic thing. I can of course adopt this
> suggestion.

> The run_gpg_verify function has two variables size, and len. This may
> come off as confusing when reading the code. We clarify which one
> pertains to the length of the tag headers by renaming len to
> payload_length.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---

> Instead of running the verify-tag plumbing command, we use the
> pgp_verify_tag(). This avoids the usage of an extra fork call. To do
> this, we extend the number of parameters that tag.c takes, and
> verify-tag passes. Redundant calls done in the pgp_verify_tag function
> are removed.

> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..f4450f8 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -104,13 +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)
>  {
> -       const char *argv_verify_tag[] = {"verify-tag",
> -                                       "-v", "SHA1_HEX", NULL};
> -       argv_verify_tag[2] = sha1_to_hex(sha1);
> -
> -       if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
> -               return error(_("could not verify the tag '%s'"), name);
> -       return 0;
> +       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
>  }
>
>  static int do_sign(struct strbuf *buffer)
> --

> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> > Instead of running the verify-tag plumbing command, we use the
> > pgp_verify_tag(). This avoids the usage of an extra fork call. To do
> > this, we extend the number of parameters that tag.c takes, and
> > verify-tag passes. Redundant calls done in the pgp_verify_tag function
> > are removed.
> 
> I'm confused about everything following "an extra fork call" since
> those subsequent sentences don't seem to pertain to this patch. Is
> that leftover gunk from the previous version of this series?

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

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

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

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

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

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

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