diff mbox series

[2/5] gpg-interface: support one-line summaries in print_signature_buffer()

Message ID 20200105135616.19102-3-hji@dyntopia.com (mailing list archive)
State New, archived
Headers show
Series refactor gpg-interface and add gpg verification for clones | expand

Commit Message

Hans Jerry Illikainen Jan. 5, 2020, 1:56 p.m. UTC
Most consumers of the GPG interface use print_signature_buffer() to show
the result from signature verifications.  One notable exception was
verify_merge_signature().

Previously, the verify_merge_signature() function processed the
signature_check structure in order to print a one-line summary of the
result.

I think that the one-line summary format has potential use in other
parts of Git; and not only for merges.  I also think that it makes sense
to have /one/ summary format in order to avoid confusing users with
different output styles for different operations.

This patch moves the one-line summary from verify_merge_signature() to
print_signature_buffer().  It also introduces a GPG_VERIFY_SHORT flag
that determines whether the summary should be printed.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 builtin/verify-commit.c |  3 ++-
 commit.c                | 23 +++++------------------
 gpg-interface.c         | 41 ++++++++++++++++++++++++++++++++++++++++-
 gpg-interface.h         |  4 +++-
 tag.c                   |  7 ++++---
 5 files changed, 54 insertions(+), 24 deletions(-)

Comments

Junio C Hamano Jan. 6, 2020, 7:33 p.m. UTC | #1
Hans Jerry Illikainen <hji@dyntopia.com> writes:

> Most consumers of the GPG interface use print_signature_buffer() to show
> This patch moves the one-line summary from verify_merge_signature() to
> print_signature_buffer().  It also introduces a GPG_VERIFY_SHORT flag
> that determines whether the summary should be printed.

I think the motivation makes sense.

> -void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
> +void print_signature_buffer(const struct object_id *oid,
> +			    const struct signature_check *sigc, int status,
> +			    unsigned flags)
>  {
>  	const char *output = flags & GPG_VERIFY_RAW ?
>  		sigc->gpg_status : sigc->gpg_output;
> +	char hex[GIT_MAX_HEXSZ + 1];
> +	char *type;
>  
>  	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
>  		fputs(sigc->payload, stdout);
>  
>  	if (flags & GPG_VERIFY_FULL && output)
>  		fputs(output, stderr);
> +
> +	if (flags & GPG_VERIFY_SHORT && oid) {

I am not sure about the wisdom of "&& oid" here.  Wouldn't it be a
bug for a caller who asks for _SHORT format to feed a NULL oid to
us?  I would understand

	if (flags & GPG_VERIFY_SHORT) {
		if (!oid)
			BUG("Caller asking for SHORT without oid???");

much better, but if there is a caller that has a legitimate need
tobe able to pass NULL and still request SHORT, let's see it and
discuss if it is a good idea.

For that matter, the two print routines we have immediately above
share the same issue.

> +		type = xstrdup_or_null(
> +			type_name(oid_object_info(the_repository, oid, NULL)));
> +		if (!type)
> +			type = xstrdup("object");

This is minor, but I wonder if this is easier to follow.

		type = type_name(oid_object_info(the_repository, oid, NULL));
		if (!type)
			type = "object";
		type = xstrdup(type);

> +		*type = toupper(*type);

This is not helpful at all for i18n/l10n, I am afraid.

> +		find_unique_abbrev_r(hex, oid, DEFAULT_ABBREV);
> +
> +		switch (sigc->result) {
> +		case 'G':
> +			if (status)
> +				error(_("%s %s has an untrusted GPG signature, "
> +					"allegedly by %s."),
> +				      type, hex, sigc->signer);

The original was of course

        die(_("Commit %s has an untrusted GPG signature, "
              "allegedly by %s."), hex, signature_check.signer);

so the message can properly localized including the typename.  That
is no longer true with this conversion.

You probably would need to prepare a 3-element array (one element
for each of <object, commit, tag>), each element of the array being
a 3-element array that holds the message for these three cases
(i.e. G, N and default) instead.
diff mbox series

Patch

diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 2a099ec6ba..acc01a7be9 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -28,7 +28,8 @@  static int run_gpg_verify(struct commit *commit, unsigned flags)
 	memset(&signature_check, 0, sizeof(signature_check));
 
 	ret = check_commit_signature(commit, &signature_check);
-	print_signature_buffer(&signature_check, flags);
+	print_signature_buffer(&commit->object.oid, &signature_check, ret,
+			       flags);
 
 	signature_check_clear(&signature_check);
 	return ret;
diff --git a/commit.c b/commit.c
index 3f91d3efc5..519599469b 100644
--- a/commit.c
+++ b/commit.c
@@ -1139,29 +1139,16 @@  int check_commit_signature(const struct commit *commit, struct signature_check *
 void verify_merge_signature(struct commit *commit, int verbosity,
 			    int check_trust)
 {
-	char hex[GIT_MAX_HEXSZ + 1];
 	struct signature_check signature_check;
 	int ret;
 	memset(&signature_check, 0, sizeof(signature_check));
 
 	ret = check_commit_signature(commit, &signature_check);
-
-	find_unique_abbrev_r(hex, &commit->object.oid, DEFAULT_ABBREV);
-	switch (signature_check.result) {
-	case 'G':
-		if (ret || (check_trust && signature_check.trust_level < TRUST_MARGINAL))
-			die(_("Commit %s has an untrusted GPG signature, "
-			      "allegedly by %s."), hex, signature_check.signer);
-		break;
-	case 'B':
-		die(_("Commit %s has a bad GPG signature "
-		      "allegedly by %s."), hex, signature_check.signer);
-	default: /* 'N' */
-		die(_("Commit %s does not have a GPG signature."), hex);
-	}
-	if (verbosity >= 0 && signature_check.result == 'G')
-		printf(_("Commit %s has a good GPG signature by %s\n"),
-		       hex, signature_check.signer);
+	ret |= check_trust && signature_check.trust_level < TRUST_MARGINAL;
+	print_signature_buffer(&commit->object.oid, &signature_check, ret,
+			       GPG_VERIFY_SHORT);
+	if (ret)
+		die(_("Signature verification failed"));
 
 	signature_check_clear(&signature_check);
 }
diff --git a/gpg-interface.c b/gpg-interface.c
index fc182d39be..838ece2b37 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -5,6 +5,8 @@ 
 #include "gpg-interface.h"
 #include "sigchain.h"
 #include "tempfile.h"
+#include "object.h"
+#include "object-store.h"
 
 static char *configured_signing_key;
 static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
@@ -333,16 +335,53 @@  int check_signature(const char *payload, size_t plen, const char *signature,
 	return !!status;
 }
 
-void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
+void print_signature_buffer(const struct object_id *oid,
+			    const struct signature_check *sigc, int status,
+			    unsigned flags)
 {
 	const char *output = flags & GPG_VERIFY_RAW ?
 		sigc->gpg_status : sigc->gpg_output;
+	char hex[GIT_MAX_HEXSZ + 1];
+	char *type;
 
 	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
 		fputs(sigc->payload, stdout);
 
 	if (flags & GPG_VERIFY_FULL && output)
 		fputs(output, stderr);
+
+	if (flags & GPG_VERIFY_SHORT && oid) {
+		type = xstrdup_or_null(
+			type_name(oid_object_info(the_repository, oid, NULL)));
+		if (!type)
+			type = xstrdup("object");
+		*type = toupper(*type);
+
+		find_unique_abbrev_r(hex, oid, DEFAULT_ABBREV);
+
+		switch (sigc->result) {
+		case 'G':
+			if (status)
+				error(_("%s %s has an untrusted GPG signature, "
+					"allegedly by %s."),
+				      type, hex, sigc->signer);
+			else
+				printf(_("%s %s has a good GPG signature by %s\n"),
+				       type, hex, sigc->signer);
+			break;
+		case 'N':
+			error(_("%s %s does not have a GPG signature."), type,
+			      hex);
+			break;
+		default:
+			error(_("%s %s has a bad GPG signature "
+				"allegedly by %s."),
+			      type, hex, sigc->signer);
+			break;
+		}
+
+		free(type);
+	}
 }
 
 size_t parse_signature(const char *buf, size_t size)
diff --git a/gpg-interface.h b/gpg-interface.h
index 4631a91330..2f349f8ed5 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -6,6 +6,7 @@  struct strbuf;
 #define GPG_VERIFY_VERBOSE (1 << 0)
 #define GPG_VERIFY_RAW (1 << 1)
 #define GPG_VERIFY_FULL (1 << 2)
+#define GPG_VERIFY_SHORT (1 << 3)
 
 enum signature_trust_level {
 	TRUST_UNDEFINED,
@@ -60,7 +61,8 @@  const char *get_signing_key(void);
 int check_signature(const char *payload, size_t plen,
 		    const char *signature, size_t slen,
 		    struct signature_check *sigc);
-void print_signature_buffer(const struct signature_check *sigc,
+void print_signature_buffer(const struct object_id *oid,
+			    const struct signature_check *sigc, int status,
 			    unsigned flags);
 
 #endif
diff --git a/tag.c b/tag.c
index b8d6da81eb..f6ad4171f9 100644
--- a/tag.c
+++ b/tag.c
@@ -10,7 +10,8 @@ 
 
 const char *tag_type = "tag";
 
-static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+static int run_gpg_verify(const struct object_id *oid, const char *buf,
+			  unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
 	size_t payload_size;
@@ -28,7 +29,7 @@  static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 
 	ret = check_signature(buf, payload_size, buf + payload_size,
 				size - payload_size, &sigc);
-	print_signature_buffer(&sigc, flags);
+	print_signature_buffer(oid, &sigc, ret, flags);
 
 	signature_check_clear(&sigc);
 	return ret;
@@ -57,7 +58,7 @@  int gpg_verify_tag(const struct object_id *oid, const char *name_to_report,
 				name_to_report :
 				find_unique_abbrev(oid, DEFAULT_ABBREV));
 
-	ret = run_gpg_verify(buf, size, flags);
+	ret = run_gpg_verify(oid, buf, size, flags);
 
 	free(buf);
 	return ret;