[PATCH v5 0/6] tag: move PGP verification code to tag.c

[PATCH v5 0/6] tag: move PGP verification code to tag.c

[santiago]

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

[PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface

[santiago]

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.

Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the
verify_signed_buffer call (pretty much like in sign_buffer()) so
that any caller is not required to perform this task. This will avoid
possible mistakes by further developers using verify_signed_buffer.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/verify-tag.c | 3 ---
 gpg-interface.c      | 2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 00663f6..77f070a 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -95,9 +95,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	/* sometimes the program was terminated because this signal
-	 * was received in the process of writing the gpg input: */
-	signal(SIGPIPE, SIG_IGN);
 	while (i < argc)
 		if (verify_tag(argv[i++], flags))
 			had_error = 1;
diff --git a/gpg-interface.c b/gpg-interface.c
index 3dc2fe3..2259938 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -237,6 +237,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		return error(_("could not run gpg."));
 	}
 
+	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(gpg.in, payload, payload_size);
 	close(gpg.in);
 
@@ -250,6 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	close(gpg.out);
 
 	ret = finish_command(&gpg);
+	sigchain_pop(SIGPIPE);
 
 	unlink_or_warn(path);
 
-- 
2.8.0

Re: [PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface

[Junio C Hamano]

santiago@nyu.edu writes:

> Subject: [PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface

s/Ignore/ignore/

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

s/comand/command/ but more importantly, it is not a command ;-)

> Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the
> verify_signed_buffer call (pretty much like in sign_buffer()) so
> that any caller is not required to perform this task. This will avoid
> possible mistakes by further developers using verify_signed_buffer.

I'd rephrase like this if I were doing this patch:

    verify-tag: ignore SIGPIPE in verify_signed_buffer()

    The verify_signed_buffer() function may trigger a SIGPIPE when
    the GPG child process terminates early (due to a bad keyid, for
    example) and we try to write to it afterwards.  Ignoring SIGPIPE
    is done in builtin/verify-tag.c to avoid dying from it, but any
    other caller who wants to call verify_signed_buffer() would have
    to do the same.

    Use sigchain_push(SIGPIPE, SIG_IGN) in verify_signed_buffer(),
    pretty much like in sign_buffer(), so that any caller is not
    required to perform this task.

    This will avoid possible mistakes by further developers using
    verify_signed_buffer().

The patch text (as I already said in the response to the cover
letter) looked fine.

[PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[santiago]

From: Santiago Torres <santiago@nyu.edu>

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

Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[Eric Sunshine]

On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:
> t7030-verify-tag: Adds validation for multiple tags

s/t7030-verify-tag/t7030/
s/Adds/add

> 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

Style: 'do' on its own line; drop semicolon

    for i in ...
    do
        ...
    done

> +               git verify-tag -v --raw $i || return 1
> +       done >expect.stdout 2>expect.stderr.1 &&
> +       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.)

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

Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[Santiago Torres]

Sorry for the delay! I just realized I had missed the second comment.

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

Thanks again for the reviews!
-Santiago.

Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[Eric Sunshine]

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.

Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[Santiago Torres]

On Sun, Apr 17, 2016 at 02:19:00PM -0400, Eric Sunshine wrote:
> 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.

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?

Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[Eric Sunshine]

On Sun, Apr 17, 2016 at 2:38 PM, Santiago Torres <santiago@nyu.edu> wrote:
> 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?

I just re-read the commit messages as Junio queued them on 'pu', and
they are all good, so yes it would be plenty sensible to copy the
commit messages from the three queued patches (along with whatever
other tweaks he made).

Since the patches are only in 'pu' (but not in 'next'), when you
re-roll, resubmit the entire series, not just the patches he didn't
queue.

Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Santiago Torres <santiago@nyu.edu>
>
> The verify-tag command supports multiple tag names as an argument.

That makes it sound as if this is a valid input

	$ git verify-tag "one two three four"

but that is not what you meant.

> However, existing tests only test for invocation with a single tag, so
> we add a test invoking with multiple tags.

I'd rephrase the above like so:

    t7030: test verifying multiple tags

    The verify-tag command accepts multiple tag names to verify, but
    existing tests only test for invocation with a single tag.

    Add a test invoking it with multiple tags.

I had the same (minor) problem with Eric on the patch text itself.

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

[PATCH v5 3/6] builtin/verify-tag: change variable name for readability

[santiago]

From: Santiago Torres <santiago@nyu.edu>

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>
---
 builtin/verify-tag.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 77f070a..1ca9a05 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	int len;
+	int payload_size;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	len = parse_signature(buf, size);
+	payload_size = parse_signature(buf, size);
 
-	if (size == len) {
+	if (size == payload_size) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, len);
+			write_in_full(1, buf, payload_size);
 		return error("no signature found");
 	}
 
-	ret = check_signature(buf, len, buf + len, size - len, &sigc);
+	ret = check_signature(buf, payload_size, buf + payload_size,
+				size - payload_size, &sigc);
 	print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
-- 
2.8.0

Re: [PATCH v5 3/6] builtin/verify-tag: change variable name for readability

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Santiago Torres <santiago@nyu.edu>
>
> The run_gpg_verify function has two variables size, and len. 

I somehow found this harder to parse than necessary.

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

I'd rephrase it like so:

    verify-tag: change variable name for readability

    The run_gpg_verify() function has two variables, size and len.
    This may come off as confusing when reading the code.

    Clarify which one pertains to the length of the tag headers by
    renaming len to payload_length.

The patch text looked good.

[PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1

[santiago]

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));
 
 	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);
 
 	ret = run_gpg_verify(buf, size, flags);
 
@@ -80,6 +78,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
 	unsigned flags = 0;
+	unsigned char sha1[20];
+	const char *name;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
 		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
@@ -96,8 +96,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	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;
+	}
 	return had_error;
 }
-- 
2.8.0

Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1

[Eric Sunshine]

On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:
> builtin/verify-tag: replace name argument with sha1

    builtin/verify-tag: prepare verify_tag() for libification

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

If I hadn't already understood the purpose of the patch, I think I'd
still be somewhat clueless after reading this because it doesn't do a
thorough job of explaining what the actual problem is that this is
solving. Perhaps something like this might be better:

    verify_tag() accepts a tag name which it resolves to a SHA1
    before verification, however, the plan is to make this
    functionality public and it is possible that future callers will
    already have a SHA1 in hand. Since it would be wasteful for them
    to supply a tag name only to have it resolved again, change
    verify_tag() to accept a tag SHA1 rather than a name.

> To avoid breaking
> builtin/verify-tag, we move the refname resolution outside of the
> verify_tag() call.

The reasonably intelligent reader should understand implicitly that
this is a natural consequence of changing the signature of
verify_tag(), thus it's not really necessary to state it explicitly.
(It makes the commit message noisier without adding value.)

More below...

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

Nit: sha1_to_hex() gets invoked *always*, even when there is no error.
An alternative would be to call it within each error() invocation,
when it's actually needed.

    return error("%s: unable to read file.", sha1_to_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;

An alternative without 'continue':

    if (get_sha1(...)) {
        error("tag ...");
        had_error = 1;
    } else if (verify_tag(...))
        had_error = 1;

I don't feel strongly about it, and it's certainly not worth a re-roll.

> +       }
>         return had_error;
>  }

Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1

[Junio C Hamano]

santiago@nyu.edu writes:

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

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.

 - use find_unique_abbrev() to shorten the fallback name displayed
   in the error message.

Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1

[Santiago Torres]

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

Yep, I agree. At least I believe that we can do better in terms of user
feedback.

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

Yep, I'll do some experimenting with this.

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

This seems to be fundamental. As it was suggested before, making it
clear that name is only for feedback purposes is really necessary. 

> 
>  - use find_unique_abbrev() to shorten the fallback name displayed in
>  the error message.

I think we can do both. Let me try this out. 

Would you suggest having these changes in a separate patch?


Thanks!
-Santiago.

[PATCH v5 5/6] builtin/verify-tag: move verification code to tag.c

[santiago]

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/verify-tag.c | 50 +-------------------------------------------------
 tag.c                | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tag.h                |  1 +
 3 files changed, 50 insertions(+), 49 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 7a7c376..e9a2005 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -18,54 +18,6 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
-static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
-{
-	struct signature_check sigc;
-	int payload_size;
-	int ret;
-
-	memset(&sigc, 0, sizeof(sigc));
-
-	payload_size = parse_signature(buf, size);
-
-	if (size == payload_size) {
-		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, payload_size);
-		return error("no signature found");
-	}
-
-	ret = check_signature(buf, payload_size, buf + payload_size,
-				size - payload_size, &sigc);
-	print_signature_buffer(&sigc, flags);
-
-	signature_check_clear(&sigc);
-	return ret;
-}
-
-static int verify_tag(const unsigned char *sha1, unsigned flags)
-{
-	enum object_type type;
-	char *buf;
-	char *hex_sha1;
-	unsigned long size;
-	int ret;
-
-	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.",
-				hex_sha1, typename(type));
-
-	buf = read_sha1_file(sha1, &type, &size);
-	if (!buf)
-		return error("%s: unable to read file.", hex_sha1);
-
-	ret = run_gpg_verify(buf, size, flags);
-
-	free(buf);
-	return ret;
-}
-
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
 	int status = git_gpg_config(var, value, cb);
@@ -103,7 +55,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 			had_error = 1;
 			continue;
 		}
-		if (verify_tag(sha1, flags))
+		if (gpg_verify_tag(sha1, flags))
 			had_error = 1;
 	}
 	return had_error;
diff --git a/tag.c b/tag.c
index d72f742..3f7669f 100644
--- a/tag.c
+++ b/tag.c
@@ -6,6 +6,54 @@
 
 const char *tag_type = "tag";
 
+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+{
+	struct signature_check sigc;
+	int payload_size;
+	int ret;
+
+	memset(&sigc, 0, sizeof(sigc));
+
+	payload_size = parse_signature(buf, size);
+
+	if (size == payload_size) {
+		if (flags & GPG_VERIFY_VERBOSE)
+			write_in_full(1, buf, payload_size);
+		return error("no signature found");
+	}
+
+	ret = check_signature(buf, payload_size, buf + payload_size,
+				size - payload_size, &sigc);
+	print_signature_buffer(&sigc, flags);
+
+	signature_check_clear(&sigc);
+	return ret;
+}
+
+int gpg_verify_tag(const unsigned char *sha1, unsigned flags)
+{
+	enum object_type type;
+	char *buf;
+	char *hex_sha1;
+	unsigned long size;
+	int ret;
+
+	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.",
+				hex_sha1, typename(type));
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.", hex_sha1);
+
+	ret = run_gpg_verify(buf, size, flags);
+
+	free(buf);
+	return ret;
+}
+
 struct object *deref_tag(struct object *o, const char *warn, int warnlen)
 {
 	while (o && o->type == OBJ_TAG)
diff --git a/tag.h b/tag.h
index f4580ae..cb643b9 100644
--- a/tag.h
+++ b/tag.h
@@ -17,5 +17,6 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern struct object *deref_tag_noverify(struct object *);
+extern int gpg_verify_tag(const unsigned char *sha1, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.8.0

Re: [PATCH v5 5/6] builtin/verify-tag: move verification code to tag.c

[Junio C Hamano]

santiago@nyu.edu writes:

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

After outlining the observation and opinion, we describe the changes
as if we are ordering somebody to "do this, do that" to the codebase,
like so:

	... require it.

        Move the function to tag.c and make it public, and rename it
	gpg_verify_tag() to make its name more descriptive and also
	avoid conflicts with a static function in builtin/mktag.c.

[PATCH v5 6/6] tag: use gpg_verify_function in tag -v call

[santiago]

From: Santiago Torres <santiago@nyu.edu>

Instead of running the verify-tag plumbing command, we use the
gpg_verify_tag() function within the verify_tag function to avoid doing
an additional fork call.

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

Re: [PATCH v5 6/6] tag: use gpg_verify_function in tag -v call

[Eric Sunshine]

On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:
> tag: use gpg_verify_function in tag -v call

This is a low-level description of what the patch is doing, but you
normally want the subject to present a high-level overview. Perhaps
something like:

    tag -v: verify directly rather than exec'ing git-verify-tag

> Instead of running the verify-tag plumbing command, we use the
> gpg_verify_tag() function within the verify_tag function to avoid doing
> an additional fork call.

I'm a bit confused about "gpg_verify_tag() function within the
verify_tag function". I *think* you can drop the "within the
verify_tag function" bit.

> 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

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Santiago Torres]

Sorry, forgot to add, this is a follow on to [1].

Thanks,
-Santiago.

[1]:
http://git.661346.n2.nabble.com/PATCH-v4-0-6-tag-move-PGP-verification-code-to-tag-c-td7652451.html
On Tue, Apr 05, 2016 at 12:07:23PM -0400, santiago@nyu.edu wrote:
> 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
> 

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Eric Sunshine]

On Tue, Apr 5, 2016 at 12:07 PM,  <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.

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Santiago Torres]

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

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Eric Sunshine]

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. (And, if you do make changes beyond ones I mentioned,
and if I review them and consider them issue-free, I can always
re-extend my Reviewed-by:.)

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Santiago Torres]

On Thu, Apr 07, 2016 at 12:19:37PM -0400, Eric Sunshine wrote:
> 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:.

Yeah, I didn't mean to imply that. I'm rewriting the commit messages and
testing out patches 3+/6, so I'm not going to assume there's any
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.

Speaking of which, would it make sense to add "helped-by" to the patches
in which I'm plagiarizing the commit messages?

> (And, if you do make changes beyond ones I mentioned, and if I review
> them and consider them issue-free, I can always re-extend my
> Reviewed-by:.)

Thanks!
-Santiago.

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Eric Sunshine]

On Sun, Apr 17, 2016 at 1:34 PM, Santiago Torres <santiago@nyu.edu> wrote:
> 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?

Yes, that would be quite sensible.

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Junio C Hamano]

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.

Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c

[Santiago Torres]

Thanks for the reviews!

I'll update the commit messages. It appears that I there's a lot to
learn on my side on the art of writing good commit messages.

-Santiago.

On Wed, Apr 06, 2016 at 09:39:39AM -0700, Junio C Hamano wrote:
> 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.