v5, 6 partstag: move PGP verification code to tag.c

> From: Santiago Torres <santiago@nyu.edu>
>
> The verify_signed_buffer comand might cause a SIGPIPE signal when the
> gpg child process terminates early (due to a bad keyid, for example) and
> git tries to write to it afterwards. Previously, ignoring SIGPIPE was
> done on the builtin/verify-tag.c command to avoid this issue. However,
> any other caller who wanted to use the verify_signed_buffer command
> would have to include this signal call.

> The verify-tag command supports multiple tag names as an argument.
> However, existing tests only test for invocation with a single tag, so
> we add a test invoking with multiple tags.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git 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 $tags >actual.stdout 2>actual.stderr.1 &&
> +       grep "^.GNUPG" <actual.stderr.1 >actual.stderr &&
> +       test_cmp expect.stdout actual.stdout &&
> +       test_cmp expect.stderr actual.stderr
> +'
> +
>  test_done

> +       grep "^.GNUPG" <expect.stderr.1 >expect.stderr &&
> 
> Hmm, is there a reason you didn't stick with the more strict regex
> Peff suggested?
> 
>     ^.GNUPG:.
> 
> (Genuine question: I'm not saying your choice is wrong, I'm just
> interested in the reasoning behind the decision.)
> 

>> +       grep "^.GNUPG" <expect.stderr.1 >expect.stderr &&
>>
>> Hmm, is there a reason you didn't stick with the more strict regex
>> Peff suggested?
>>
>>     ^.GNUPG:.
>>
>> (Genuine question: I'm not saying your choice is wrong, I'm just
>> interested in the reasoning behind the decision.)
>
> I actually had missed the ":". I read the email and tried to internalize
> what the new test was actually doing, then I rewrote the test.
>
> I think I could add it for completeness though.

> On Sun, Apr 17, 2016 at 1:31 PM, Santiago Torres <santiago@nyu.edu> wrote:
> >> +       grep "^.GNUPG" <expect.stderr.1 >expect.stderr &&
> >>
> >> Hmm, is there a reason you didn't stick with the more strict regex
> >> Peff suggested?
> >>
> >>     ^.GNUPG:.
> >>
> >> (Genuine question: I'm not saying your choice is wrong, I'm just
> >> interested in the reasoning behind the decision.)
> >
> > I actually had missed the ":". I read the email and tried to internalize
> > what the new test was actually doing, then I rewrote the test.
> >
> > I think I could add it for completeness though.
> 
> Junio already made this correction and others in the three patches he
> queued on his 'pu' branch. It's possible that he also made other
> tweaks not mentioned in the reviews, so it's a good idea to compare
> what he queued against what you plan to send for the re-roll to ensure
> that nothing is missed. Thanks.

> On Sun, Apr 17, 2016 at 02:19:00PM -0400, Eric Sunshine wrote:
>> Junio already made this correction and others in the three patches he
>> queued on his 'pu' branch. It's possible that he also made other
>> tweaks not mentioned in the reviews, so it's a good idea to compare
>> what he queued against what you plan to send for the re-roll to ensure
>> that nothing is missed. Thanks.
>
> Oh, I'm looking at the patches in pu, I didn't know they were there yet.
> Thanks for the heads up.
>
> Also, would it make sense to copy the commit messages as they are on the pu
> branch? for consistency? Or should ommit those three patches and work
> on 4+ for the re-roll instead?

>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
>  t/t7030-verify-tag.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
> index 4608e71..c01621a 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 "^.GNUPG" <expect.stderr.1 >expect.stderr &&
> +	git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
> +	grep "^.GNUPG" <actual.stderr.1 >actual.stderr &&
> +	test_cmp expect.stdout actual.stdout &&
> +	test_cmp expect.stderr actual.stderr
> +'
> +
>  test_done

> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> -static int verify_tag(const char *name, unsigned flags)
> +static int verify_tag(const unsigned char *sha1, unsigned flags)
>  {
>         enum object_type type;
> -       unsigned char sha1[20];
>         char *buf;
> +       char *hex_sha1;
>         unsigned long size;
>         int ret;
>
> -       if (get_sha1(name, sha1))
> -               return error("tag '%s' not found.", name);
> -
> +       hex_sha1 = sha1_to_hex(sha1);
>         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));
> +                               hex_sha1, typename(type));
>
>         buf = read_sha1_file(sha1, &type, &size);
>         if (!buf)
> -               return error("%s: unable to read file.", name);
> +               return error("%s: unable to read file.", hex_sha1);

> @@ -96,8 +96,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> -       while (i < argc)
> -               if (verify_tag(argv[i++], flags))
> +       while (i < argc) {
> +               name = argv[i++];
> +               if (get_sha1(name, sha1)) {
> +                       error("tag '%s' not found.", name);
>                         had_error = 1;
> +                       continue;
> +               }
> +               if (verify_tag(sha1, flags))
> +                       had_error = 1;

> From: Santiago Torres <santiago@nyu.edu>
>
> This change is meant to prepare verify_tag for libification. Many
> existing modules/commands already do the refname to sha1 resolution, so
> should avoid resolving the refname twice. To avoid breaking
> builtin/verify-tag, we move the refname resolution outside of the
> verify_tag() call.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
>  builtin/verify-tag.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 1ca9a05..7a7c376 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
>  	return ret;
>  }
>  
> -static int verify_tag(const char *name, unsigned flags)
> +static int verify_tag(const unsigned char *sha1, unsigned flags)
>  {
>  	enum object_type type;
> -	unsigned char sha1[20];
>  	char *buf;
> +	char *hex_sha1;
>  	unsigned long size;
>  	int ret;
>  
> -	if (get_sha1(name, sha1))
> -		return error("tag '%s' not found.", name);
> -
> +	hex_sha1 = sha1_to_hex(sha1);
>  	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));
> +				hex_sha1, typename(type));

> >  	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));
> > +				hex_sha1, typename(type));
> 
> So, if I said
> 
>     $ git verify-tag master
> 
> the code used to take "master" from argv[], fed it to verify_tag()
> as parameter 'name', turned it to the object name of the commit,
> noticed that it is not a tag, and complained that "master: cannot verify".
> 
> With this rewrite, the same invocation would cause "master" to be
> turned into the object name, which is passed to verify_tag() and the
> complaint is an overlong
> 
>     76bece327f490cb344b95ae8f869cbeb89a4d20b: cannot verify a non-tag object of type commit
> 
> That does not sound like a good change at all.

> 
> If you want to support a future caller of a libified version of
> verify_tag() that has a raw object name but not the original name,
> I'd suggest to make this function keep parameter 'name' while adding
> the new parameter 'sha1'.  Then, the error reporting may become:
> 
> 	return error("%s: cannot verify a non-tag object of type '%s'",
> 		     name ? name : sha1_to_hex(sha1), typename(type));
> 

> and the output would still be useful.  Further improvements may be
> 
>  - rename 'name' to 'report_name' to clarify that the parameter is
>    only used for reporting, and that the tag object to verify is
>    always identified by the new 'sha1' parameter.

> From: Santiago Torres <santiago@nyu.edu>
>
> The PGP verification routine for tags could be accessed by other
> commands that require it. We do this by moving it to the common tag.c
> module. We rename the verify_tag() function to gpg_verify_tag() to avoid
> conflicts with the mktag.c function.

> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
>  builtin/tag.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..398c892 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(sha1, GPG_VERIFY_VERBOSE);
>  }
>
>  static int do_sign(struct strbuf *buffer)
> --
> 2.8.0

> From: Santiago Torres <santiago@nyu.edu>
> 
> v5 (this): 
> Added helpful feedback by Eric
> 
>  * Reordering of the patches, to avoid temporal inclusion of a regression
>  * Fix typos here and there.
>  * Review commit messages, as some weren't representative of what the patches
>    were doing anymore.
>  * Updated t7030 to include Peff's suggestion, and added a helped-by line here
>    as it was mostly Peff's code.
>  * Updated the error-handling/printing issues that were introduced when.
>    libifying the verify_tag function.
>    
> v4:
> 
> Thanks Eric, Jeff, and Hannes for the feedback.
> 
>  * I relocated the sigchain_push call so it comes after the error on
>    gpg-interface (thanks Hannnes for catching this).
>  * I updated the unit test to match the discussion on [3]. Now it generates
>    the expected output of the tag on the fly for comparison. (This is just
>    copy and paste from [3], but I verified that it works by breaking the
>    while)
>  * I split moving the code and renaming the variables into two patches so
>    these are easier to review.
>  * I used an adapter on builtin/tag.c instead of redefining all the fn*
>    declarations everywhere. This introduces an issue with the way git tag -v
>    resolves refnames though. I added a new commit to restore the previous
>    behavior of git-tag. I'm not sure if I should've split this into two commits
>    though.
> 
> v3:
> Thanks Eric, Jeff, for the feedback.
> 
>  * I separated the patch in multiple sub-patches.
>  * I compared the behavior of previous git tag -v and git verify-tag 
>    invocations to make sure the behavior is the same
>  * I dropped the multi-line comment, as suggested.
>  * I fixed the issue with the missing brackets in the while (this is 
>    now detected by the test).
> 
> v2:
> 
>  * I moved the pgp-verification code to tag.c 
>  * I added extra arguments so git tag -v and git verify-tag both work
>    with the same function
>  * Relocated the SIGPIPE handling code in verify-tag to gpg-interface
> 
> v1:
>  
> The verify tag function is just a thin wrapper around the verify-tag
> command. We can avoid one fork call by doing the verification inside
> the tag builtin instead.
> 
> 
> This applies on v2.8.0.
> 
> Thanks!
> -Santiago
> 
> 
> Santiago Torres (6):
>   builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface
>   t7030-verify-tag: Adds validation for multiple tags
>   builtin/verify-tag: change variable name for readability
>   builtin/verify-tag: replace name argument with sha1
>   builtin/verify-tag: move verification code to tag.c
>   tag: use gpg_verify_function in tag -v call
> 
>  builtin/tag.c         |  8 +------
>  builtin/verify-tag.c  | 65 +++++++++------------------------------------------
>  gpg-interface.c       |  2 ++
>  t/t7030-verify-tag.sh | 12 ++++++++++
>  tag.c                 | 48 +++++++++++++++++++++++++++++++++++++
>  tag.h                 |  1 +
>  6 files changed, 75 insertions(+), 61 deletions(-)
> 
> -- 
> 2.8.0
> 

> v5 (this):
> Added helpful feedback by Eric
>
>  * Reordering of the patches, to avoid temporal inclusion of a regression
>  * Fix typos here and there.
>  * Review commit messages, as some weren't representative of what the patches
>    were doing anymore.
>  * Updated t7030 to include Peff's suggestion, and added a helped-by line here
>    as it was mostly Peff's code.
>  * Updated the error-handling/printing issues that were introduced when.
>    libifying the verify_tag function.

> > v5 (this):
> > Added helpful feedback by Eric
> >
> >  * Reordering of the patches, to avoid temporal inclusion of a regression
> >  * Fix typos here and there.
> >  * Review commit messages, as some weren't representative of what the patches
> >    were doing anymore.
> >  * Updated t7030 to include Peff's suggestion, and added a helped-by line here
> >    as it was mostly Peff's code.
> >  * Updated the error-handling/printing issues that were introduced when.
> >    libifying the verify_tag function.
> 
> This version is a more pleasant read, easier to digest and understand.
> All of my review comments were minor; nothing demanding a re-roll. As
> such, this version is:
> 
>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> 
> If you do happen to re-roll based upon the review comments, feel free
> to add my Reviewed-by: (but not if you make larger changes).

>> > v5 (this):
>> > Added helpful feedback by Eric
>> >
>> >  * Reordering of the patches, to avoid temporal inclusion of a regression
>> >  * Fix typos here and there.
>> >  * Review commit messages, as some weren't representative of what the patches
>> >    were doing anymore.
>> >  * Updated t7030 to include Peff's suggestion, and added a helped-by line here
>> >    as it was mostly Peff's code.
>> >  * Updated the error-handling/printing issues that were introduced when.
>> >    libifying the verify_tag function.
>>
>> This version is a more pleasant read, easier to digest and understand.
>> All of my review comments were minor; nothing demanding a re-roll. As
>> such, this version is:
>>
>>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>>
>> If you do happen to re-roll based upon the review comments, feel free
>> to add my Reviewed-by: (but not if you make larger changes).
>
> Thanks! I'll add your and Junio's in another re-roll.

> On Wed, Apr 6, 2016 at 11:40 PM, Santiago Torres <santiago@nyu.edu> wrote:
> >> > v5 (this):
> >> > Added helpful feedback by Eric
> >> >
> >> >  * Reordering of the patches, to avoid temporal inclusion of a regression
> >> >  * Fix typos here and there.
> >> >  * Review commit messages, as some weren't representative of what the patches
> >> >    were doing anymore.
> >> >  * Updated t7030 to include Peff's suggestion, and added a helped-by line here
> >> >    as it was mostly Peff's code.
> >> >  * Updated the error-handling/printing issues that were introduced when.
> >> >    libifying the verify_tag function.
> >>
> >> This version is a more pleasant read, easier to digest and understand.
> >> All of my review comments were minor; nothing demanding a re-roll. As
> >> such, this version is:
> >>
> >>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> >>
> >> If you do happen to re-roll based upon the review comments, feel free
> >> to add my Reviewed-by: (but not if you make larger changes).
> >
> > Thanks! I'll add your and Junio's in another re-roll.
> 
> I don't think Junio explicitly gave his Reviewed-by: (indicating his
> approval of the patches as-is), so you wouldn't want to include his
> Reviewed-by:.

> 
> If you make any changes beyond the minor ones mentioned in my reviews
> or beyond plagiarizing commit message enhancements offered by my or
> Junio's reviews, then you'd also probably want to hold off adding my
> Reviewed-by: since I wouldn't yet have reviewed whatever new changes
> you're making.

> On Thu, Apr 07, 2016 at 12:19:37PM -0400, Eric Sunshine wrote:
>> If you make any changes beyond the minor ones mentioned in my reviews
>> or beyond plagiarizing commit message enhancements offered by my or
>> Junio's reviews, then you'd also probably want to hold off adding my
>> Reviewed-by: since I wouldn't yet have reviewed whatever new changes
>> you're making.
>
> Speaking of which, would it make sense to add "helped-by" to the patches
> in which I'm plagiarizing the commit messages?

> The first three looked almost good.  I do not fully agree with what
> 4/6 does, and update to its function signature is likely to require
> updates to 5/6 and 6/6.
> 
> Thanks.