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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 9, 2020, 14:09 UTC
Message-ID: <nycvar.QRO.7.76.6.2012091502000.25979@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqpn3j4ved.fsf@gitster.c.googlers.com>

Hi Junio,

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

Show 29 quoted lines

> * 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