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

[Eric Sunshine <sunshine@sunshineco.com>]

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