Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> >> >> -                       trust_size = strcspn(line, "\n");
> >> >> +                       trust_size = strcspn(line, "\n"); /* truncate at LF */
> >> >> +                       if (trust_size && trust_size != strlen(line) &&
> >> >> +                           line[trust_size - 1] == '\r')
> >> >> +                               trust_size--; /* the LF was part of CRLF at the end */
> >> >
> >> > I may be misunderstanding, but isn't the strlen() unnecessary?
> >> >
> >> >     if (trust_size && line[trust_size] &&
> >> >         line[trust_size - 1] == '\r')
> >> >             trust_size--;
> >>
> >> That changes behaviour when "line" has more than one lines in it.
> >> strcspn() finds the first LF, and the posted patch ignores CRLF not
> >> at the end of line[].  Your variant feels more correct if the
> >> objective is to find the end of the first line (regardless of the
> >> choice of the end-of-line convention, either LF or CRLF) and omit
> >> the line terminator.
> >
> > Okay, that makes sense if that's the intention of the patch. Perhaps
> > the commit message should mention that `line` might contain multiple
> > lines and that it's only interested in the very last LF (unless it's
> > already obvious to everyone else, even though it wasn't to me).
>
> I do not think that is the case.  strcspn(line, "\n") will stop at
> the first one, so unless it is guaranteed that "line" has only one
> line in it, the patch as posted is not correct.  Your variant
> without strlen() feels more correct, as I said.

Okay, sorry for my unclear thinking. The existing code (before this
patch) does indeed seem to be interested only in the first line of
`line`, in which case I agree that the patch's use of strlen() does
not appear to be correct if `line` could ever contain more than one
line.