threads / patch / 47170

patchgpg-interface: Strip CR chars for Windows gpg2

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

## tl;dr

4 messages between Nov 11, 2017 and Nov 12, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jerzy Kozera· Nov 11, 2017, 16:06 UTC · lore

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(-)

Show changes to gpg-interface.c +16 −9

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

Junio C Hamano· Nov 12, 2017, 01:51 UTC · re: Jerzy Kozera · lore

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

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

Show 12 quoted lines

> +/* 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.

Junio C Hamano· Nov 12, 2017, 01:53 UTC · re: Jerzy Kozera · lore

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

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).

Junio C Hamano· Nov 12, 2017, 05:58 UTC · re: Jerzy Kozera · lore

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

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.