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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2022, 19:33 UTC
Message-ID: <xmqqo84rcn3j.fsf@gitster.g>
In-Reply-To: <20220104125534.wznwbkyxfcmyfqhb@fs>

Fabian Stelzer <fs@gigacodes.de> writes:

Show 18 quoted lines

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