fc/pull-merge-rebase, was Re: What's cooking in git.git (Dec 2020, #01; Tue, 8)

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

Hi Junio,

On Tue, 8 Dec 2020, Junio C Hamano wrote:

> * fc/pull-merge-rebase (2020-12-08) 19 commits
>  - future: pull: enable ff-only mode by default
>  - pull: advice of future changes
>  - pull: add pull.mode=ff-only
>  - pull: add pull.mode
>  - pull: trivial memory fix
>  - test: pull-options: revert unnecessary changes
>  - test: merge-pull-config: trivial cleanup
>  - pull: move configurations fetches
>  - rebase: add REBASE_DEFAULT
>  - pull: show warning with --ff
>  - pull: introduce --merge option
>  - pull: trivial whitespace style fix
>  - pull: display default warning only when non-ff
>  - pull: move default warning
>  - pull: trivial cleanup
>  - pull: cleanup autostash check
>  - pull: refactor fast-forward check
>  - pull: improve default warning
>  - doc: pull: explain what is a fast-forward
>
>  When a user does not tell "git pull" to use rebase or merge, the
>  command gives a loud message telling a user to choose between
>  rebase or merge but creates a merge anyway, forcing users who would
>  want to rebase to redo the operation.  Fix this by (1) tightening
>  the condition to give the message---there is no reason to stop or
>  force the user to choose between rebase or merge if the history
>  fast-forwards, and (2) failing the operation when the history does
>  not fast-forward, instead of making a merge, in such a case.

Despite what the commit message of the tip commit says, it is not "time to
flip the switch and make ff-only the default" because it breaks our very
own test suite left and right. See for yourself:

https://github.com/gitgitgadget/git/runs/1521231966?check_suite_focus=true
claims that a whopping 120 (!!!) test cases fail. Here's the exact list:

Test Summary Report
-------------------
t4013-diff-various.sh                            (Wstat: 256 Tests: 195
Failed: 84)
  Failed tests:  1, 49-62, 66-68, 70-84, 94-109, 113-117
                120-123, 131-134, 137-138, 140-143, 145-146
                164-168, 182-186, 188-191
  Non-zero exit status: 1
t5524-pull-msg.sh                                (Wstat: 256 Tests: 3
Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 1
t5521-pull-options.sh                            (Wstat: 256 Tests: 20
Failed: 2)
  Failed tests:  12, 16
  Non-zero exit status: 1
t5553-set-upstream.sh                            (Wstat: 256 Tests: 19
Failed: 6)
  Failed tests:  11-14, 16-17
  Non-zero exit status: 1
t5520-pull.sh                                    (Wstat: 256 Tests: 81
Failed: 8)
  Failed tests:  16, 33-39
  Non-zero exit status: 1
t5604-clone-reference.sh                         (Wstat: 256 Tests: 33
Failed: 4)
  Failed tests:  13-16
  Non-zero exit status: 1
t6409-merge-subtree.sh                           (Wstat: 256 Tests: 12
Failed: 3)
  Failed tests:  9, 11-12
  Non-zero exit status: 1
t6417-merge-ours-theirs.sh                       (Wstat: 256 Tests: 7
Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
t6402-merge-rename.sh                            (Wstat: 256 Tests: 46
Failed: 8)
  Failed tests:  2-8, 11
  Non-zero exit status: 1
t7603-merge-reduce-heads.sh                      (Wstat: 256 Tests: 13
Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=915, Tests=22158, 448 wallclock secs ( 5.63 usr  1.64 sys + 555.46
cusr 190.71 csys = 753.44 CPU)
Result: FAIL

Given that not even our very own test suite is well-suited to this change,
I rather doubt that this is a safe thing to do.

In the _least_, the patch series should put in the effort to show just how
much work it is to adjust the test suite to let it pass again. This would
also give an indication how much work we impose on our users by that
ff-only change in behavior.

Ciao,
Dscho