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

[Junio C Hamano <gitster@pobox.com>]

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:
>> >> We need to trim \r from the output of 'ssh-keygen -Y find-principals' on
>> >> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer
>> >> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms
>> >> this hypothesis. Signature verification passes with the fix.
>> >> ---
>> >> -                       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.