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

From: Jeff King <peff@peff.net>
Date: May 22, 2017, 20:12 UTC
Message-ID: <20170522201212.uuas26n6npdebsxg@sigill.intra.peff.net>
In-Reply-To: <xmqqy3tppu13.fsf@gitster.mtv.corp.google.com>

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

Show 11 quoted lines

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