Eric Sunshine <sunsh...@sunshineco.com> writes:
> Thanks for the re-roll...
>
> On Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera <jerzy.koz...@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.
