On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Fabian Stelzer <fs@xxxxxxxxxxxx> 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);
>
> Ahh, OK. Sorry for being ultra lazy for not visiting the actual
> source but just responding after reading only somebody else's
> comments.
I'm also guilty of being lazy and not consulting the actual source. Sorry.
Fabian, thanks for all the extra context information.
> 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?
Indeed, the existing code is confusing me. I've been staring at it for
several minutes and I think I'm still failing to understand the
purpose of the +1 in the strchrnul() call. Perhaps I'm missing
something obvious(?).
> 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--;
It took several seconds for me to convince myself that the -1 array
index was safe. Had the `line < end_of_text` condition been written
`end_of_text > line`, I think it would have been immediately obvious,
but it's subjective, of course.
> /* 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.
Yes, this may be an improvement, though the cognitive load is still
somewhat high. Using one of the `split` functions from strbuf.h or
string-list.h might reduce the cognitive load significantly, even if
this code still needs to handle CR removal manually since none of the
`split` functions are LF/CRLF agnostic. (Adding such a function might
be useful but could be outside the scope of this bug fix patch.)
