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

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

[Jerzy Kozera]

This fixes the issue with newlines being \r\n and not being displayed
correctly when using gpg2 for Windows, as 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>
---

Addressed comments by Junio C Hamano (check for following \n, and
updated the commit description).

 gpg-interface.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4feacf16e5..ab592af7f2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -145,6 +145,20 @@ const char *get_signing_key(void)
 	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }
 
+/* Strip CR from the CRLF 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 (!(i < buffer->len - 1 &&
+				buffer->buf[i] == '\r' &&
+				buffer->buf[i + 1] == '\n')) {
+			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 +169,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 +194,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 +237,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 v2] gpg-interface: strip CR chars for Windows gpg2

[Eric Sunshine]

Thanks for the re-roll...

On Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera <jerzy.kozera@gmail.com> wrote:
> This fixes the issue with newlines being \r\n and not being displayed
> correctly when using gpg2 for Windows, as reported at
> https://github.com/git-for-windows/git/issues/1249

It's still not clear from this description what "not being displayed
correctly" means. Ideally, the commit message should stand on its own,
explaining exactly what problem the patch is solving, without the
reader having to chase URLs to pages (which might disappear). If you
could summarize the problem and solution in your own words in such a
way that your description itself conveys enough information for
someone not familiar with that problem report to understand the
problem, then that would likely make a good commit message.

More below...

> Issues with non-ASCII characters remain for further investigation.
>
> Signed-off-by: Jerzy Kozera <jerzy.kozera@gmail.com>
> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -145,6 +145,20 @@ const char *get_signing_key(void)
> +/* Strip CR from the CRLF line endings, in case we are on Windows. */
> +static void strip_cr(struct strbuf *buffer, size_t bottom) {

It's not at all clear what 'bottom' means. In the original, when the
code was inline, the surrounding context would likely have given a
good clue to the meaning of 'bottom', but here stand-alone, it conveys
little or nothing. Perhaps a better name for this argument would be
'start_at' or 'from' or something.

> +       size_t i, j;
> +       for (i = j = bottom; i < buffer->len; i++)
> +               if (!(i < buffer->len - 1 &&
> +                               buffer->buf[i] == '\r' &&
> +                               buffer->buf[i + 1] == '\n')) {

Hmm, was this tested? If I'm reading this correctly, this strips out
the entire CRLF pair, whereas the original code only stripped the CR
and left what followed it (typically LF) alone. Junio's suggestion was
to enhance this to be more careful and strip CR only when followed
immediately by LF (but to leave the LF intact). Therefore, this seems
like a regression.

> +                       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 +169,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 +194,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 +237,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 v2] gpg-interface: strip CR chars for Windows gpg2

[Torsten Bögershausen]

On Sun, Nov 12, 2017 at 01:07:10PM +0000, Jerzy Kozera wrote:
> This fixes the issue with newlines being \r\n and not being displayed
> correctly when using gpg2 for Windows, as 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>
> ---
> 
> Addressed comments by Junio C Hamano (check for following \n, and
> updated the commit description).
> 
>  gpg-interface.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 4feacf16e5..ab592af7f2 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -145,6 +145,20 @@ const char *get_signing_key(void)
>  	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
>  }
>  
> +/* Strip CR from the CRLF line endings, in case we are on Windows. */
> +static void strip_cr(struct strbuf *buffer, size_t bottom) {
> +	size_t i, j;

It is not wrong to say "Strip CR from the CRLF".
In Git we often talk about "convert CRLF into LF",

The comment somewhat different to the function name.
The function namd and the name of the parameters can be more in
in line with existing strbuf functions:
(And the opening '{' should go into it's own line:

static void convert_crlf_to_lf(struct strbuf *sb, size_t len)
{
	size_t i, j;

An even more generic approach (could be done in a seperate commit)
would be to move the whole function into strbuf.c/strbuf.h,
and it may be called like this.

void strbuf_crlf_to_lf(struct strbuf *sb, size_t len)
{
  /* I would even avoid "i" and "j", and use src and dst or so) */
  size_t src_pos, dst_idx;
}

Thanks for working on this.

[]

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for the re-roll...
>
> On Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera <jerzy.kozera@gmail.com> wrote:
>> This fixes the issue with newlines being \r\n and not being displayed
>> correctly when using gpg2 for Windows, as reported at
>> https://github.com/git-for-windows/git/issues/1249
>
> It's still not clear from this description what "not being displayed
> correctly" means. Ideally, the commit message should stand on its own,
> explaining exactly what problem the patch is solving, without the
> reader having to chase URLs to pages (which might disappear). If you
> could summarize the problem and solution in your own words in such a
> way that your description itself conveys enough information for
> someone not familiar with that problem report to understand the
> problem, then that would likely make a good commit message.

Thanks.  I was wondering if I am the only one who does not
understand what the revised wording wants to say.

>> @@ -145,6 +145,20 @@ const char *get_signing_key(void)
>> +/* Strip CR from the CRLF line endings, in case we are on Windows. */
>> +static void strip_cr(struct strbuf *buffer, size_t bottom) {
>
> It's not at all clear what 'bottom' means. In the original, when the
> code was inline, the surrounding context would likely have given a
> good clue to the meaning of 'bottom', but here stand-alone, it conveys
> little or nothing. Perhaps a better name for this argument would be
> 'start_at' or 'from' or something.

I personally do not mind 'bottom' (especially when it appears in
contrast to 'top') too much, but start_at would be much clearer.

>> +       size_t i, j;
>> +       for (i = j = bottom; i < buffer->len; i++)
>> +               if (!(i < buffer->len - 1 &&
>> +                               buffer->buf[i] == '\r' &&
>> +                               buffer->buf[i + 1] == '\n')) {
>
> Hmm, was this tested? If I'm reading this correctly, this strips out
> the entire CRLF pair, whereas the original code only stripped the CR
> and left what followed it (typically LF) alone. Junio's suggestion was
> to enhance this to be more careful and strip CR only when followed
> immediately by LF (but to leave the LF intact). Therefore, this seems
> like a regression.
>
>> +                       if (i != j)
>> +                               buffer->buf[j] = buffer->buf[i];
>> +                       j++;
>> +               }

I think the "negate the entire thing" condition confuses the
readers.  The negated condition is "Do we still have enough bytes to
see if we are looking at CRLF?  Are we at CR?  Is the one byte
beyond what we have a LF?  Do all of these three conditions hold
true?"  If not, i.e. for all the bytes on the line except for that
narrow "we are at CR of a CRLF sequence" case, the body of the loop
makes a literal copy and advances the destination pointer 'j'.  The
only thing that is skipped is CR that comes at the beginning of a
CRLF sequence.

So I think the loop does what it wants to do.

In any case, I think this should be a two patch series---one with
the code as-is with a better explanation, but without "make sure
only CR in CRLF and no other CR are stripped" improvement, and the
other as a follow-up to it to make the improvement.

Thanks.

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

[Eric Sunshine]

On Sun, Nov 12, 2017 at 9:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> +       size_t i, j;
>>> +       for (i = j = bottom; i < buffer->len; i++)
>>> +               if (!(i < buffer->len - 1 &&
>>> +                               buffer->buf[i] == '\r' &&
>>> +                               buffer->buf[i + 1] == '\n')) {
>>
>> Hmm, was this tested? If I'm reading this correctly, this strips out
>> the entire CRLF pair, whereas the original code only stripped the CR
>> and left what followed it (typically LF) alone. Junio's suggestion was
>> to enhance this to be more careful and strip CR only when followed
>> immediately by LF (but to leave the LF intact). Therefore, this seems
>> like a regression.
>>
>>> +                       if (i != j)
>>> +                               buffer->buf[j] = buffer->buf[i];
>>> +                       j++;
>>> +               }
>
> I think the "negate the entire thing" condition confuses the
> readers.  The negated condition is "Do we still have enough bytes to
> see if we are looking at CRLF?  Are we at CR?  Is the one byte
> beyond what we have a LF?  Do all of these three conditions hold
> true?"  If not, i.e. for all the bytes on the line except for that
> narrow "we are at CR of a CRLF sequence" case, the body of the loop
> makes a literal copy and advances the destination pointer 'j'.  The
> only thing that is skipped is CR that comes at the beginning of a
> CRLF sequence.
>
> So I think the loop does what it wants to do.

You're right. I misunderstood it. Rephrasing the logic in a simpler
fashion would help.

> In any case, I think this should be a two patch series---one with
> the code as-is with a better explanation, but without "make sure
> only CR in CRLF and no other CR are stripped" improvement, and the
> other as a follow-up to it to make the improvement.

Agreed, a 2-patch series makes sense.