[PATCH v3 0/4] tag: move PGP verification code to tag.c

[PATCH v3 0/4] tag: move PGP verification code to tag.c

[santiago]

This is a follow up of [1] and [2]:

v3 (this):
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

[1]
http://git.661346.n2.nabble.com/PATCH-RFC-builtin-tag-c-move-PGP-verification-inside-builtin-td7651529.html#a7651547
[2] http://git.661346.n2.nabble.com/PATCH-tag-c-move-PGP-verification-code-from-plumbing-td7651562.html 

[PATCH v3 1/4] 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/gpg-verify.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>
---
Notes:
 I dropped the multiline comment altogheter.

 builtin/verify-tag.c | 3 ---
 gpg-interface.c      | 3 +++
 2 files changed, 3 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..c1f6b2d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -232,6 +232,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	if (gpg_output)
 		gpg.err = -1;
 	args_gpg[3] = path;
+
+	sigchain_push(SIGPIPE, SIG_IGN);
 	if (start_command(&gpg)) {
 		unlink(path);
 		return error(_("could not run gpg."));
@@ -250,6 +252,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 v3 1/4] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface

[Jeff King]

On Sat, Apr 02, 2016 at 07:16:12PM -0400, santiago@nyu.edu wrote:

> 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/gpg-verify.c command to avoid this issue. However,

s/gpg-verify/verify-tag/ here, I think.

Other than that, nicely explained.

-Peff

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

[Johannes Sixt]

Am 03.04.2016 um 01:16 schrieb santiago@nyu.edu:
> 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/gpg-verify.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>
> ---
> Notes:
>   I dropped the multiline comment altogheter.
>
>   builtin/verify-tag.c | 3 ---
>   gpg-interface.c      | 3 +++
>   2 files changed, 3 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..c1f6b2d 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -232,6 +232,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>   	if (gpg_output)
>   		gpg.err = -1;
>   	args_gpg[3] = path;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
>   	if (start_command(&gpg)) {
>   		unlink(path);
>   		return error(_("could not run gpg."));

But no sigchain_pop() in the error path that we see here?

Perhaps you can even defer the sigchain_push() until after start_command()?

> @@ -250,6 +252,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);
>
>

-- Hannes

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

[Santiago Torres]

On Sun, Apr 03, 2016 at 08:50:27AM +0200, Johannes Sixt wrote:
> Am 03.04.2016 um 01:16 schrieb santiago@nyu.edu:
> > 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/gpg-verify.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>
> > ---
> > Notes:
> >   I dropped the multiline comment altogheter.
> > 
> >   builtin/verify-tag.c | 3 ---
> >   gpg-interface.c      | 3 +++
> >   2 files changed, 3 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..c1f6b2d 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -232,6 +232,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
> >   	if (gpg_output)
> >   		gpg.err = -1;
> >   	args_gpg[3] = path;
> > +
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> >   	if (start_command(&gpg)) {
> >   		unlink(path);
> >   		return error(_("could not run gpg."));
> 
> But no sigchain_pop() in the error path that we see here?
> 
> Perhaps you can even defer the sigchain_push() until after start_command()?

Yes, after reviewing where the sigchain_push() call is made on
sign_buffer(), I should put the push call after starting the command in
the same way sign_buffer() does. 

Thanks!
-Santiago.

[PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[santiago]

From: Santiago Torres <santiago@nyu.edu>

The verify-tag command supports mutliple tag names as an argument.
However, no previous tests try to verify multiple tags at once. This
test runs the verify-tag command against three trusted tags (created
previously), and ensures that:

	1) Three tags are verified appropriately (grep GOODSIG) and
	2) The three tags verified are indeed differently (uniq
		-u)

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 t/t7030-verify-tag.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 4608e71..5918f86 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -112,4 +112,12 @@ test_expect_success GPG 'verify signatures with --raw' '
 	)
 '
 
+test_expect_success GPG 'verify multiple tags' '
+	git verify-tag -v --raw fourth-signed sixth-signed seventh-signed 2>actual 1> tagnames &&
+		grep -c "GOODSIG" actual > count &&
+		! grep "BADSIG" actual &&
+		grep -E "tag\ .*" tagnames | uniq -u | wc - l | grep "3"
+'
+
+
 test_done
-- 
2.8.0

Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[Jeff King]

On Sat, Apr 02, 2016 at 07:16:13PM -0400, santiago@nyu.edu wrote:

> The verify-tag command supports mutliple tag names as an argument.

s/mutliple/multiple/

> +test_expect_success GPG 'verify multiple tags' '
> +	git verify-tag -v --raw fourth-signed sixth-signed seventh-signed 2>actual 1> tagnames &&

Style: we don't put a space between ">" and the filename. Also, we
usually omit "1" when redirecting stdout.

> +		grep -c "GOODSIG" actual > count &&

Funny indentation here.

I wondered if we could use test_cmp instead of a counting grep here, but
this is looking at gpg spew, and we probably don't want to count on that
never changing.

I don't see us actually verifying that "count" is 3, though.

> +		! grep "BADSIG" actual &&

Makes sense...

> +		grep -E "tag\ .*" tagnames | uniq -u | wc - l | grep "3"

Do we need "grep -E" here? I don't see any extended regex in use. Is
there a reason to backslash-escape the space?

Your "wc -l" has an extra space, which means "read stdin, and then the
file 'l'". Which sort-of happens to work, except as you noticed, you
have to grep for "3" instead of matching it.

I think, though, that rather than counting we could just write what we
expect into a file and compare that. It makes it easier for somebody
reading the test to see what it is we're trying to do.

In fact, I suspect you could replace the "GOODSIG" check as well by
doing something like:

  # verifying 3 tags in one invocation should be exactly like
  # verifying the 3 separately
  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 &&
  git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
  test_cmp expect.stdout actual.stdout &&
  test_cmp expect.stderr actual.stderr

but I didn't test it.

-Peff

Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[Eric Sunshine]

On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
> In fact, I suspect you could replace the "GOODSIG" check as well by
> doing something like:
>
>   # verifying 3 tags in one invocation should be exactly like
>   # verifying the 3 separately
>   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 &&
>   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
>   test_cmp expect.stdout actual.stdout &&
>   test_cmp expect.stderr actual.stderr

Hmm, does [1] suggest that using test_cmp on stderr here would be
contraindicated?

[1]: http://article.gmane.org/gmane.comp.version-control.git/289077

Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[Jeff King]

On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:

> On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
> > In fact, I suspect you could replace the "GOODSIG" check as well by
> > doing something like:
> >
> >   # verifying 3 tags in one invocation should be exactly like
> >   # verifying the 3 separately
> >   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 &&
> >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
> >   test_cmp expect.stdout actual.stdout &&
> >   test_cmp expect.stderr actual.stderr
> 
> Hmm, does [1] suggest that using test_cmp on stderr here would be
> contraindicated?
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289077

It does, but I am not sure I agree with the advice in that email in the
general case (I like making "-x" work, too, but not at the cost of
making the tests harder to read and write). In this case, I suppose you
could grep for gpg raw-output on stderr, though, and compare only that.

-Peff

Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[Santiago Torres]

On Sun, Apr 03, 2016 at 09:07:25AM -0400, Jeff King wrote:
> On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:
> 
> > On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
> > > In fact, I suspect you could replace the "GOODSIG" check as well by
> > > doing something like:
> > >
> > >   # verifying 3 tags in one invocation should be exactly like
> > >   # verifying the 3 separately
> > >   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 &&
> > >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
> > >   test_cmp expect.stdout actual.stdout &&
> > >   test_cmp expect.stderr actual.stderr
> > 
> > Hmm, does [1] suggest that using test_cmp on stderr here would be
> > contraindicated?
> > 
> > [1]: http://article.gmane.org/gmane.comp.version-control.git/289077
> 
> It does, but I am not sure I agree with the advice in that email in the
> general case (I like making "-x" work, too, but not at the cost of
> making the tests harder to read and write). In this case, I suppose you
> could grep for gpg raw-output on stderr, though, and compare only that.
> 
> -Peff

I just read [1], I'll take the later advice and use test_i18ngrep
instead.

Thanks!
-Santiago.

Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[Eric Sunshine]

On Sun, Apr 3, 2016 at 5:58 PM, Santiago Torres <santiago@nyu.edu> wrote:
> On Sun, Apr 03, 2016 at 09:07:25AM -0400, Jeff King wrote:
>> On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:
>> > On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
>> > > In fact, I suspect you could replace the "GOODSIG" check as well by
>> > > doing something like:
>> > >
>> > >   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 &&
>> > >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
>> > >   test_cmp expect.stdout actual.stdout &&
>> > >   test_cmp expect.stderr actual.stderr
>> >
>> > Hmm, does [1] suggest that using test_cmp on stderr here would be
>> > contraindicated?
>> >
>> > [1]: http://article.gmane.org/gmane.comp.version-control.git/289077
>>
>> It does, but I am not sure I agree with the advice in that email in the
>> general case (I like making "-x" work, too, but not at the cost of
>> making the tests harder to read and write). In this case, I suppose you
>> could grep for gpg raw-output on stderr, though, and compare only that.
>
> I just read [1], I'll take the later advice and use test_i18ngrep
> instead.

I think Peff meant that a simple grep would suffice; no need for
test_i18ngrep. In other words (reproducing Peff's example), something
like this:

    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

Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags

[Jeff King]

On Sun, Apr 03, 2016 at 09:38:34PM -0400, Eric Sunshine wrote:

> I think Peff meant that a simple grep would suffice; no need for
> test_i18ngrep. In other words (reproducing Peff's example), something
> like this:
> 
>     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

Yep, though I think I would actually have done:

   grep '^.GNUPG:.' ...

or something to just catch all of the gnupg output.

-Peff

[PATCH v3 3/4] 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
code. We rename the verify_tag() function to pgp_verify_tag() to avoid
conflicts with the mktag.c function.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/verify-tag.c | 51 +--------------------------------------------------
 tag.c                | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tag.h                |  1 +
 3 files changed, 52 insertions(+), 50 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 77f070a..f776778 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -18,55 +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 len;
-	int ret;
-
-	memset(&sigc, 0, sizeof(sigc));
-
-	len = parse_signature(buf, size);
-
-	if (size == len) {
-		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, len);
-		return error("no signature found");
-	}
-
-	ret = check_signature(buf, len, buf + len, size - len, &sigc);
-	print_signature_buffer(&sigc, flags);
-
-	signature_check_clear(&sigc);
-	return ret;
-}
-
-static int verify_tag(const char *name, 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));
-
-	buf = read_sha1_file(sha1, &type, &size);
-	if (!buf)
-		return error("%s: unable to read file.", name);
-
-	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);
@@ -96,7 +47,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		flags |= GPG_VERIFY_VERBOSE;
 
 	while (i < argc)
-		if (verify_tag(argv[i++], flags))
+		if (pgp_verify_tag(argv[i++], flags))
 			had_error = 1;
 	return had_error;
 }
diff --git a/tag.c b/tag.c
index d72f742..918ae39 100644
--- a/tag.c
+++ b/tag.c
@@ -6,6 +6,56 @@
 
 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) {
+		write_in_full(1, buf, payload_size);
+		return error("No PGP signature found in this tag!");
+	}
+
+	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 pgp_verify_tag(const char *name, unsigned flags)
+{
+
+	enum object_type type;
+	unsigned long size;
+	unsigned char sha1[20];
+	char* buf;
+	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));
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.", name);
+
+	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..09e71f9 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 pgp_verify_tag(const char *name, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.8.0

Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c

[Jeff King]

On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:

> 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
> code. We rename the verify_tag() function to pgp_verify_tag() to avoid
> conflicts with the mktag.c function.

One nit: even though GPG is just an implementation of PGP, and
technically the standard and formats are called PGP, we tend to name
everything GPG in the code. So this probably should be gpg_verify_tag().

> -	len = parse_signature(buf, size);
> -
> -	if (size == len) {
> -		if (flags & GPG_VERIFY_VERBOSE)
> -			write_in_full(1, buf, len);
> -		return error("no signature found");
> -	}
> [...]
> +	payload_size = parse_signature(buf, size);
> +
> +	if (size == payload_size) {
> +		write_in_full(1, buf, payload_size);
> +		return error("No PGP signature found in this tag!");
> +	}

I'm happy to see the more readable variable name here. I wonder if we
should leave the error message as-is, though, as this is just supposed
to be about code movement (and if we are changing it, it should adhere
to our usual style of not starting with a capital letter, and not ending
in punctuation).

-Peff

Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c

[Eric Sunshine]

On Sun, Apr 3, 2016 at 12:45 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:
>> -     len = parse_signature(buf, size);
>> -
>> -     if (size == len) {
>> -             if (flags & GPG_VERIFY_VERBOSE)
>> -                     write_in_full(1, buf, len);
>> -             return error("no signature found");
>> -     }
>> [...]
>> +     payload_size = parse_signature(buf, size);
>> +
>> +     if (size == payload_size) {
>> +             write_in_full(1, buf, payload_size);
>> +             return error("No PGP signature found in this tag!");
>> +     }
>
> I'm happy to see the more readable variable name here. I wonder if we
> should leave the error message as-is, though, as this is just supposed
> to be about code movement (and if we are changing it, it should adhere
> to our usual style of not starting with a capital letter, and not ending
> in punctuation).

Agreed it would be nice for this patch to be just code movement since
it's difficult for a reviewer to spot actual changes. A pure code
movement patch was suggested by [1], but perhaps it should also have
explained the reason ("code changes are difficult to spot in
movement").

Such changes could be done as preparatory or follow-on patches.
Alternately, since these are such minor changes, it might also be okay
just to mention them in the commit message (as the function rename is
already mentioned).

[1]: http://article.gmane.org/gmane.comp.version-control.git/289847

Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c

[Eric Sunshine]

On Sat, Apr 2, 2016 at 7:16 PM,  <santiago@nyu.edu> wrote:
> 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
> code. We rename the verify_tag() function to pgp_verify_tag() to avoid
> conflicts with the mktag.c function.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> -       len = parse_signature(buf, size);
> -
> -       if (size == len) {
> -               if (flags & GPG_VERIFY_VERBOSE)
> -                       write_in_full(1, buf, len);
> -               return error("no signature found");
> -       }
> [...]
> +       payload_size = parse_signature(buf, size);
> +
> +       if (size == payload_size) {
> +               write_in_full(1, buf, payload_size);
> +               return error("No PGP signature found in this tag!");
> +       }

Also, [1] asked why the moved code no longer respects
GPG_VERIFY_VERBOSE, and that question doesn't seem to be answered
either in the previous review thread or by this patch's commit
message. It's not clear at a casual glance why this change is
desirable.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289977

Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c

[Santiago Torres]

On Sun, Apr 03, 2016 at 04:19:26AM -0400, Eric Sunshine wrote:
> On Sat, Apr 2, 2016 at 7:16 PM,  <santiago@nyu.edu> wrote:
> > 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
> > code. We rename the verify_tag() function to pgp_verify_tag() to avoid
> > conflicts with the mktag.c function.
> >
> > Signed-off-by: Santiago Torres <santiago@nyu.edu>
> > ---
> > -       len = parse_signature(buf, size);
> > -
> > -       if (size == len) {
> > -               if (flags & GPG_VERIFY_VERBOSE)
> > -                       write_in_full(1, buf, len);
> > -               return error("no signature found");
> > -       }
> > [...]
> > +       payload_size = parse_signature(buf, size);
> > +
> > +       if (size == payload_size) {
> > +               write_in_full(1, buf, payload_size);
> > +               return error("No PGP signature found in this tag!");
> > +       }
> 
> Also, [1] asked why the moved code no longer respects
> GPG_VERIFY_VERBOSE, and that question doesn't seem to be answered
> either in the previous review thread or by this patch's commit
> message. It's not clear at a casual glance why this change is
> desirable.
> 

I must've missed this when moving code around. I don't think that this
if should change in any way. I'll put it back as it is (other than the
variable naming that is)

[PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[santiago]

From: 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>
---
 Notes:
 - In this version I fixed the issues with the brackets (the old patch
   doesn't work in this case due to the new test.

 builtin/tag.c        | 28 +++++++++-------------------
 builtin/verify-tag.c | 14 ++++++++++++--
 tag.c                |  7 ++-----
 tag.h                |  3 ++-
 4 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..3dffdff 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 }
 
 typedef int (*each_tag_name_fn)(const char *name, const char *ref,
-				const unsigned char *sha1);
+				const unsigned char *sha1, unsigned flags);
 
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+		unsigned flags)
 {
 	const char **p;
 	char ref[PATH_MAX];
@@ -86,33 +87,21 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 			had_error = 1;
 			continue;
 		}
-		if (fn(*p, ref, sha1))
+		if (fn(*p, ref, sha1, flags))
 			had_error = 1;
 	}
 	return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-				const unsigned char *sha1)
+				const unsigned char *sha1, unsigned flags)
 {
-	if (delete_ref(ref, sha1, 0))
+	if (delete_ref(ref, sha1, flags))
 		return 1;
 	printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));
 	return 0;
 }
 
-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;
-}
-
 static int do_sign(struct strbuf *buffer)
 {
 	return sign_buffer(buffer, buffer, get_signing_key());
@@ -424,9 +413,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (filter.merge_commit)
 		die(_("--merged and --no-merged option are only allowed with -l"));
 	if (cmdmode == 'd')
-		return for_each_tag_name(argv, delete_tag);
+		return for_each_tag_name(argv, delete_tag, 0);
 	if (cmdmode == 'v')
-		return for_each_tag_name(argv, verify_tag);
+		return for_each_tag_name(argv, pgp_verify_tag,
+				GPG_VERIFY_VERBOSE);
 
 	if (msg.given || msgfile) {
 		if (msg.given && msgfile)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f776778..8abc357 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -30,6 +30,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),
@@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	while (i < argc)
-		if (pgp_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;
+		}
+
+		if (pgp_verify_tag(name, NULL, sha1, flags))
+			had_error = 1;
+
+	}
 	return had_error;
 }
diff --git a/tag.c b/tag.c
index 918ae39..2a0b24c 100644
--- a/tag.c
+++ b/tag.c
@@ -29,18 +29,15 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 	return ret;
 }
 
-int pgp_verify_tag(const char *name, unsigned flags)
+int pgp_verify_tag(const char *name, const char *ref,
+		const unsigned char *sha1, unsigned flags)
 {
 
 	enum object_type type;
 	unsigned long size;
-	unsigned char sha1[20];
 	char* buf;
 	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.",
diff --git a/tag.h b/tag.h
index 09e71f9..22289a5 100644
--- a/tag.h
+++ b/tag.h
@@ -17,6 +17,7 @@ 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 pgp_verify_tag(const char *name, unsigned flags);
+extern int pgp_verify_tag(const char *name, const char *ref,
+		const unsigned char *sha1, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.8.0

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Jeff King]

On Sat, Apr 02, 2016 at 07:16:15PM -0400, santiago@nyu.edu wrote:

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..3dffdff 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
>  }
>  
>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> -				const unsigned char *sha1);
> +				const unsigned char *sha1, unsigned flags);

I'm not sure it's a good idea to add a flags field here; most of the
callbacks don't use it, and as you probably noticed, it makes the patch
a lot noisier. It does let you directly use pgp_verify_tag like this:

>  	if (cmdmode == 'v')
> -		return for_each_tag_name(argv, verify_tag);
> +		return for_each_tag_name(argv, pgp_verify_tag,
> +				GPG_VERIFY_VERBOSE);

but I think that is coupling too closely. What happens later when the
public, multi-file pgp_verify_tag function changes its interface? Or we
want to change our interface here, and it no longer matches
pgp_verify_tag? The results ripple a lot further than they should.

I think you probably want to keep a simple adapter callback in this
file, like:

  int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
  {
	return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
  }

> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f776778..8abc357 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -30,6 +30,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),
> @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  	if (verbose)
>  		flags |= GPG_VERIFY_VERBOSE;
>  
> -	while (i < argc)
> -		if (pgp_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;
> +		}
> +
> +		if (pgp_verify_tag(name, NULL, sha1, flags))
> +			had_error = 1;
> +
> +	}

So this is a good example of the rippling I mentioned earlier.

As a side note, it might actually be an improvement for pgp_verify_tag
to take a sha1 (so that git-tag is sure that it is verifying the same
object that it is printing), but that refactoring should probably come
separately, I think.

-Peff

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

> >  	if (cmdmode == 'v')
> > -		return for_each_tag_name(argv, verify_tag);
> > +		return for_each_tag_name(argv, pgp_verify_tag,
> > +				GPG_VERIFY_VERBOSE);
> 
> but I think that is coupling too closely. What happens later when the
> public, multi-file pgp_verify_tag function changes its interface? Or we
> want to change our interface here, and it no longer matches
> pgp_verify_tag? The results ripple a lot further than they should.
> 
> I think you probably want to keep a simple adapter callback in this
> file, like:
> 
>   int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
>   {
> 	return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
>   }

Yes, agreed. I'll give this a go

Thanks!
-Santiago

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > index f776778..8abc357 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -30,6 +30,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),
> > @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> >  	if (verbose)
> >  		flags |= GPG_VERIFY_VERBOSE;
> >  
> > -	while (i < argc)
> > -		if (pgp_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;
> > +		}
> > +
> > +		if (pgp_verify_tag(name, NULL, sha1, flags))
> > +			had_error = 1;
> > +
> > +	}
> 
> So this is a good example of the rippling I mentioned earlier.
> 
> As a side note, it might actually be an improvement for pgp_verify_tag
> to take a sha1 (so that git-tag is sure that it is verifying the same
> object that it is printing), but that refactoring should probably come
> separately, I think.
> 
> -Peff

Just to be sure, this refactoring is something we should still include
in this set of patches, right? I think that otherwise we'd lose the
desambigutaion that git tag -v does in this patch.

I also think that most of the rippling is gone if we use and adaptor as
you suggested. Should I add a patch on top of this to support a sha1 as
part for gpg_verify_tag()?

Thanks!
-Santiago.

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Jeff King]

On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:

> > As a side note, it might actually be an improvement for pgp_verify_tag
> > to take a sha1 (so that git-tag is sure that it is verifying the same
> > object that it is printing), but that refactoring should probably come
> > separately, I think.
> 
> Just to be sure, this refactoring is something we should still include
> in this set of patches, right? I think that otherwise we'd lose the
> desambigutaion that git tag -v does in this patch.

I think it can be part of this series, but doesn't have to be. As I
understand it, the current code is just handing the name to the `git
verify-tag` process, so if we continue to do so, that would be OK.

> I also think that most of the rippling is gone if we use and adaptor as
> you suggested. Should I add a patch on top of this to support a sha1 as
> part for gpg_verify_tag()?

Yes, though I'd generally advise against a function taking either a name or
a sha1, and ignoring the other option. That often leads to confusing
interfaces for the callers. Instead, perhaps just take the sha1, and let
the caller do the get_sha1() themselves. Or possibly provide two
functions, one of which is a convenience to translate the name to sha1
and then call the other.

-Peff

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

On Mon, Apr 04, 2016 at 09:38:54AM -0400, Jeff King wrote:
> On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:
> 
> > > As a side note, it might actually be an improvement for pgp_verify_tag
> > > to take a sha1 (so that git-tag is sure that it is verifying the same
> > > object that it is printing), but that refactoring should probably come
> > > separately, I think.
> > 
> > Just to be sure, this refactoring is something we should still include
> > in this set of patches, right? I think that otherwise we'd lose the
> > desambigutaion that git tag -v does in this patch.
> 
> I think it can be part of this series, but doesn't have to be. As I
> understand it, the current code is just handing the name to the `git
> verify-tag` process, so if we continue to do so, that would be OK.

IIRC, the current code for git tag -v hands the hex-representation[1] of
the sha1 to git verify-tag --- I believe that's related to the
desamgibuation issue I've seen people discuss.  I think this behavior is
lost unless we add this on top of the patch.

> 
> > I also think that most of the rippling is gone if we use and adaptor as
> > you suggested. Should I add a patch on top of this to support a sha1 as
> > part for gpg_verify_tag()?
> 
> Yes, though I'd generally advise against a function taking either a name or
> a sha1, and ignoring the other option. That often leads to confusing
> interfaces for the callers. Instead, perhaps just take the sha1, and let
> the caller do the get_sha1() themselves. Or possibly provide two
> functions, one of which is a convenience to translate the name to sha1
> and then call the other.

I think the former sounds easier. I can replace the name argument and
move the sha1-resolution code to in verify-tag. git tag -v already
resolves the tagname to a sha1, so it is easier there.

Does this sound reasonable? 

Thanks!
-Santiago

[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c#n109

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Jeff King]

On Mon, Apr 04, 2016 at 02:24:48PM -0400, Santiago Torres wrote:

> > I think it can be part of this series, but doesn't have to be. As I
> > understand it, the current code is just handing the name to the `git
> > verify-tag` process, so if we continue to do so, that would be OK.
> 
> IIRC, the current code for git tag -v hands the hex-representation[1] of
> the sha1 to git verify-tag --- I believe that's related to the
> desamgibuation issue I've seen people discuss.  I think this behavior is
> lost unless we add this on top of the patch.

Oh, you're right. I didn't notice that. So yeah, we should make sure in
this series to hand the sha1 over to gpg_verify_tag().

> > Yes, though I'd generally advise against a function taking either a name or
> > a sha1, and ignoring the other option. That often leads to confusing
> > interfaces for the callers. Instead, perhaps just take the sha1, and let
> > the caller do the get_sha1() themselves. Or possibly provide two
> > functions, one of which is a convenience to translate the name to sha1
> > and then call the other.
> 
> I think the former sounds easier. I can replace the name argument and
> move the sha1-resolution code to in verify-tag. git tag -v already
> resolves the tagname to a sha1, so it is easier there.
> 
> Does this sound reasonable?

Yes, I think that is a good solution.

Thanks.

-Peff