Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos

On 05/22/2017 09:19 AM, Junio C Hamano wrote:

If you look at how `git branch -v` before that problematic change
removed the extra CR, you would notice that pretty_print_commit()
was used for that, which eventually called format_subject() with
"one\r\n\r\nline3...", got one line "one\r\n" by calling
get_one_line(), adjusted the line length from 5 to 3 by calling
is_blank_line() which as a side effect trims all whitespaces (not
just LF and CR), and emitted "one".  The reason why the next \r\n
was not mistaken as a non-empty line is the same---is_blank_line()
call onthe next line said that "\r\n" is an all-white space line.

So, while I think the realization that we have a problem, and the
analysis above i.e. "emptiness of the line matters", are both good,
but I do not think this is suffucient to get back the old behaviour.

The thing is, we never treated CRLF as line separator in this code.
What we did was to treat LF as line separator, but trimmed trailing
bytes that are isspace().  That is what the analysis you quoted from
J6t says.

If I understand correctly, we should trim all whitespaces at the end of
subject line, and we should treat lines contain only whitespaces as
empty lines, right?

Here is your test addition:
If you change this test to

quoted

+	git branch branch-two $(printf "%s\n" one " " line3_long line4 |
+	     git commit-tree HEAD:)

then the old code before the problematic change will only show the
first line i.e. "one" in "branch -v" output.  I think with or
without your code change, the new code would still show line3_long
and line4 in the output.

Agree!

If you could confirm my understand, I would happily provide new patch
to trim all trailing whitespaces at the end of subject line
and treat next all-whitespace-line as empty line.