Re: [PATCH] gpg-interface: fix for gpgsm v2.3

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2022, 18:55 UTC
Message-ID: <xmqq7dabvkze.fsf@gitster.g>
In-Reply-To: <20220203123724.47529-1-fs@gigacodes.de>

Fabian Stelzer <fs@gigacodes.de> writes:

Show 9 quoted lines

> gpgsm v2.3 changed some details about its output:
>  - instead of displaying `fingerprint:` for keys it will print `sha1
>    fpr:` and `sha2 fpr:`
>  - some wording of errors has changed
>  - signing will omit an extra debug output line before the [GNUPG]: tag
>
> This change adjusts the gpgsm test prerequisite to work with v2.3 as
> well by accepting `sha1 fpr:` as well as `fingerprint:` and allows both
> variants of errors for unknown certs.

OK, so the change is meant to add support for the new behaviour without deprecating/removing the support for the older one. Good.

> Checking for successful signature creation will omit the leading NL in
> its search string.

I think this is to adjust for "will omit an extra debug output"; as long as we still ensure that the "[GNUPG:] SIG_CREATED" comes at the beginning of a line with some other means, I think that is a good change.

Show 7 quoted lines

> I am not a user of gpgsm but noticed that the GPGSM test prereq was disabled 
> on my runs so i investigated. The `fix` seems rather trivial and I tried to 
> test this as thorough as possible. I ran the test suite on machines 
> available to me (fedora35, centos8) and did a full CI run on github without 
> any issues.
> But if you actually use gpgsm with git please give this a go and let me know 
> if I missed anything.

Yup, thanks for a call for help. I am not gpgsm user either and testing by actual users is very much appreciated.

Show 6 quoted lines

> @@ -939,7 +939,7 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
>  			   signature, 1024, &gpg_status, 0);
>  	sigchain_pop(SIGPIPE);
>  
> -	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
> +	ret |= !strstr(gpg_status.buf, "[GNUPG:] SIG_CREATED ");

This I am not sure about. I understand that the intention is to allow this at the beginning of gpg_status.buf, but not to allow the substring appear in the middle of an otherwise unrelated line. I am afraid that the new check is a bit too lose for that.

Totally untested but just to illustrate the idea...

 gpg-interface.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git c/gpg-interface.c w/gpg-interface.c
index b52eb0e2e0..4238e60dfa 100644
--- c/gpg-interface.c
+++ w/gpg-interface.c
@@ -920,6 +920,7 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
 	size_t bottom;
+	const char *cp;
 	struct strbuf gpg_status = STRBUF_INIT;
 
 	strvec_pushl(&gpg.args,
@@ -939,7 +940,13 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 			   signature, 1024, &gpg_status, 0);
 	sigchain_pop(SIGPIPE);
 
-	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
+	for (cp = gpg_status.buf;
+	     cp && (cp = strstr(cp, "[GNUPG:] SIG_CREATED "));
+	     cp++) {
+		if (cp == gpg_status.buf || cp[-1] == '\n')
+			break; /* found */
+	}
+	ret |= !cp;
 	strbuf_release(&gpg_status);
 	if (ret)
 		return error(_("gpg failed to sign the data"));