Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Me again,

On 2015-10-09 11:50, Johannes Schindelin wrote:
> 
> On 2015-10-09 03:40, Paul Tan wrote:
>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>>
>>>> Brendan Forster noticed that we no longer see the helpful message after
>>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls
>>>> the recursive merge function directly, not via a separate process.
>>>>
>>>> But that function was not really safe to be called that way, as it
>>>> die()s pretty liberally.
>>
>> I'm not too familiar with the merge-recursive.c code, but I was under
>> the impression that it only called die() under fatal conditions. In
>> common use cases, such as merge conflicts, it just errors out and the
>> helpful error message does get printed. Is there a reproduction recipe
>> for this?
> 
> Yes. Sorry, I should have added that as part of the patch series.
> Actually, I should have written it *before* making those patches.
> Because it revealed that the underlying problem is completely
> different: *Normally* you are correct, if `pull --rebase` fails with a
> merge conflict, the advice is shown.
> 
> The problem occurs with CR/LF.

I finally have that test case working, took way longer than I wanted to:

-- snip --
Subject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice when failing
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
Date:   Fri Oct 9 11:15:30 2015 +0200
    
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a0013ee..bce332f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -237,6 +237,18 @@ test_expect_success '--rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success 'failed --rebase shows advice' '
+	git checkout -b diverging &&
+	test_commit attributes .gitattributes "* text=auto" attrs &&
+	sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
+	git update-index --cacheinfo 0644 $sha1 file &&
+	git commit -m v1-with-cr &&
+	git checkout -f -b fails-to-rebase HEAD^ &&
+	test_commit v2-without-cr file "2" file2-lf &&
+	test_must_fail git pull --rebase . diverging 2>err >out &&
+	grep "When you have resolved this problem" out
+'
+
 test_expect_success '--rebase fails with multiple branches' '
 	git reset --hard before-rebase &&
 	test_must_fail git pull --rebase . copy master 2>err &&
--

So the reason is that `unpack_trees()` fails with

    error: Your local changes to the following files would be overwritten by merge:
	file
    Please, commit your changes or stash them before you can merge.

then returns -1 to its caller, `git_merge_trees()`, which still returns -1 in turn to *its* caller, `merge_trees()`, which then gives up by die()ing:

    Aborting

I think there is more than one fix necessary to truly address the issue: the underlying problem is that Git handles *committed* CR/LF really badly when the corresponding `.gitattributes` label the file as `text=auto`. In fact, those files are labeled as modified in `git status`. If you change the line endings of them, they are labeled as modified in `git status`. And after a `git reset --hard`, they are *still* labeled as modified in `git status`.

I will try to make some time to continue to work on this later today, but in the meantime I would be relatively happy if we could introduce that gentle flag. It is really a very gentle patch, after all, much gentler than reverting to the heavy-handed spawning of `merge-recursive`.

Ciao,
Dscho