From: Fabian Stelzer <fs@gigacodes.de>
Date: Mon, 10 Jan 2022 12:28:59 GMT
Subject: Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen
Message-ID: <20220110122859.3x37a7xdbmcsmuvm@fs>
In-Reply-To: <CAPig+cQMP_Ppg6uywAcFhaVqSoa71dD6UjXbUtC-bvK0WzJnZA@mail.gmail.com>

On 09.01.2022 15:49, Eric Sunshine wrote:
>On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> On 05.01.2022 02:09, Eric Sunshine wrote:
>> >> >      line = strchrnul(line + 1, '\n')) {
>> >> >       while (*line == '\n')
>> >> >               line++;
>> >> >       if (!*line)
>> >> >               break;
>> >
>> >Indeed, the existing code is confusing me. I've been staring at it for
>> >several minutes and I think I'm still failing to understand the
>> >purpose of the +1 in the strchrnul() call. Perhaps I'm missing
>> >something obvious(?).
>>
>> This whole loop was basically copied from parse_gpg_output() above. Without
>> the +1 this would always find the same line in the buffer. The +1 skips over
>> the previously found LF.
>
>I still don't see the point of +1 in the strchrnul() call. After:
>
>    line = strchrnul(line + 1, '\n'))
>
>`line` is going to point either at '\n' or at NUL. Then:
>
>    while (*line == '\n')
>        line++;
>
>skips over the '\n' if present. So, by the time the next loop
>iteration starts, `line` will already be pointing past the '\n' we
>just found, thus the +1 seems pointless (and maybe even buggy).
>
>But perhaps I have a blind spot and am missing something obvious...

Hm, yeah. I think you are correct. The while below should make the +1 
unnecessary. I think this never mattered to parse_gpg_output() since it is 
only looking for the [GNUPG:] status line which probably comes first anyway.  
If it does not then I think the loop will skip over it. Same thing with ssh 
(but we are changing this whole loop anyway)