From: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
Date: Thu, 16 Oct 2025 19:38:11 GMT
Subject: Re: [PATCH] gpg-interface: trim only CR characters that precede LF
Message-ID: <CAFpMFfBe7+pMUL8aaDkGkPUaE9RhCW25OJhJy69EcukgSFn9+A@mail.gmail.com>
In-Reply-To: <xmqq4iry4r3e.fsf@gitster.g>

Hi Junio,
Haha, I smiled at your “teh” comment — I myself often make teh same typo
Thanks a lot for catching the typo and for the detailed feedback on
style and indentation.
I’ll fix the tab/space mix, shorten the long line, and use your
suggested comment wording in the next revision

On Thu, Oct 16, 2025 at 7:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:
>
> >  /*
> > - * Strip CR from the line endings, in case we are on Windows.
> > - * NEEDSWORK: make it trim only CRs before LFs and rename
> > + * Trim CR characters only when they appear before LF (\r\n) line endings.
> > + * This avoids removing legitimate lone CRs from teh content.
>
> "teh" -> "the".  I know, I myself often make teh same typo.
>
> >   */
> > -static void remove_cr_after(struct strbuf *buffer, size_t offset)
> > +static void trim_cr_before_lf(struct strbuf *buffer, size_t offset)
>
> In other words, this normalizes crlf to lf line ending.
>
> >  {
> >       size_t i, j;
> >
> >       for (i = j = offset; i < buffer->len; i++) {
> > -             if (buffer->buf[i] != '\r') {
> > +          /* skip CR only if it comes right before LF */
> > +             if (buffer->buf[i] == '\r' && i + 1 < buffer->len && buffer->buf[i+1] == '\n')
>
> Are two different mixture of tabs and spaces used in the above two
> lines?  I think they wanted to begin at the same column.
>
> Also, the second line is overly long that it does not even fit on my
> 92-column wide terminal (yes, 80 is the limit, but this will let a
> line in the patches quoted a few times to still fit, as long as the
> patch honors the 80-column limit).
>
> > +                 continue;
>
> >                       if (i != j)
> >                               buffer->buf[j] = buffer->buf[i];
> >                       j++;
> > -             }
> > +
>
> Do we need a blank line here?  I dunno.
>
> >       }
> >       strbuf_setlen(buffer, j);
> >  }
> > @@ -1023,8 +1026,10 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
> >       }
> >       strbuf_release(&gpg_status);
> >
> > -     /* Strip CR from the line endings, in case we are on Windows. */
> > -     remove_cr_after(signature, bottom);
> > +     /* Trim carriage returns (CR) only when they appear before line feeds (LF),.
> > +     *  mainly for handling Windows-style line endings
> > +     */
>
>         /* Convert CRLF to LF, in case we are on Windows */
>
> > +     trim_cr_before_lf(signature, bottom);
> >
> >       return 0;
> >  }
> > @@ -1110,8 +1115,10 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> >                       ssh_signature_filename.buf);
> >               goto out;
> >       }
> > -     /* Strip CR from the line endings, in case we are on Windows. */
> > -     remove_cr_after(signature, bottom);
> > +     /* Trim carriage returns (CR) only when they appear before line feeds (LF),
> > +     *  mainly for handling Windows-style line endings.
> > +     */
> > +     trim_cr_before_lf(signature, bottom);
>
> Ditto.
>
> >
> >  out:
> >       if (key_file)