On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > >> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@xxxxxxxxxxxx> wrote: > >> >> - trust_size = strcspn(line, "\n"); > >> >> + 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 */ > >> > > >> > I may be misunderstanding, but isn't the strlen() unnecessary? > >> > > >> > if (trust_size && line[trust_size] && > >> > line[trust_size - 1] == '\r') > >> > trust_size--; > >> > >> That changes behaviour when "line" has more than one lines in it. > >> strcspn() finds the first LF, and the posted patch ignores CRLF not > >> at the end of line[]. Your variant feels more correct if the > >> objective is to find the end of the first line (regardless of the > >> choice of the end-of-line convention, either LF or CRLF) and omit > >> the line terminator. > > > > Okay, that makes sense if that's the intention of the patch. Perhaps > > the commit message should mention that `line` might contain multiple > > lines and that it's only interested in the very last LF (unless it's > > already obvious to everyone else, even though it wasn't to me). > > I do not think that is the case. strcspn(line, "\n") will stop at > the first one, so unless it is guaranteed that "line" has only one > line in it, the patch as posted is not correct. Your variant > without strlen() feels more correct, as I said. Okay, sorry for my unclear thinking. The existing code (before this patch) does indeed seem to be interested only in the first line of `line`, in which case I agree that the patch's use of strlen() does not appear to be correct if `line` could ever contain more than one line.
