[PATCH] gpg-interface: Strip CR chars for Windows gpg2

[PATCH] gpg-interface: Strip CR chars for Windows gpg2

[Jerzy Kozera]

This fixes the issue with newlines reported at
https://github.com/git-for-windows/git/issues/1249

Issues with non-ASCII characters remain for further investigation.

Signed-off-by: Jerzy Kozera <jerzy.kozera@gmail.com>
---
The patch applies cleanly on top of maint as well as master.
 gpg-interface.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4feacf16e5..a92fb2f3b9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -145,6 +145,18 @@ const char *get_signing_key(void)
 	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }
 
+/* Strip CR from the line endings, in case we are on Windows. */
+static void strip_cr(struct strbuf *buffer, size_t bottom) {
+	size_t i, j;
+	for (i = j = bottom; i < buffer->len; i++)
+		if (buffer->buf[i] != '\r') {
+			if (i != j)
+				buffer->buf[j] = buffer->buf[i];
+			j++;
+		}
+	strbuf_setlen(buffer, j);
+}
+
 /*
  * Create a detached signature for the contents of "buffer" and append
  * it after "signature"; "buffer" and "signature" can be the same
@@ -155,7 +167,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
-	size_t i, j, bottom;
+	size_t bottom;
 	struct strbuf gpg_status = STRBUF_INIT;
 
 	argv_array_pushl(&gpg.args,
@@ -180,14 +192,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	if (ret)
 		return error(_("gpg failed to sign the data"));
 
-	/* Strip CR from the line endings, in case we are on Windows. */
-	for (i = j = bottom; i < signature->len; i++)
-		if (signature->buf[i] != '\r') {
-			if (i != j)
-				signature->buf[j] = signature->buf[i];
-			j++;
-		}
-	strbuf_setlen(signature, j);
+	strip_cr(signature, bottom);
 
 	return 0;
 }
@@ -230,6 +235,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	sigchain_push(SIGPIPE, SIG_IGN);
 	ret = pipe_command(&gpg, payload, payload_size,
 			   gpg_status, 0, gpg_output, 0);
+	strip_cr(gpg_status, 0);
+	strip_cr(gpg_output, 0);
 	sigchain_pop(SIGPIPE);
 
 	delete_tempfile(&temp);
-- 
2.14.2.windows.3

Re: [PATCH] gpg-interface: Strip CR chars for Windows gpg2

[Junio C Hamano]

Jerzy Kozera <jerzy.kozera@gmail.com> writes:

> +/* Strip CR from the line endings, in case we are on Windows. */
> +static void strip_cr(struct strbuf *buffer, size_t bottom) {
> +	size_t i, j;
> +	for (i = j = bottom; i < buffer->len; i++)
> +		if (buffer->buf[i] != '\r') {
> +			if (i != j)
> +				buffer->buf[j] = buffer->buf[i];
> +			j++;
> +		}
> +	strbuf_setlen(buffer, j);
> +}
> +

While the approach of turning CRLF into LF as a workaround is a good
one, I do not see this loop doing that; instead it is removing CR
anywhere on the line unconditionally.  

It might not make a difference in practice to rely on the assumption
that nobody sane would place a lone CR in the middle of a line, but
it feels dirty.

Because I know this was lifted from an existing code, I think it may
be better to take this patch as-is, but the code screams loudly that
it wants a clean-up patch to fix that immediately on top of it.

Thanks.

Re: [PATCH] gpg-interface: Strip CR chars for Windows gpg2

[Junio C Hamano]

Jerzy Kozera <jerzy.kozera@gmail.com> writes:

One minor detail I missed:

> Subject: Re: [PATCH] gpg-interface: Strip CR chars for Windows gpg2

Please downcase "Strip" (see "git shortlog --no-merges" for recent
history to notice that it would make this commit sit better with the
existing ones).

Re: [PATCH] gpg-interface: Strip CR chars for Windows gpg2

[Junio C Hamano]

Jerzy Kozera <jerzy.kozera@gmail.com> writes:

> This fixes the issue with newlines reported at
> https://github.com/git-for-windows/git/issues/1249

Do not omit the description of what you wanted to do completely and
force readers of "git log" to visit an external website.  You are
already bothering to write "issue with newlines"; it shouldn't be
too much to complete the description by saying what issues around
newlines you are solving, before saying "reported at".

Thanks.