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

[Fabian Stelzer <fs@gigacodes.de>]

On 05.01.2022 12:40, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>> How about something like this:
>>
>> int string_find_line(char **line, size_t *len) {
>> 	const char *eol = NULL;
>>
>> 	if (*len > 0) {
>> 		*line = *line + *len;
>> 		if (**line && **line == '\r')
>> 			(*line)++;
>> 		if (**line && **line == '\n')
>> 			(*line)++;
>> 	}
>>
>> 	if (!**line)
>> 		return 0;
>>
>> 	eol = strchrnul(*line, '\n');
>>
>> 	/* Trim trailing CR from length */
>> 	if (eol > *line && eol[-1] == '\r')
>> 		eol--;
>>
>> 	*len = eol - *line;
>> 	return 1;
>> }
>
>It is a confusing piece of "we handle one line at a time" helper.
>It is not obvious what the loop invariants are.
>
>It would be most natural to readers if *line points at the very
>beginning of the buffer, i.e. the beginning of the first line,
>and *len points at the very first character of that line, i.e. 0.
>
>But then the first thing this function worries about is a case where
>*len is not 0.  I obviously am biased, but sorry, I find what I gave
>you 100 times simpler to understand.
>

There are a few more places where the same thing happens and text is just 
split by LF, ignoring CR. The gpg parsing where this code originated being 
the most prominent example. However those just parse some parts from the 
output and the worst that seems to happen is a trailing CR in some log 
outputs.
If we are ok with this then your version is indeed the better one. If we 
want to correct the parsing at the other sites then I think a more 
generalized function would be better. Since the gpg stuff is in place for a 
long time and no one complained we can probably leave it as is. I'll prepare 
a new patch.

Thanks