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

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: DOAN Tran Cong Danh <congdanhqx@gmail.com>,
	git@vger.kernel.org, animi.vulpis@gmail.com, j6t@kdbg.org,
	Karthik Nayak <karthik.188@gmail.com>
Subject: Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos
Date: Tue, 23 May 2017 11:01:15 +0900	[thread overview]
Message-ID: <xmqq37bwnxg4.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170522201212.uuas26n6npdebsxg@sigill.intra.peff.net> (Jeff King's message of "Mon, 22 May 2017 16:12:12 -0400")

Jeff King <peff@peff.net> writes:

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

I actually think the entire codeflow of "find positions and length
of threeparts" using find_subpos() and then "copy the length bytes
starting position for C_{SUB,BODY,SIG,LINES,...}" must be rethought,
if the behavior of pretty.c::pretty_print_commit() is to be matched.
With the current code, %(contents:body) and other atoms that are
handled in ref-filter.c::grab_sub_body_contents() keep trailing
whitespaces on their lines with the current code that copies length
bytes starting the position using xmemdupz().  There need to be some
code that loses these trailing whiltespaces in the copied result.

While I do not claim that refactoring and reusing code from pretty.c
is the only viable way forward, it is clear to me that a patch that
updates find_subpos() and changes nothing else falls short X-<.