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

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

[santiago]

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

v4 (this):

Thanks Eric, Peff, 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

[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 
[3] http://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-td7652334.html

[PATCH v4 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>
---
	Note: On this version i moved the sigchain_push below the return error
in the same way that sign_buffer does(). Thanks to Johanes Sixt for catching
this. 

 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

[PATCH v4 2/6] 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 tags separately and then
compares the result against the invocation with the same three tags as
an argument. The results shouldn't differ.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
	Note: In this version we don't check or uniq for the verify output, but
instead compare against an invocation for each of the three tags indivdually.

 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..f9161332 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' '
 	)
 '
 
+test_expect_success GPG 'verify multiple tags' '
+	tags="fourth-signed sixth-signed seventh-signed" &&
+	for i in $tags; do
+		git verify-tag -v --raw $i || return 1
+	done >expect.stdout 2>expect.stderr.1 &&
+	grep GOODSIG <expect.stderr.1 >expect.stderr &&
+	git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
+	grep GOODSIG <actual.stderr.1 >actual.stderr &&
+	test_cmp expect.stdout actual.stdout &&
+	test_cmp expect.stderr actual.stderr
+'
+
 test_done
-- 
2.8.0

[PATCH v4 3/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
code. 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>
---
	Note: In this version, I just blndly moved code around and renamed
the function's name for ease of review. The following patch only renames
variables.

 builtin/verify-tag.c | 51 +--------------------------------------------------
 tag.c                | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 tag.h                |  1 +
 3 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 77f070a..7e36d53 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 (gpg_verify_tag(argv[i++], flags))
 			had_error = 1;
 	return had_error;
 }
diff --git a/tag.c b/tag.c
index d72f742..81e86e6 100644
--- a/tag.c
+++ b/tag.c
@@ -6,6 +6,55 @@
 
 const char *tag_type = "tag";
 
+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;
+}
+
+int gpg_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;
+}
+
 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..877f180 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 char *name, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.8.0

[PATCH v4 4/6] tag.c: Replace varialbe 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>
---
	Note: this used to be part of the previous patch on v3. I moved the
renaming part to a single patch to simplify the review. 

 tag.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tag.c b/tag.c
index 81e86e6..f6443db 100644
--- a/tag.c
+++ b/tag.c
@@ -9,20 +9,21 @@ const char *tag_type = "tag";
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	int len;
+	int payload_length;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	len = parse_signature(buf, size);
+	payload_length = parse_signature(buf, size);
 
-	if (size == len) {
+	if (size == payload_length) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, len);
+			write_in_full(1, buf, payload_length);
 		return error("no signature found");
        }
 
-	ret = check_signature(buf, len, buf + len, size - len, &sigc);
+	ret = check_signature(buf, payload_length, buf + payload_length,
+				size - payload_length, &sigc);
 	print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
-- 
2.8.0

[PATCH v4 5/6] 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>
---
	Note: This follows Peff's suggestion to use an adapter, instead of 
changing the the fn* pointer structure to match gpg_verify_tag.

 builtin/tag.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..f4450f8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,
 static int verify_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	const char *argv_verify_tag[] = {"verify-tag",
-					"-v", "SHA1_HEX", NULL};
-	argv_verify_tag[2] = sha1_to_hex(sha1);
-
-	if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
-		return error(_("could not verify the tag '%s'"), name);
-	return 0;
+	return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
 }
 
 static int do_sign(struct strbuf *buffer)
-- 
2.8.0

[PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1

[santiago]

From: Santiago Torres <santiago@nyu.edu>

The gpg_verify_tag function resolves the ref for any existing object.
However, git tag -v resolves to only tag-refs. We can provide support
for sha1 by moving the refname resolution code out of gpg_verify_tag and
allow for the object's sha1 as an argument.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  9 +++++++--
 tag.c                | 12 ++++--------
 tag.h                |  2 +-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index f4450f8..398c892 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref,
 static int verify_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
+	return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 7e36d53..2ff01d8 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -30,6 +30,7 @@ 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 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 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	while (i < argc)
-		if (gpg_verify_tag(argv[i++], flags))
+	while (i < argc) {
+		if (get_sha1(argv[i++], sha1))
+			return error("tag '%s' not found.", argv[i]);
+
+		if (gpg_verify_tag(sha1, flags))
 			had_error = 1;
+	}
 	return had_error;
 }
diff --git a/tag.c b/tag.c
index f6443db..8ac9de5 100644
--- a/tag.c
+++ b/tag.c
@@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 	return ret;
 }
 
-int gpg_verify_tag(const char *name, unsigned flags)
+int gpg_verify_tag(const unsigned char *sha1, unsigned flags)
 {
 	enum object_type type;
-	unsigned char sha1[20];
 	char *buf;
 	unsigned long size;
 	int ret;
 
-	if (get_sha1(name, sha1))
-		return error("tag '%s' not found.", name);
-
 	type = sha1_object_info(sha1, NULL);
 	if (type != OBJ_TAG)
-		return error("%s: cannot verify a non-tag object of type %s.",
-				name, typename(type));
+		return error("cannot verify a non-tag object of type %s.",
+				typename(type));
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf)
-		return error("%s: unable to read file.", name);
+		return error("unable to read file.");
 
 	ret = run_gpg_verify(buf, size, flags);
 
diff --git a/tag.h b/tag.h
index 877f180..cb643b9 100644
--- a/tag.h
+++ b/tag.h
@@ -17,6 +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 char *name, unsigned flags);
+extern int gpg_verify_tag(const unsigned char *sha1, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.8.0

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

[Eric Sunshine]

On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> t/t7030-verify-tag.sh: Adds validation for multiple tags

Rewrite:

    t7030: test verify-tag with multiple tags

The leading "t/" and the trailing "-*.sh" were dropped since they add no value.

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

s/mutliple/multiple/

> However, no previous tests try to verify multiple tags at once. This
> test runs the verify-tag command against three tags separately and then
> compares the result against the invocation with the same three tags as
> an argument. The results shouldn't differ.

The bulk of this description is merely repeating what the patch itself
already says, thus is redundant. The entire commit message could
probably be collapsed to:

    t7030: test verify-tag with multiple tags

    git-verify-tag accepts multiple tags as arguments, however,
    existing tests only ever invoke it with a single tag, so add a
    test invoking it with multiple tags.

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

Hmm, I had expected you to adopt Peff's suggestion[1] for the greps:

    grep '^.GNUPG:.' ...

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

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

Re: [PATCH v4 4/6] tag.c: Replace varialbe name for readability

[Eric Sunshine]

On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> tag.c: Replace varialbe name for readability

s/Replace/replace/
s/varialbe/variable/

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

Re: [PATCH v4 5/6] tag: use pgp_verify_function in tag -v call

[Eric Sunshine]

On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> Instead of running the verify-tag plumbing command, we use the
> pgp_verify_tag(). This avoids the usage of an extra fork call. To do
> this, we extend the number of parameters that tag.c takes, and
> verify-tag passes. Redundant calls done in the pgp_verify_tag function
> are removed.

I'm confused about everything following "an extra fork call" since
those subsequent sentences don't seem to pertain to this patch. Is
that leftover gunk from the previous version of this series?

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

Re: [PATCH v4 5/6] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

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

Yes, I thought I had taken this part of the commit message away. 
I'll take it out right away. Apologies for this.

-Santiago.

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

[Santiago Torres]

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

Yeah, this is actually clearer. Sorry for the academ-ose commit message.

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

I thought this was an stylistic thing. I can of course adopt this
suggestion.

Thanks,
-Santiago

Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1

[Eric Sunshine]

On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> tag.c: Change gpg_verify_tag argument to sha1

s/Change/change/

> The gpg_verify_tag function resolves the ref for any existing object.
> However, git tag -v resolves to only tag-refs. We can provide support
> for sha1 by moving the refname resolution code out of gpg_verify_tag and
> allow for the object's sha1 as an argument.

This description leaves me fairly clueless about why this change is
being made since justification seems to be lacking. More about this
below...

> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>                                 const unsigned char *sha1)
>  {
> -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
> +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);
>  }

So, by this point, 'name' has already been resolved to 'sha1', thus
this change avoids a second resolution of 'name' inside
gpg_verify_tag(). Therefore, this is really an optimization, right?
Perhaps the intent of the patch would be clearer if the commit message
sold it as such. For instance, the commit message might start off:

    tag: avoid resolving tag name twice

and then go on to say that by hefting tag name resolution out of
gpg_verify_tag(), the extra resolution can be avoided.

>  static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>         if (verbose)
>                 flags |= GPG_VERIFY_VERBOSE;
>
> -       while (i < argc)
> -               if (gpg_verify_tag(argv[i++], flags))
> +       while (i < argc) {
> +               if (get_sha1(argv[i++], sha1))
> +                       return error("tag '%s' not found.", argv[i]);

Why does this 'return' after the first error, but the gpg_verify_tag()
call below merely sets a 'had_error' flag and continues? I would
expect this one to set the flag and continue, as well.

> +

Style: unnecessary blank line

> +               if (gpg_verify_tag(sha1, flags))
>                         had_error = 1;
> +       }
>         return had_error;
>  }
> diff --git a/tag.c b/tag.c
> @@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> -int gpg_verify_tag(const char *name, unsigned flags)
> +int gpg_verify_tag(const unsigned char *sha1, unsigned flags)
>  {
>         enum object_type type;
> -       unsigned char sha1[20];
>         char *buf;
>         unsigned long size;
>         int ret;
>
> -       if (get_sha1(name, sha1))
> -               return error("tag '%s' not found.", name);
> -
>         type = sha1_object_info(sha1, NULL);
>         if (type != OBJ_TAG)
> -               return error("%s: cannot verify a non-tag object of type %s.",
> -                               name, typename(type));
> +               return error("cannot verify a non-tag object of type %s.",
> +                               typename(type));

This error message becomes much less useful since it now only says
that there is a problem with *some* tag but doesn't give any
identifying information. How about including the sha1 in the error
message?

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

Ditto regarding making this more useful by including the sha1.

>         ret = run_gpg_verify(buf, size, flags);

Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1

[Santiago Torres]

On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:
> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
> > tag.c: Change gpg_verify_tag argument to sha1
> 
> s/Change/change/

Sorry I've been consistently missing these... 
> 
> > The gpg_verify_tag function resolves the ref for any existing object.
> > However, git tag -v resolves to only tag-refs. We can provide support
> > for sha1 by moving the refname resolution code out of gpg_verify_tag and
> > allow for the object's sha1 as an argument.
> 
> This description leaves me fairly clueless about why this change is
> being made since justification seems to be lacking. More about this
> below...
> 
> > Signed-off-by: Santiago Torres <santiago@nyu.edu>
> > ---
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > @@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref,
> >  static int verify_tag(const char *name, const char *ref,
> >                                 const unsigned char *sha1)
> >  {
> > -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
> > +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);
> >  }
> 
> So, by this point, 'name' has already been resolved to 'sha1', thus
> this change avoids a second resolution of 'name' inside
> gpg_verify_tag(). Therefore, this is really an optimization, right?
> Perhaps the intent of the patch would be clearer if the commit message
> sold it as such. For instance, the commit message might start off:
> 
>     tag: avoid resolving tag name twice
> 
> and then go on to say that by hefting tag name resolution out of
> gpg_verify_tag(), the extra resolution can be avoided.

Yep, this is actually true, but something I didn't consider. I think
that, from what I could draw on [1] and [2], git tag -v is reserved to
tags only (refs/tags iirc). This patch makes it so that this behavior is
not lost. I'm not sure if it should be separate from 5/6 though. 
> 
> >  static int do_sign(struct strbuf *buffer)
> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> >         if (verbose)
> >                 flags |= GPG_VERIFY_VERBOSE;
> >
> > -       while (i < argc)
> > -               if (gpg_verify_tag(argv[i++], flags))
> > +       while (i < argc) {
> > +               if (get_sha1(argv[i++], sha1))
> > +                       return error("tag '%s' not found.", argv[i]);
> 
> Why does this 'return' after the first error, but the gpg_verify_tag()
> call below merely sets a 'had_error' flag and continues? I would
> expect this one to set the flag and continue, as well.

This sounds better than what I had thought. I'll set had error and do
continue instead.
> 
 >  {
> >         enum object_type type;
> > -       unsigned char sha1[20];
> >         char *buf;
> >         unsigned long size;
> >         int ret;
> >
> > -       if (get_sha1(name, sha1))
> > -               return error("tag '%s' not found.", name);
> > -
> >         type = sha1_object_info(sha1, NULL);
> >         if (type != OBJ_TAG)
> > -               return error("%s: cannot verify a non-tag object of type %s.",
> > -                               name, typename(type));
> > +               return error("cannot verify a non-tag object of type %s.",
> > +                               typename(type));
> 
> This error message becomes much less useful since it now only says
> that there is a problem with *some* tag but doesn't give any
> identifying information. How about including the sha1 in the error
> message?
> 
> >
> >         buf = read_sha1_file(sha1, &type, &size);
> >         if (!buf)
> > -               return error("%s: unable to read file.", name);
> > +               return error("unable to read file.");
> 
> Ditto regarding making this more useful by including the sha1.

yes, I wasn't sure about how to move forward here. I'll replace the name
with the sha1 instead of just removing it.

Thanks!
-Santiago.


[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c
[2]
http://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-tp7652334p7652437.html

Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1

[Eric Sunshine]

On Mon, Apr 4, 2016 at 10:10 PM, Santiago Torres <santiago@nyu.edu> wrote:
> On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:
>> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
>> > -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
>> > +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);
>>
>> So, by this point, 'name' has already been resolved to 'sha1', thus
>> this change avoids a second resolution of 'name' inside
>> gpg_verify_tag(). Therefore, this is really an optimization, right?
>> Perhaps the intent of the patch would be clearer if the commit message
>> sold it as such. For instance, the commit message might start off:
>>
>>     tag: avoid resolving tag name twice
>>
>> and then go on to say that by hefting tag name resolution out of
>> gpg_verify_tag(), the extra resolution can be avoided.
>
> Yep, this is actually true, but something I didn't consider. I think
> that, from what I could draw on [1] and [2], git tag -v is reserved to
> tags only (refs/tags iirc). This patch makes it so that this behavior is
> not lost. I'm not sure if it should be separate from 5/6 though.

Okay, so this is a fix for a regression introduced by the previous
patch. I agree that this is suboptimal. You can avoid this regression
issue altogether by merely re-ordering the patch series. For instance,
the series could be ordered like this:

1. SIGPIPE dance
2. add new t7030 test
3. improve variable name in verify_tag()
4. heft get_sha1() lookup out of verify_tag()
5. move code and publish gpg_verify_tag()
6. re-implement "git-tag -v" in terms of gpg_verify_tag()

Sell patch #4 as a libification preparation step, explaining as
justification that some future clients may already have the sha1 in
hand and would want to avoid duplicate resolution of the tag name.

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

[Eric Sunshine]

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

It's probably not a big deal, but Peff's suggestion (at least feels
like it) makes the test a bit more comprehensive.

By the way, as the test is heavily inspired by Peff's example, it
might be worth giving him a nod via a Helped-by: just above your
Signed-off-by:.