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

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2017, 01:19 UTC
Message-ID: <xmqqy3tppu13.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170521134209.25659-1-congdanhqx@gmail.com>

DOAN Tran Cong Danh <congdanhqx@gmail.com> writes:

> Starting from commit 949af06 (branch: use ref-filter printing APIs, 2017-01-10),
> `git branch -v` doesn't treat CRLF as line separator anymore.

A seemingly good problem identification (but not quite; see below) ...

Show 16 quoted lines

>
> Quote from git mailing-list:
>
>> Here is a recipe to reproduce the error:
>>
>>    git init
>>    git commit --allow-empty -m initial
>>    git branch crlf $(printf '%s\r\n' subject '' line3_long line4 |
>>        git commit-tree HEAD:)
>> The reason for the "bug" is obviously that a line having CR in addition
>> to LF is not "an empty line". Consequently, the second line is not
>> treated as a separator between subject and body, whereupon Git
>> concatenates all line into one large subject line. This strips the LFs
>> but leaves the CRS in tact, which, when printed on a terminal move the
>> cursor to the beginning of the line, so that text after the CRs
>> overwrites what is already in the terminal.

... and good analysis.

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.

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.

Here is your test addition:

Show 12 quoted lines

> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index 5778c0afe..29b392066 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -13,7 +13,8 @@ test_expect_success 'make commits' '
>  
>  test_expect_success 'make branches' '
>  	git branch branch-one &&
> -	git branch branch-two HEAD^
> +	git branch branch-two $(printf "%s\r\n" one "" line3_long line4 |
> +	     git commit-tree HEAD:)
>  '

If you change this test to

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