On Thu, Jul 15, 2021 at 10:33 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt > > index 54048306095..e70ed99e408 100644 > > --- a/Documentation/config/pull.txt > > +++ b/Documentation/config/pull.txt > > @@ -7,12 +7,13 @@ pull.ff:: > > line). When set to `only`, only such fast-forward merges are > > allowed (equivalent to giving the `--ff-only` option from the > > command line). This setting overrides `merge.ff` when pulling. > > + Incompatible with pull.rebase. > > This update may mean well, but I sense that the description needs to > be tightened up a bit more. Unless you mean to say that I am not > allowed to say "[pull] rebase=no" when I set "[pull] ff", that is. > > Or do you mean pull.ff=only is incompatible with any setting of > pull.rebase? > > Or do you mean pull.ff=only is incompatible with any setting of > pull.rebase other than false? > > Or do you mean any setting of pull.ff is imcompatible with any > setting of pull.rebase other than false? > > (You do not have to answer---the above questions just demonstrate > that the description is unnecessarily loose). Fair enough. > > pull.rebase:: > > When true, rebase branches on top of the fetched branch, instead > > of merging the default branch from the default remote when "git > > pull" is run. See "branch.<name>.rebase" for setting this on a > > - per-branch basis. > > + per-branch basis. Incompatible with pull.ff. > > Likewise. > > I think it technically is OK to say "only ff update is allowed" or > "ff update is allowed when able" while saying pull.rebase=yes. > > - For those who say ff=only, the actual value of pull.rebase would > not matter (i.e. the integration immediately finishes by updating > to what was obtained from the upstream because there is no new > development on our own on top of theirs to either replay or > merge). > > - For those who merely allow ff, an update may fast-forward even > when pull.rebase is set to true (or any non-false value), while > we'll replay our own development on top of theirs when their > history is not descendent of ours. > > So I do not see need to declare these combinations "incompatible". > But perhaps I am missing some subtle cases? Let me ask two questions: 1. When is it beneficial for users to set both pull.ff and pull.rebase? 2. Is it harmful to users for us to allow both to be set when we will just ignore one? I believe the answer to (1) is "never", and the answer to (2) is "yes". For the second question in particular, I can think of two example cases: 2a) Users start with pull.ff=only, perhaps suggested by someone else and left in their config for a long time. When users hit a case that can't fast-forward and they either ask for help or find a post on stack overflow that suggests setting pull.rebase=true, they do so and then get no warning that the setting they just added is being ignored. 2b) Perhaps users start with pull.rebase=true (suggested by a colleague and forgot about it as they are more of a tester than a developer and thus usually only see fast-forwards). Then at some point they need to function as an integrator, and they read the docs and determine that pull.ff=false should do what they want to create merge commits. But then they get shocked that they've rebased public commits (and perhaps also pushed them out) when they wanted merges. Our docs have pretty clearly stated that pull.ff=false and --no-ff create merges. Additionally, pull.rebase=interactive seems like a case that interacts poorly with pull.ff. It may be that if you disagree that pull.rebase and pull.ff are more generally incompatible, then we may still need to micro-document individual incompatibilities. (And does that list grow and get confusing?) > > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt > > index 5c3fb67c014..03e8694e146 100644 > > --- a/Documentation/git-pull.txt > > +++ b/Documentation/git-pull.txt > > @@ -121,6 +121,9 @@ When false, merge the current branch into the upstream branch. > > + > > When `interactive`, enable the interactive mode of rebase. > > + > > +Note that these flags are incompatible with --no-ff and --ff-only; if > > +such incompatible flags are given, the last one will take precedence. > > ++ > > I am not sure about these, either. Again, --ff-only (whether it is > combined with --rebase or --rebase=no) would mean that the operation > would fail when we have our own development on top of their history, > and we repoint the tip of our history to theirs when we do not. > > We see "--no-ff" is explained to "always create an unnecessary extra > merge", bit I am not sure it is the right mental model to apply when > the user prefers rebasing. The merge workflow is to treat our > history as the primary and merging theirs in, so when theirs is a > descendant (i.e. we have no new development of our own), some people > may want to mark "we were there before we updated from them" with an > extra merge. Without --no-ff, the resulting history would be quite > different in the merge workflow if you have or do not have your own > development. But the rebase workflow is to treat their history as > the primary and replay our own development on top of theirs, and the > shape of the resulting history would be the same whether you have > your own development on top of theirs. We start from their tip and > then replay our own development on top (which could be an empty > set), and there is nothing "--no-ff" would need to do differently to > keep the resulting history similar to cases where we do have > something of our own. IOW, "--no-ff" becoming a no-op in a "rebase" > workflow is a natural consequence, and there is no strong reason to > say it is incompatible. I think you're overlooking a variety of ways this could harm the user. But first let me check if I understand you correctly: I think you're suggesting that * if --ff-only is given along with any --rebase[=*] flags, then we silently ignore the --rebase[=*] flags. * if --no-ff is given along with any --rebase flag other than --rebase=false, then we silently ignore --no-ff. Let me ask the same two questions as for the config: Does it benefit the users to allow them to specify both flags? Does it hurt some users, especially if undocumented that one of the flags will be ignored? I think the same usecases from the config settings apply, though the fact that both flags are present simultaneously on the command line at least makes it less problematic in the case of --ff-only (users will more quickly discover why their --rebase=<whatever> flag was ignored). But there's an additional reason why treating --no-ff as a no-op is harmful when --rebase is present: git rebase has a --no-ff flag (a synonym for --force-rebase). Some users may intend for git pull --rebase --no-ff to run `git fetch origin && git rebase --no-ff FETCH_HEAD`. The fact that --no-ff will be ignored and sometimes results in a fast-forward when the user explicitly requested no fast forward seems buggy. I thought this was the biggest hole in my proposal: there is both a git merge --no-ff and a git rebase --no-ff, and they avoid fast-forwards in different ways with different results. It _might_ be beneficial to allow both to be triggered directly from the pull command. However, allowing both has some backward compatibility problems because (1) --no-ff is documented as making a merge, and (2) people could have pull.rebase=true set and forget about it and then get surprised when they add --no-ff to the command line and get a rebase --no-ff (or even a plain rebase without --no-ff) rather than the documented merge --no-ff. I think the split config/parameters of git pull make it really hard for us to determine whether users intend to get a merge --no-ff or rebase --no-ff. I think silently ignoring and not even documenting these pitfalls is problematic. We should at least document them.
