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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2017, 02:08 UTC
Message-ID: <xmqqk1yvaq2e.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cSR_GGog2vATwGgMWDofVzkgDE7K=VvZ4GEyrzYC1t-GQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 15 quoted lines

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

Show 9 quoted lines

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

Show 17 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.
>
>> +                       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.