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

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

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.

>
> Its use would then simply be:
>
> char *line = strbuf.buf;
> size_t len = 0;
> while(string_find_line(&line,&len)) {
> 	if (!len)
> 		continue; /* Skip over empty lines */
> 	principal = xmemdupz(line, len);
> }
>
> Not sure about the name though.
> Maybe string_find_line() / _iterate_line / foreach_line ?