Re: [PATCH v2 0/3] Teach ref-filter API to correctly handle CRLF in messages

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

To be honest, I suspect that it is not a bug in the parser that parsed out %(contents:subject), but a user error that left the log message in CRLF endings ;-).

So "correctly handle CRLF" is probably a tad unfair to those who wrote the current ref-filter code; a description that is more fair to them is probably along the lines of "handle malformed log messages more gracefully", I would think.

I am not sure where you want to go with this. Whether it is shown in the ^X notation (and some terminals even reverse color to highlight them), or it is shown literally (i.e. causing the next byte to overwrite the same line starting from the left-edge), you would be annoyed either way, no? I suspect that the latter would annoy you even more. Isn't what "most terminal emulators" do, i.e. to show it in the ^X notation instead of emitting it literally, a good thing? IOW, "resulting in ..." is not correctly telling us what you think is wrong---you don't have to blame terminals.

It is not limited to CR, and is not limited to control characters at the end of the lines, no? If you had "\a" (or "\r") in the middle of the title, either the current or the old code would ring a bell (or cause the next character to appear at the end of the same line) or when piped to "less" you'd see "^G" (or "^M") in the liddle of the line.

The old code used pretty.c::pretty_print_commit() mechanism; pretty.c::format_subject() uses pretty.c::is_blank_line() to trim whitespaces at the right end while trying to notice where the first paragraph break is, so any whitespace at the end of first paragraph break is removed, and each end of line got replaced by a SP, but it did not do anything special to control characters in the middle of the lines (and it didn't do anything to the control characters in the middle of the line, either). So while the old code happened to cleanse CR at the end of the lines, it wasn't doing enough.

I think fixing _that_ is (and should be) outside the scope of this series, of course.

API is not changed (i.e. the callers do not have to do anything special); only the implementation.

	ref-filter: handle CR at the end of the lines more gracefully

perhaps?

In other words...

	When a commit or a tag object uses CRLF line endings, the
	ref-filter machinery does not identify the end of the first
	paragraph as intended by the writer, because it only looks
	for two consecutive LFs and CR-LF-CR-LF does not look like a
	blank line that separates paragraphs to it.  "git branch",
	"git tag" and "git for-each-ref" all rely on the messages
	split correctly into "%(contents:subject)" and
	"%(contents:body)" placeholders and ends up showing
	everything as the subject.

Now based on what I hinted in the far-above part, there can be two valid solutions here.

 * recognize CRLF as a valid line ending, but still retain ^M in the
   message.  The replacement for "%(contents:subject)" would still
   end with "^M", and we add LF to it, which makes the resulting
   output end with CRLF and all is well.  This will keep "\a" and
   "\r" in the middle of the line in the output.

 * strip CR and any control character other than LF from everywhere.
   This will cleanse "\a" and "\r" in the middle of, or anywhere on,
   the line, so that "%(contents:subject)", "%(contents:body)" and
   "%(contents)" all are "clean".

I am not offhand sure which one is better (I haven't read the patch to see which one you chose to implement).

The above few lines may need revising (based on what I said to the cover); --- even if they don't, CRFL here needs to become CRLF ;-)

Thanks for working on this.