On 2015-10-09 12.11, Johannes Schindelin wrote:
> 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 <gits...@pobox.com> wrote:
>>>> Johannes Schindelin <johannes.schinde...@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.schinde...@gmx.de>
> Date: Fri Oct 9 11:15:30 2015 +0200
>
> Signed-off-by: Johannes Schindelin <johannes.schinde...@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`.
This is related to the normalization feature of Git:
https://www.kernel.org/pub/software/scm/git/docs/gitattributes.html
* text=auto
This ensures that all files that Git considers to be text will have normalized
(LF) line endings in the repository.
The normalization feature has 2 consequences:
a) - Files will get normalized at the next commit,
Line endings of the changed lines are normalized
Line endings of unchanged lines are normalized
b) - Not normalized files will get normalized (at the next commit),
even if they are unchanged otherwise.
As Git knows (* text=auto), that files are normalized at the next commit,
they will change in the repo, and they are marked as changed already now.
This is by design.
The normalization has been disabled for core.autocrlf = true in commit
fd6cce9e (Eyvind Bernhardsen 2010-05-19 22:43:10 +0200 207)
* This is the
new safer autocrlf handling.
(See convert.c)
I'm in the mood to propose a patch that disables/suppresses the normalization
even for "* text=auto", if a file has CRLF in the repo.
This would make core.autocrlf = true do the same as "* text=auto".
I'm nearly sure, that this change would break things for some users,
and improve for others.
Currently t0027 tests this behavior, and as soon as we have the new
NNO tests establish, I will propose some cleanups in convert.c
(without change of behavour), and later to make
core.autocrlf = true
to do the same as
* text=auto
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
