[PATCH] tag.c: move PGP verification code from plumbing

[PATCH] tag.c: move PGP verification code from plumbing

[santiago]

From: Santiago Torres <santiago@nyu.edu>

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.

To do this, the run_pgp_verify() and verify_tag() functions are moved to
tag.c. The definition of verify_tag was changed to support extra
arguments that match the builtin/tag and builtin/verify-tag modules. The
SIGPIPE ignore call in tag-verify was also moved to a more sensible
place, now that both modules need it.

The function name was also changed to pgp_verify_tag to avoid conflicts with
mktag.c's.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c        | 26 ++++++++---------------
 builtin/verify-tag.c | 60 ++++++----------------------------------------------
 gpg-interface.c      |  6 ++++++
 tag.c                | 48 +++++++++++++++++++++++++++++++++++++++++
 tag.h                |  2 ++
 5 files changed, 72 insertions(+), 70 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..af8f3ba 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,32 +87,22 @@ 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)
 {
@@ -424,9 +415,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 00663f6..2e6a175 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);
@@ -78,6 +29,8 @@ static int git_verify_tag_config(const char *var, const char *value, void *cb)
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
+	unsigned char sha1[20];
+	const char *name;
 	unsigned flags = 0;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
@@ -95,11 +48,12 @@ 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))
+		name = argv[i++];
+		if (get_sha1(name, sha1))
+			return error("tag '%s' not found.", name);
+
+		if (pgp_verify_tag(NULL, NULL, sha1, flags))
 			had_error = 1;
 	return had_error;
 }
diff --git a/gpg-interface.c b/gpg-interface.c
index 3dc2fe3..1b421b7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	if (gpg_output)
 		gpg.err = -1;
 	args_gpg[3] = path;
+
+	/* sometimes the program was terminated because this signal
+	 * was received in the process of writing the gpg input.
+	 * We ignore it for this call and restore it afterwards */
+	sigchain_push(SIGPIPE, SIG_IGN);
 	if (start_command(&gpg)) {
 		unlink(path);
 		return error(_("could not run gpg."));
@@ -250,6 +255,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);
 
diff --git a/tag.c b/tag.c
index d72f742..7097a84 100644
--- a/tag.c
+++ b/tag.c
@@ -3,9 +3,57 @@
 #include "commit.h"
 #include "tree.h"
 #include "blob.h"
+#include "sigchain.h"
 
 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, const char *ref,
+		const unsigned char *sha1, unsigned flags)
+{
+
+	enum object_type type;
+	unsigned long size;
+	const char* buf;
+	int ret;
+
+	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);
+
+	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..22289a5 100644
--- a/tag.h
+++ b/tag.h
@@ -17,5 +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, const char *ref,
+		const unsigned char *sha1, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.7.3

Re: [PATCH] tag.c: move PGP verification code from plumbing

[Eric Sunshine]

On Thu, Mar 24, 2016 at 8:33 PM,  <santiago@nyu.edu> wrote:
> 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.

Hopefully, the below review comments are meaningful, however, aside
from having just read Peff's review of the previous version of this
patch, I haven't been following this discussion, so it's possible some
comments may be off the mark. Caveat emptor.

> To do this, the run_pgp_verify() and verify_tag() functions are moved to
> tag.c. The definition of verify_tag was changed to support extra
> arguments that match the builtin/tag and builtin/verify-tag modules. The
> SIGPIPE ignore call in tag-verify was also moved to a more sensible
> place, now that both modules need it.

This patch is doing too much, thus making it difficult to review and
reason about. Based upon this paragraph alone, you may want to split
the patch into three or more patches. At the very least, have a patch
which does the code movement (and nothing else); one which adjusts the
argument lists; and one which adjusts SIGPIPE handling. More
generally, figure out the distinct conceptual changes being made here,
and give each such change its own patch.

> The function name was also changed to pgp_verify_tag to avoid conflicts with
> mktag.c's.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---

Right here below the "---" line is where, for the sake of reviewers,
you would explain what changed since the previous version. Also, as a
reviewer aid, provide a link to the previous discussion, like this[1].
Finally, indicate the version number of the patch in the subject, like
this [PATCH v3] (see git-format-patch's -v option).

More below...

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289803

> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -86,32 +87,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
>         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;
> -}
>

This deletion inconsistently leaves an extra blank line between
functions; thus you'd want to delete one more (blank) line.

>  static int do_sign(struct strbuf *buffer)
>  {
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -95,11 +48,12 @@ 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))
> +               name = argv[i++];
> +               if (get_sha1(name, sha1))
> +                       return error("tag '%s' not found.", name);
> +
> +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
>                         had_error = 1;

Meh, this isn't Python. Due to the missing braces, the only thing
inside the while() loop is the assignment to 'name'; all the other
indented code is outside the while().

Did you run the test suite following this change? Did it all pass? If
so, perhaps an additional test or two to catch this sort of error
would be warranted.

>         return had_error;
>  }
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>         if (gpg_output)
>                 gpg.err = -1;
>         args_gpg[3] = path;
> +
> +       /* sometimes the program was terminated because this signal
> +        * was received in the process of writing the gpg input.
> +        * We ignore it for this call and restore it afterwards */

I realize that the bulk of this comment block was merely relocated
from builtin/verify-tag.c, however, now would be a good time to fix
its style violation and format it like this:

    /*
     * This is a multi-line
     * comment.
     */

Also, the last line of the comment, which you added when relocating
it, merely repeats what the code itself already says clearly, thus is
not particularly useful and should be dropped.

> +       sigchain_push(SIGPIPE, SIG_IGN);
>         if (start_command(&gpg)) {
>                 unlink(path);
>                 return error(_("could not run gpg."));
> @@ -250,6 +255,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);

Re: [PATCH] tag.c: move PGP verification code from plumbing

[Jeff King]

On Fri, Mar 25, 2016 at 01:23:57AM -0400, Eric Sunshine wrote:

> On Thu, Mar 24, 2016 at 8:33 PM,  <santiago@nyu.edu> wrote:
> > 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.
> 
> Hopefully, the below review comments are meaningful, however, aside
> from having just read Peff's review of the previous version of this
> patch, I haven't been following this discussion, so it's possible some
> comments may be off the mark. Caveat emptor.

Thanks for reviewing.  I agree with all of the comments you made, but
I'd add a little more to the last one:

> > +
> > +       /* sometimes the program was terminated because this signal
> > +        * was received in the process of writing the gpg input.
> > +        * We ignore it for this call and restore it afterwards */
> 
> I realize that the bulk of this comment block was merely relocated
> from builtin/verify-tag.c, however, now would be a good time to fix
> its style violation and format it like this:
> 
>     /*
>      * This is a multi-line
>      * comment.
>      */
> 
> Also, the last line of the comment, which you added when relocating
> it, merely repeats what the code itself already says clearly, thus is
> not particularly useful and should be dropped.

I actually think we can drop this comment entirely. Pushing and popping
SIGPIPE when piping to a sub-program like this is not that exotic in our
code base. And if the SIGPIPE handling here is done in its own patch,
then if somebody wants to see more discussion or reasoning, they can go
to its commit message (which can then go into more detail about why we
might see SIGPIPE, and not just a vague "sometimes...").

-Peff

Re: [PATCH] tag.c: move PGP verification code from plumbing

[Santiago Torres]

> > -       signal(SIGPIPE, SIG_IGN);
> >         while (i < argc)
> > -               if (verify_tag(argv[i++], flags))
> > +               name = argv[i++];
> > +               if (get_sha1(name, sha1))
> > +                       return error("tag '%s' not found.", name);
> > +
> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
> >                         had_error = 1;
> 
> Meh, this isn't Python. Due to the missing braces, the only thing
> inside the while() loop is the assignment to 'name'; all the other
> indented code is outside the while().
> 
> Did you run the test suite following this change? Did it all pass? If
> so, perhaps an additional test or two to catch this sort of error
> would be warranted.

Wow, you're right! I just re-ran the tests again to make sure I didn't
miss anything. All the tests pass for me, so I'll write an extra case to
avoid this. Just to be sure, I should include it in t7030-verify-tag.sh
right?

All your other comments seem straightforward and on point. I'll apply
them right away :)

Thanks!
-Santiago.

Re: [PATCH] tag.c: move PGP verification code from plumbing

[Eric Sunshine]

On Fri, Mar 25, 2016 at 10:45 AM, Santiago Torres <santiago@nyu.edu> wrote:
>> >         while (i < argc)
>> > -               if (verify_tag(argv[i++], flags))
>> > +               name = argv[i++];
>> > +               if (get_sha1(name, sha1))
>> > +                       return error("tag '%s' not found.", name);
>> > +
>> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
>> >                         had_error = 1;
>>
>> Meh, this isn't Python. Due to the missing braces, the only thing
>> inside the while() loop is the assignment to 'name'; all the other
>> indented code is outside the while().
>>
>> Did you run the test suite following this change? Did it all pass? If
>> so, perhaps an additional test or two to catch this sort of error
>> would be warranted.
>
> Wow, you're right! I just re-ran the tests again to make sure I didn't
> miss anything. All the tests pass for me, so I'll write an extra case to
> avoid this. Just to be sure, I should include it in t7030-verify-tag.sh
> right?

Generally speaking, it is desirable to have test coverage for all
functionality you're refactoring to ensure that the refactoring
doesn't break that functionality.

t7030-verify-tag.sh indeed seems like a good place to add a new test
for catching this sort of regression.

t7004-tag.sh is also of interest since, in an earlier version of this
patch, if I recall correctly, Peff caught a regression where "git tag
-v" failed to pass the GPG_VERIFY_VERBOSE flag, and I don't think
t7004-tag.sh would have caught that problem either, so a new test for
that would be good, as well.

Speaking of GPG_VERIFY_VERBOSE, now that I'm examining the changes
more closely, it appears that this version of the patch no longer
respects GPG_VERIFY_VERBOSE at all but is instead hard-coded to be
verbose. Is that desirable or intentional?