On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano [off-list ref] wrote:

Fabian Stelzer [off-list ref] writes:

quoted

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