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

On Mon, May 22, 2017 at 10:19:52AM +0900, Junio C Hamano wrote:

However.

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.

I noticed a similar thing regarding pretty_print_commit(). Which really
made me wonder whether the right solution is to drop the custom parsing
code in ref-filter.c and use the bits from pretty.c. Then we'd have only
one parser. That's less code, but more importantly, there can't be
inconsistencies between the two (we're fixing one now, but we have no
idea if there are others).

I suspect that's more work because we'd need to refactor pretty.c a bit
to make the right functionality available. But the end result would be
much more maintainable.

-Peff

PS I'm also a bit curious how a CRLF got into a commit message in the
   first place. Stripspace should be removing that for normal "git
   commit" runs. I don't know that we've ever said it explicitly, but I
   think we'd consider the canonical in-repo object representation to be
   LF-only (just like we do for smudge/clean filters). Which means that
   whatever is generating these commit objects is arguably buggy.