Re: 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 Wed, 9 Dec 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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:
>
> With respect to what is in 'seen' that are not marked as "Will merge
> to...", I am merely a messenger, and you are shooting one ;-).

Yes, I guess my complaint was not actually directed to _you_.

> I know of these breakages, becuase I've seen them before pushing the
> integration results out.  The first-parent history near the tip of
> 'seen' looks like so:
>
>     4a386f47a7 Merge branch 'fc/pull-merge-rebase' into seen
>     e0b9615ea3 Merge branch 'es/config-hooks' into seen
>     addabb0fb7 ### start breaking 5411,5520,4013,5524,6402,5604,...
>     0ffeed16ab Merge branch 'sj/untracked-files-in-submodule-dir...
>
> Notice the ### separator?  Everything above the line breaks 'seen'.
> (Emily cc'ed---even though her config-hooks is not on topic of your
> message, it is above the separator as the result of merging it to
> 'seen' breaks 5411).

I had not actually noticed that separator because I had worked off of the
topic branch itself, and it does not pass the test suite.

> The reason why I am experimenting with this is because I suspect
> that those who are involved in the topics above the separator would
> want them in 'seen', as it would be easier to work with, when the
> breakage may be coming from interactions with other topics, to
> pinpoint what interaction may be at fault.
>
> But if you and others who are not involved in these topics above the
> separator want to see 'seen' in a buildable/testable state by
> excluding them, I am fine with that, too.  It is a hassle for me to
> maintain the two parts of 'seen' the way I pushed out last night.  I
> have to identify which merge of the topics into 'seen' breaks
> builds/tests, shuffle the order of topics, minimizing the number of
> topics above the breakage point, and tentatively rewind 'seen' to
> below the breakage point when building and installing for my own
> use, etc.  It would become unnecessary if I can just discard new
> topics after seeing builds/tests break in 'seen'.

It is up to you what you integrate into `seen`. From a purely procedural
point of view, if it was up to me, I would liberally drop any patch series
that does not pass our CI build. In my mind, patches must be correct
enough to pass that check before they are ready for review.

Please note that I am making a big difference between patches that work on
their own (but break the test suite when being merged into `seen`) and
patches that break the test suite "without any outside help".

I am happy to assist in fixing problems with the first category (those
that only break when merged with other topics). It's what I do already in
Git for Windows' `shears/*` branches where Git for Windows' `main` branch
is continuously rebased onto the four integration branches of git.git. The
idea is to catch conflicts between topics that are in-flight.

At the same time, I would rather like to avoid having to fix the CI build
for topics that are already broken without even being integrated into
`seen`. I did that in the past so that I could benefit from seeing
conflicts between Git for Windows' topics and upcoming changes in Git, but
I plan on leaving things in a broken state when I see broken topics (i.e.
the second category I mentioned above).

> > 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.
>
> I do not doubt the need to make sure that the test suite covers the
> new use case as well as the need to adjust the existing tests.
>
> Having said that, I have a strong suspicion that the "turn the
> current warn-but-go-ahead-to-merge into error-out-and-stop for those
> who haven't chosen between rebase and merge when we see a non-ff
> situation" this topic aims for *can* be made without causing as much
> harm to existing users' workflows as these test failures imply.  The
> reason why I say so is because the existing behaviour of loudly
> warning is so annoying, even with the current behaviour of making a
> merge after showing the warning, existing users would have made and
> recorded the choice in pull.rebase long time ago already.  Those who
> want to rebase by default would have had more incentive to avoid
> the "warn but still go ahead to merge" doing a wrong thing to them,
> but those who want to merge by default would also have set it, if
> only to squelch the annoying loud warning.
>
> A spot check: do you have pull.rebase set to anything in your
> config?

I do, as a matter of fact.

As do Git for Windows users as of version v2.27.1 or so, because Git for
Windows' installer now asks specifically about the desired behavior and
configures it explicitly.

> So, if this "default change" is done in such a way so that it won't
> make any difference to those who have pull.rebase set to either yes
> or no, I suspect that the fallout would not be as bad as what these
> test failures imply.

That assumes that all setups call `git pull` interactively.

In my case, I have tons of automation (how would I be able to maintain Git
for Windows otherwise?) and I recently stumbled over a `pull.rebase`
advice in one of those pipelines.

Since the advice was non-fatal, I did not fix it (and would not have seen
the advice at all, if it hadn't been for an unrelated problem I had to
track down).

> On the other hand, if the implementation in the topic forces
> everybody (including those who have made the choice by setting
> pull.rebase) to set another variable before allowing to work with a
> non-ff pull, it would break everybody and is a disaster.

Well, here's my vote for keeping the current behavior to continue to merge
and show that advice about pull.rebase. I find it the least annoying
option of all that are available to us.

Ciao,
Dscho