Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Nov 12, 2017, 17:32 UTC
- Message-ID
- <CAPig+cSR_GGog2vATwGgMWDofVzkgDE7K=VvZ4GEyrzYC1t-GQ@mail.gmail.com>
- In-Reply-To
- <20171112130710.16000-1-jerzy.kozera@gmail.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...
Show 8 quoted lines
> 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.
Show 5 quoted lines
> + 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.
Show 46 quoted lines
> + 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