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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2022, 20:40 UTC
Message-ID: <xmqqsfu1hq6x.fsf@gitster.g>
In-Reply-To: <20220105103611.upfmcrudw6n3ymx6@fs>

Fabian Stelzer <fs@gigacodes.de> writes:

Show 25 quoted lines

> 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.

Show 13 quoted lines

>
> 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 ?