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

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

Fabian Stelzer <fs@gigacodes.de> writes:

> I guess we need a bit more context for this patch to make sense:
>
> for (line = ssh_principals_out.buf; *line;
>      line = strchrnul(line + 1, '\n')) {
> 	while (*line == '\n')
> 		line++;
> 	if (!*line)
> 		break;
>
> 	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 */
> 	principal = xmemdupz(line, trust_size);
>
> ssh_principals_out contains the result of the find-principals call
> which contains one found principal per line (normally LF, CRLF in some
> cygwin setup).

Ahh, OK.  Sorry for being ultra lazy for not visiting the actual
source but just responding after reading only somebody else's
comments.

So, the code skips over one or more LFs (but users of platforms that
use CRLF line termination are screwed here already) to find the
beginning of a non-empty line.  Then it wants to find the end of
that non-empty line (if there is still LF there in the buffer).
Since strcspn() may not find any LF (i.e. it is an incomplete line),
strlen(line) is used to see if we found a LF or if we hit the
terminating NUL.  If the line ended with CR, we do not want to strip
it.

OK, so I was completely missing the idea.  And I agree that it may
be a good idea to check how strcspn() returned to deal with an
incomplete line, although as you hint later in the message I am
responding to, checking line[trust_size] would be a more obvious
implementation.

In any case, I think the earlier part of the loop is more confusing,
and I think fixing that would naturally fix the trust_size
computation.  For example, wouldn't this easier to grok?

	const char *next;

	for (line = ssh_principals_out.buf;
	     *line;
	     line = next) {
		const char *end_of_text;

                /* Find the terminating LF */
               	next = end_of_text = strchrnul(line, '\n');

		/* Did we find a LF, and did we have CR before it? */
		if (*end_of_text &&
                    line < end_of_text &&
		    end_of_text[-1] == '\r')
			end_of_text--;

		/* Unless we hit NUL, skip over the LF we found */
		if (*next)
			next++;

		/* Not all lines are data.  Skip empty ones */
		if (line == end_of_text)
			/* 
                         * You may want to allow skipping more than just
			 * lines with 0-byte on them (e.g. comments?)
			 * depending on the format you are reading.
			 */
			continue;

		/* We now know we have an non-empty line. Process it */
		principal = xmemdupz(line, end_of_text - line);
		...
	}
		
The idea is to make sure that the place where the line ending
convention is taken care of is very isolated at the beginning of the
loop.

Hmm?