threads / patch / 42074

v7, 6 partsverify-tag: prepare verify_tag for libification

Subject: [PATCH v7 4/6] verify-tag: prepare verify_tag for libification

## tl;dr

11 messages between Apr 19, 2016 and Apr 19, 2016. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · lore

[PATCH v7 0/6] Move PGP verification out of verify-tag

From: Santiago Torres <santiago@nyu.edu>

This is a follow up of [1], [2], [3], [4], [5], [6]. patches 1/6, 2/6, are the same as the corresponding commits in pu.

v7: Mostly style/clarity changes mostly. Thanks Peff, Eric and Junio for the feedback! In summary: 

 * Eric pointed out issues with 3/6's commit message. It doesn't match the one 
   in pu though. I also took the opportunity to update payload_size to a size_t
   as Peff suggested.
 * 4/6 I updated report_name to name_to_report, I updated the commit message 
   and addressed some nits in the code, one of the fixes removed all three nits
   that Eric pointed out. I updated 5/6 to match these changes
 * I gave the commit message on 6/6 another go.
v6: 
 * As Junio suggested, updated 4/6, to include the name argument and the
   ternary operator to provide more descriptive error messages. I propagated
   these changes to 5/6 and 6/6 as well. I'm unsure about the 80-column
   on 4/6, the ternary operator is rather long.
 * Updated and reviewed the commit messages based on Eric and Junio's
   feedback

v5: Added helpful feedback by Eric

 * Reordering of the patches, to avoid temporal inclusion of a regression
 * Fix typos here and there.
 * Review commit messages, as some weren't representative of what the patches
   were doing anymore.
 * Updated t7030 to include Peff's suggestion, and added a helped-by line here
   as it was mostly Peff's code.
 * Updated the error-handling/printing issues that were introduced when.
   libifying the verify_tag function.
v4:
Thanks Eric, Jeff, and Hannes for the feedback.
 * I relocated the sigchain_push call so it comes after the error on
   gpg-interface (thanks Hannnes for catching this).
 * I updated the unit test to match the discussion on [3]. Now it generates
   the expected output of the tag on the fly for comparison. (This is just
   copy and paste from [3], but I verified that it works by breaking the
   while)
 * I split moving the code and renaming the variables into two patches so
   these are easier to review.
 * I used an adapter on builtin/tag.c instead of redefining all the fn*
   declarations everywhere. This introduces an issue with the way git tag -v
   resolves refnames though. I added a new commit to restore the previous
   behavior of git-tag. I'm not sure if I should've split this into two commits
   though.

v3: Thanks Eric, Jeff, for the feedback.

 * I separated the patch in multiple sub-patches.
 * I compared the behavior of previous git tag -v and git verify-tag 
   invocations to make sure the behavior is the same
 * I dropped the multi-line comment, as suggested.
 * I fixed the issue with the missing brackets in the while (this is 
   now detected by the test).
v2:
 * I moved the pgp-verification code to tag.c 
 * I added extra arguments so git tag -v and git verify-tag both work
   with the same function
 * Relocated the SIGPIPE handling code in verify-tag to gpg-interface
v1:
 
The verify tag function is just a thin wrapper around the verify-tag
command. We can avoid one fork call by doing the verification inside
the tag builtin instead.

This applies on v2.8.0. Thanks! -Santiago

[1] http://thread.gmane.org/gmane.comp.version-control.git/287649 [2] http://thread.gmane.org/gmane.comp.version-control.git/289836 [3] http://thread.gmane.org/gmane.comp.version-control.git/290608 [4] http://thread.gmane.org/gmane.comp.version-control.git/290731 [5] http://thread.gmane.org/gmane.comp.version-control.git/290790 [6] http://thread.gmane.org/gmane.comp.version-control.git/291780

Santiago Torres (6):
  builtin/verify-tag.c: ignore SIGPIPE in gpg-interface
  t7030: test verifying multiple tags
  verify-tag: Update run_gpg_verfy len variable name and type
  verify-tag: prepare verify_tag for libification
  verify-tag: move tag verification code to tag.c
  tag -v: verfy directly rather than exec-ing verify-tag
 builtin/tag.c         |  8 +------
 builtin/verify-tag.c  | 62 +++++++--------------------------------------------
 gpg-interface.c       |  2 ++
 t/t7030-verify-tag.sh | 13 +++++++++++
 tag.c                 | 53 +++++++++++++++++++++++++++++++++++++++++++
 tag.h                 |  2 ++
 6 files changed, 79 insertions(+), 61 deletions(-)
-- 
2.8.0
santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · re: santiago@nyu.edu · lore

[PATCH v7 1/6] builtin/verify-tag.c: ignore SIGPIPE in gpg-interface

From: Santiago Torres <santiago@nyu.edu>

The verify_signed_buffer() function may trigger a SIGPIPE 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 in builtin/verify-tag.c to avoid this issue.

However, any other caller who wants to call verify_signed_buffer() would have to do the same.

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

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

Signed-off-by: Santiago Torres <santiago@nyu.edu>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/verify-tag.c | 3 ---
 gpg-interface.c      | 2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)
Show changes to 2 files +2 −3

builtin/verify-tag.c, gpg-interface.c

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
santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · re: santiago@nyu.edu · lore

[PATCH v7 2/6] t7030: test verifying multiple tags

From: Santiago Torres <santiago@nyu.edu>

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

Add a test invoking it with multiple tags.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Santiago Torres <santiago@nyu.edu>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7030-verify-tag.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t7030-verify-tag.sh +13 −0
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 4608e71..07079a4 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -112,4 +112,17 @@ test_expect_success GPG 'verify signatures with --raw' '
 	)
 '
 
+test_expect_success GPG 'verify multiple tags' '
+	tags="fourth-signed sixth-signed seventh-signed" &&
+	for i in $tags
+	do
+		git verify-tag -v --raw $i || return 1
+	done >expect.stdout 2>expect.stderr.1 &&
+	grep "^.GNUPG:." <expect.stderr.1 >expect.stderr &&
+	git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
+	grep "^.GNUPG:." <actual.stderr.1 >actual.stderr &&
+	test_cmp expect.stdout actual.stdout &&
+	test_cmp expect.stderr actual.stderr
+'
+
 test_done

-- 
2.8.0
santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · re: santiago@nyu.edu · lore

[PATCH v7 3/6] verify-tag: update variable name and type

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. Clarify which one pertains to the length of the tag headers by renaming len to payload_size. Additionally, change the type of payload_size to size_t to match the return type of parse_signature.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/verify-tag.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Show changes to builtin/verify-tag.c +6 −5
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 77f070a..fa26e40 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	int len;
+	size_t payload_size;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	len = parse_signature(buf, size);
+	payload_size = parse_signature(buf, size);
 
-	if (size == len) {
+	if (size == payload_size) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, len);
+			write_in_full(1, buf, payload_size);
 		return error("no signature found");
 	}
 
-	ret = check_signature(buf, len, buf + len, size - len, &sigc);
+	ret = check_signature(buf, payload_size, buf + payload_size,
+				size - payload_size, &sigc);
 	print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);

-- 
2.8.0
santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · re: santiago@nyu.edu · lore
From: Santiago Torres <santiago@nyu.edu>

The current interface of verify_tag() resolves reference names to SHA1, however, the plan is to make this functionality public and the current interface is cumbersome for callers: they are expected to supply the textual representation of a sha1/refname. In many cases, this requires them to turn the sha1 to hex representation, just to be converted back inside verify_tag.

Add a SHA1 parameter to use instead of the name parameter, and rename the name parameter to "name_to_report" for reporting purposes only.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/verify-tag.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)
Show changes to builtin/verify-tag.c +18 −9
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index fa26e40..01e956f 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -42,25 +42,28 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 	return ret;
 }
 
-static int verify_tag(const char *name, unsigned flags)
+static int verify_tag(const unsigned char *sha1, const char *name_to_report,
+			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));
+				name_to_report ?
+				name_to_report :
+				find_unique_abbrev(sha1, DEFAULT_ABBREV),
+				typename(type));
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf)
-		return error("%s: unable to read file.", name);
+		return error("%s: unable to read file.",
+				name_to_report ?
+				name_to_report :
+				find_unique_abbrev(sha1, DEFAULT_ABBREV));
 
 	ret = run_gpg_verify(buf, size, flags);
 
@@ -80,6 +83,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
 	unsigned flags = 0;
+	unsigned char sha1[20];
+	const char *name;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
 		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
@@ -96,8 +101,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	while (i < argc)
-		if (verify_tag(argv[i++], flags))
+	while (i < argc) {
+		name = argv[i++];
+		if (get_sha1(name, sha1))
+			had_error = !!error("tag '%s' not found.", name);
+		else if (verify_tag(sha1, name, flags))
 			had_error = 1;
+	}
 	return had_error;
 }

-- 
2.8.0
Eric Sunshine· Apr 19, 2016, 19:05 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v7 4/6] verify-tag: prepare verify_tag for libification

On Tue, Apr 19, 2016 at 1:47 PM,  <santiago@nyu.edu> wrote:
Show 9 quoted lines
> The current interface of verify_tag() resolves reference names to SHA1,
> however, the plan is to make this functionality public and the current
> interface is cumbersome for callers: they are expected to supply the
> textual representation of a sha1/refname. In many cases, this requires
> them to turn the sha1 to hex representation, just to be converted back
> inside verify_tag.
>
> Add a SHA1 parameter to use instead of the name parameter, and rename
> the name parameter to "name_to_report" for reporting purposes only.

I'd have probably called this "display_name", but then I suppose it suffers the same issue Junio mentioned previously about it sounding like a boolean. Anyhow, as long as Junio is happy with it, that's what matters.

This version of the patch is nicely improved. One nit below.
Show 9 quoted lines
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -80,6 +83,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;

Mentioned previously[1]: These two declarations could be moved inside the while-loop scope (below).

[1]: http://article.gmane.org/gmane.comp.version-control.git/291813
Show 20 quoted lines
>         const struct option verify_tag_options[] = {
>                 OPT__VERBOSE(&verbose, N_("print tag contents")),
>                 OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
> @@ -96,8 +101,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>         if (verbose)
>                 flags |= GPG_VERIFY_VERBOSE;
>
> -       while (i < argc)
> -               if (verify_tag(argv[i++], flags))
> +       while (i < argc) {
> +               name = argv[i++];
> +               if (get_sha1(name, sha1))
> +                       had_error = !!error("tag '%s' not found.", name);
> +               else if (verify_tag(sha1, name, flags))
>                         had_error = 1;
> +       }
>         return had_error;
>  }
> --
> 2.8.0
Junio C Hamano· Apr 19, 2016, 19:36 UTC · re: Eric Sunshine · lore

Re: [PATCH v7 4/6] verify-tag: prepare verify_tag for libification

Eric Sunshine <sunshine@sunshineco.com> writes:
> I'd have probably called this "display_name", but then I suppose it
> suffers the same issue Junio mentioned previously about it sounding
> like a boolean. Anyhow, as long as Junio is happy with it, that's what
> matters.

No ;-) I am just trying to help people come up with patches in a better shape. Somehow "display name" does not bother me as much as "report name" did. name_to_report may be a mouthful, but it conveys what it is clearly, and I think it is good enough.

Show 17 quoted lines
>
> This version of the patch is nicely improved. One nit below.
>
>> Signed-off-by: Santiago Torres <santiago@nyu.edu>
>> ---
>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>> @@ -80,6 +83,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;
>
> Mentioned previously[1]: These two declarations could be moved inside
> the while-loop scope (below).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/291813
Yup.
Show 21 quoted lines
>
>>         const struct option verify_tag_options[] = {
>>                 OPT__VERBOSE(&verbose, N_("print tag contents")),
>>                 OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
>> @@ -96,8 +101,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>>         if (verbose)
>>                 flags |= GPG_VERIFY_VERBOSE;
>>
>> -       while (i < argc)
>> -               if (verify_tag(argv[i++], flags))
>> +       while (i < argc) {
>> +               name = argv[i++];
>> +               if (get_sha1(name, sha1))
>> +                       had_error = !!error("tag '%s' not found.", name);
>> +               else if (verify_tag(sha1, name, flags))
>>                         had_error = 1;
>> +       }
>>         return had_error;
>>  }
>> --
>> 2.8.0
santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · re: santiago@nyu.edu · lore

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

From: Santiago Torres <santiago@nyu.edu>

The PGP verification routine for tags could be accessed by other modules that require to do so.

Publish the verify_tag function in tag.c and rename it to gpg_verify_tag so it does not conflict with builtin/mktag's static function.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/verify-tag.c | 55 +---------------------------------------------------
 tag.c                | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tag.h                |  2 ++
 3 files changed, 56 insertions(+), 54 deletions(-)
Show changes to 3 files +56 −54

builtin/verify-tag.c, tag.c, tag.h

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 01e956f..714c899 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -18,59 +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;
-	size_t payload_size;
-	int ret;
-
-	memset(&sigc, 0, sizeof(sigc));
-
-	payload_size = parse_signature(buf, size);
-
-	if (size == payload_size) {
-		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, payload_size);
-		return error("no signature found");
-	}
-
-	ret = check_signature(buf, payload_size, buf + payload_size,
-				size - payload_size, &sigc);
-	print_signature_buffer(&sigc, flags);
-
-	signature_check_clear(&sigc);
-	return ret;
-}
-
-static int verify_tag(const unsigned char *sha1, const char *name_to_report,
-			unsigned flags)
-{
-	enum object_type type;
-	char *buf;
-	unsigned long size;
-	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_to_report ?
-				name_to_report :
-				find_unique_abbrev(sha1, DEFAULT_ABBREV),
-				typename(type));
-
-	buf = read_sha1_file(sha1, &type, &size);
-	if (!buf)
-		return error("%s: unable to read file.",
-				name_to_report ?
-				name_to_report :
-				find_unique_abbrev(sha1, DEFAULT_ABBREV));
-
-	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);
@@ -105,7 +52,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		name = argv[i++];
 		if (get_sha1(name, sha1))
 			had_error = !!error("tag '%s' not found.", name);
-		else if (verify_tag(sha1, name, flags))
+		else if (gpg_verify_tag(sha1, name, flags))
 			had_error = 1;
 	}
 	return had_error;
diff --git a/tag.c b/tag.c
index d72f742..ace619a 100644
--- a/tag.c
+++ b/tag.c
@@ -6,6 +6,59 @@
 
 const char *tag_type = "tag";
 
+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+{
+	struct signature_check sigc;
+	size_t payload_size;
+	int ret;
+
+	memset(&sigc, 0, sizeof(sigc));
+
+	payload_size = parse_signature(buf, size);
+
+	if (size == payload_size) {
+		if (flags & GPG_VERIFY_VERBOSE)
+			write_in_full(1, buf, payload_size);
+		return error("no signature found");
+	}
+
+	ret = check_signature(buf, payload_size, buf + payload_size,
+				size - payload_size, &sigc);
+	print_signature_buffer(&sigc, flags);
+
+	signature_check_clear(&sigc);
+	return ret;
+}
+
+extern int gpg_verify_tag(const unsigned char *sha1,
+		const char *name_to_report, unsigned flags)
+{
+	enum object_type type;
+	char *buf;
+	unsigned long size;
+	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_to_report ?
+				name_to_report :
+				find_unique_abbrev(sha1, DEFAULT_ABBREV),
+				typename(type));
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.",
+				name_to_report ?
+				name_to_report :
+				find_unique_abbrev(sha1, DEFAULT_ABBREV));
+
+	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..a5721b6 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 gpg_verify_tag(const unsigned char *sha1,
+		const char *name_to_report, unsigned flags);
 
 #endif /* TAG_H */

-- 
2.8.0
santiago@nyu.edu· Apr 19, 2016, 17:47 UTC · re: santiago@nyu.edu · lore

[PATCH v7 6/6] tag -v: verfy directly rather than exec-ing verify-tag

From: Santiago Torres <santiago@nyu.edu>

Instead of having tag -v fork to run verify-tag, use the gpg_verify_tag() function directly.

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

-- 
2.8.0
Eric Sunshine· Apr 19, 2016, 19:06 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v7 6/6] tag -v: verfy directly rather than exec-ing verify-tag

On Tue, Apr 19, 2016 at 1:47 PM,  <santiago@nyu.edu> wrote:
> tag -v: verfy directly rather than exec-ing verify-tag
s/verfy/verify:
> Instead of having tag -v fork to run verify-tag, use the
> gpg_verify_tag() function directly.
This description is easy enough to understand. Thanks.
Show 19 quoted lines
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..7b2918e 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>                                 const unsigned char *sha1)
>  {
> -       const char *argv_verify_tag[] = {"verify-tag",
> -                                       "-v", "SHA1_HEX", NULL};
> -       argv_verify_tag[2] = sha1_to_hex(sha1);
> -
> -       if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
> -               return error(_("could not verify the tag '%s'"), name);
> -       return 0;
> +       return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
>  }
Eric Sunshine· Apr 19, 2016, 19:10 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v7 0/6] Move PGP verification out of verify-tag

On Tue, Apr 19, 2016 at 1:47 PM,  <santiago@nyu.edu> wrote:
Show 14 quoted lines
> This is a follow up of [1], [2], [3], [4], [5], [6]. patches 1/6, 2/6, are the
> same as the corresponding commits in pu.
>
> v7:
> Mostly style/clarity changes mostly. Thanks Peff, Eric and Junio for the
> feedback! In summary:
>
>  * Eric pointed out issues with 3/6's commit message. It doesn't match the one
>    in pu though. I also took the opportunity to update payload_size to a size_t
>    as Peff suggested.
>  * 4/6 I updated report_name to name_to_report, I updated the commit message
>    and addressed some nits in the code, one of the fixes removed all three nits
>    that Eric pointed out. I updated 5/6 to match these changes
>  * I gave the commit message on 6/6 another go.

Thanks, this re-roll looks good. My reviews mention a couple nits, but I hope they are not worth a re-roll (perhaps Junio can address them when/if he picks up the series).

As before, the entire series is:
    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

← back to recent threads