| Message ID | 3c07ce978caa832b08c6bef1c48c061e41a6fd0b.1626316849.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle conflicting pull options | expand |
On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > The --rebase[=...] flags and the various ff flags are incompatible, > except that --no-rebase (or --rebase=false) work with any of the ff > flags, and --ff works with any of the rebase flags. > [...] > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > +test_expect_success 'report conflicting configuration' ' > + git reset --hard c2 && > + test_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err && > + test_i18ngrep "pull.rebase and pull.ff are incompatible; please unset one" err > + > +' nit: unnecessary and inconsistent blank line at end of test
Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > The --rebase[=...] flags and the various ff flags are incompatible, > except that --no-rebase (or --rebase=false) work with any of the ff > flags, and --ff works with any of the rebase flags. This is wrong, the following commands should work git -c pull.ff=only -c pull.rebase=true pull git -c pull.ff=only -c pull.rebase=true pull --no-rebase
On Wed, Jul 14, 2021 at 10:00 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > The --rebase[=...] flags and the various ff flags are incompatible, > > except that --no-rebase (or --rebase=false) work with any of the ff > > flags, and --ff works with any of the rebase flags. > > [...] > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > > +test_expect_success 'report conflicting configuration' ' > > + git reset --hard c2 && > > + test_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err && > > + test_i18ngrep "pull.rebase and pull.ff are incompatible; please unset one" err > > + > > +' > > nit: unnecessary and inconsistent blank line at end of test Good catch, will fix (assuming the relevant folks agree with my general --ff-only & --no-ff vs. --rebase handling approach).
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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). > 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? > 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.
Junio C Hamano wrote: > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > --- a/Documentation/config/pull.txt > > +++ b/Documentation/config/pull.txt > > 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? Doing such a thing would be wrong. As I already explained multiple times, pull.rebase can be overriden by the command line. When doing --merge we want to honor pull.ff, when we don't we want to ignore it. Since both Elijah and Junio have muted me, and they keep making these mistakes, can anybody else forward this point to them? Cheers.
On Thu, Jul 15, 2021 at 10:33 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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.
Elijah Newren <newren@gmail.com> writes: > 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". I agree (1) never gives you anything, even though it does not hurt, and (2) is "meh". > 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. Well, overriding "only fast-forward is allowed" with "instead of merge, you can rebase" is a nonsense suggestion in the first place, no? Why does Git suddenly become responsible for such a misguided suggestion? > 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. Again, "I want to pee in the snow" is not what you need to act as an integrator. I do not see how relevant this example is, either. You are just reacting to a wrong suggestion. > 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. That is something we need to and can fix. The "pee in the snow commit can be created by passing --no-ff" was written back when the designed audiences of "pull" were primarily those who merge (think of "pull --rebase" as afterthought). IOW, to the minds of those who originally wrote --no-ff feature (and its doc), "pull --rebase" was not in the picture.
Elijah Newren <newren@gmail.com> writes: > 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 see downsides if you make them "incompatible" and cause the combination to error out, but otherwise no. I can imagine those who regularly use pull.rebase=yes in the configuration to say --ff-only for a single-shot, for example. We can view it as either (1) command line --ff-only overriding configured pull.rebase, or (2) ff-only made the choice of pull.rebase irrelevant---the end result is the same (as long as you do not say "no, that's incompatible" and error out). > 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. When you say "git rebase --no-ff upstream", you are telling the machinery that even if your current work is direct descendant of the upstream, you want your commits freshly rebuilt. But in the context of "pull", you being descendant of the other side is called "already up to date", not even "fast forward" (the ancestry relationship between you and the other side is the other way around). Does the "rebase --no-ff" even kick in in the context of "pull --no-ff"?
On Thu, Jul 15, 2021 at 1:17 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > 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. > > When you say "git rebase --no-ff upstream", you are telling the > machinery that even if your current work is direct descendant of the > upstream, you want your commits freshly rebuilt. But in the context > of "pull", you being descendant of the other side is called "already > up to date", not even "fast forward" (the ancestry relationship > between you and the other side is the other way around). Oh, good point.
On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > 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". > > I agree (1) never gives you anything, even though it does not hurt, > and (2) is "meh". Okay, let's drop this series then. Thanks for pointing out my mix-up on the rebase --no-ff thing in the other email.
Elijah Newren <newren@gmail.com> writes: > On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: >> >> > 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". >> >> I agree (1) never gives you anything, even though it does not hurt, >> and (2) is "meh". > > Okay, let's drop this series then. Not so fast. I did have problem with some combinations you hinted (vaguely---so it is more like "combinations I thought you hinted"), but making sure various combinations of options and configuration variables work sensibly is a worthy goal to have, I would think.
On Thu, Jul 15, 2021 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Elijah Newren <newren@gmail.com> writes: > >> > >> > 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". > >> > >> I agree (1) never gives you anything, even though it does not hurt, > >> and (2) is "meh". > > > > Okay, let's drop this series then. > > Not so fast. I did have problem with some combinations you hinted > (vaguely---so it is more like "combinations I thought you hinted"), > but making sure various combinations of options and configuration > variables work sensibly is a worthy goal to have, I would think. It may be a worthy goal, but I cannot implement correct behavior if I cannot determine what correct behavior is. You've only specified how to handle a subset of the valid combinations in each of your emails, and from those individually or even collectively I cannot deduce rules for handling the others. Reading the dozen+ recent messages in the various recent threads, I think I've figured out your opinion in all but two cases, but I have no idea your intent on those two (I would have thought --rebase override there too, but you excluded that), and I'm rather uncertain I've correctly understood you for the other ones (I really hope gmail doesn't whitespace damage the following table): pull.ff pull.rebase commandline action * * --ff-only --rebase fast-forward only[1] * * --rebase --no-ff rebase[1] * * --rebase --ff rebase[1] * * --ff-only --no-rebase fast-forward only * * --no-rebase --no-ff merge --no-ff * * --no-rebase --ff merge --ff <unset> * --no-rebase merge --ff only * --no-rebase merge --ff[2] false * --no-rebase merge --no-ff true * --no-rebase merge --ff <unset> * --rebase rebase only * --rebase rebase[2] false * --rebase ?[2] true * --rebase ?[2] * * --ff-only fast-forward only[1] * <unset> --no-ff merge --no-ff * false --no-ff merge --no-ff * !false --no-ff rebase (ignore --no-ff)[2][3] * <unset> --ff merge --ff * false --ff merge --ff * !false --ff rebase (ignore --ff)[2][3] [1] https://lore.kernel.org/git/xmqq7dhrtrc2.fsf@gitster.g/ https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/ [2] https://lore.kernel.org/git/xmqqpmvn5ukj.fsf@gitster.g/ [3] https://lore.kernel.org/git/xmqq8s2b489p.fsf@gitster.g/ It appears you, Phillip, and I all had different opinions about correct behavior and in a few cases though the documentation clearly implied what we thought. So, I'd have to say the documentation is rather unclear as well. However, even if the above table is filled out, it may be complicated enough that I'm at a bit of a loss about how to update the documentation to explain it short of including the table in the documentation.
Elijah Newren <newren@gmail.com> writes: > the dozen+ recent messages in the various recent threads, I think I've > figured out your opinion in all but two cases, but I have no idea your > intent on those two (I would have thought --rebase override there too, > but you excluded that), and I'm rather uncertain I've correctly > understood you for the other ones (I really hope gmail doesn't > whitespace damage the following table): Yeah, it's about time somebody made an exhaustive table of cases ;-) I appreciate it very much. Without such a table, it is hard to have discussion and keep track of which part is and which part is not what people agree to. Below, I rearranged some rows to group cases I think belong together to make commenting on them easier, but I didn't remove or add any rows. > pull.ff pull.rebase commandline action > * * --ff-only --rebase fast-forward only[1] > * * --ff-only --no-rebase fast-forward only > * * --ff-only fast-forward only[1] I think these three make sense. As you answered to a question in another thread, I agree that --ff-only from the command line should fail instead of letting rebase (or merge for that matter) go ahead when their history is not a descendant of ours and it is a bug if it does not. > * * --rebase --no-ff rebase[1] > * * --rebase --ff rebase[1] OK. > * * --no-rebase --no-ff merge --no-ff > * * --no-rebase --ff merge --ff OK. > <unset> * --no-rebase merge --ff > only * --no-rebase merge --ff[2] > false * --no-rebase merge --no-ff > true * --no-rebase merge --ff I think the second one deserves an explanation. The rationale for ignoring --ff-only is because the act of giving an explicit "--rebase" or "--no-rebase" from the command line, when the configured default is "I expect to have no development on my own here, and only want to follow along", is a sign enough that the user does not want to follow along in this particular invocation of "pull". And the rationale for the entire thing to become --ff is only because between --ff and --no-ff, the former is the default. About the second one, I would understand it if it became "merge --ff-only", too. That is more trivially explained. I however suspect that it would be less useful, but that is open to discussion. All three others I think are not controversial. > <unset> * --rebase rebase > only * --rebase rebase[2] > false * --rebase ?[2] > true * --rebase ?[2] Again the same reasoning applies to the second case that defeats the configured pull.ff=only setting. pull.ff=unset and pull.ff=true would be handled the same, so only the pull.ff=false might deserve an explanation. I not think there needs anything special, as there is no "I want to do something special to mark the fact that we tried to integrate their work" special case needed even when their history is descendant of ours in "rebase" workflow, where we replay what we did on top of theirs (in other words, in "merge" workflow, the shape of history left with --no-ff would be different from --ff; in "rebase", there is no difference---either you had your own development before you ran "pull --rebase" and they got replayed on top of their history, or you had nothing before you ran "pull --rebase" and you did your development after that on top of their history, you'd end up with histories of the same shape. This would not happen with "merge" workflow and that is why --no-ff may matter). So, I agree with the above 4 (and the other 4 for --no-rebase) entries. > * <unset> --no-ff merge --no-ff > * false --no-ff merge --no-ff > * <unset> --ff merge --ff > * false --ff merge --ff OK. > * !false --no-ff rebase (ignore --no-ff)[2][3] > * !false --ff rebase (ignore --ff)[2][3] OK. We do specified rebase, allowing fast-forward. pull.rebase=<something> combined with --no-ff could mean "I do expect I have some development of my own, and please stop me if I don't" and instead of rebasing nothing, error out without even repointing our branch tip to their history when it fast forwards, but I suspect that it would be less useful. Again, that (and anything else I said here) is open to discussion. > It appears you, Phillip, and I all had different opinions about > correct behavior and in a few cases though the documentation clearly > implied what we thought. So, I'd have to say the documentation is > rather unclear as well. However, even if the above table is filled > out, it may be complicated enough that I'm at a bit of a loss about > how to update the documentation to explain it short of including the > table in the documentation. Thanks.
Junio C Hamano wrote: > Elijah Newren <newren@gmail.com> writes: > > > <unset> * --no-rebase merge --ff > > only * --no-rebase merge --ff[2] > > false * --no-rebase merge --no-ff > > true * --no-rebase merge --ff > > I think the second one deserves an explanation. The rationale for > ignoring --ff-only is because the act of giving an explicit > "--rebase" or "--no-rebase" from the command line, when the > configured default is "I expect to have no development on my own > here, and only want to follow along", is a sign enough that the user > does not want to follow along in this particular invocation of > "pull". And the rationale for the entire thing to become --ff is > only because between --ff and --no-ff, the former is the default. > > About the second one, I would understand it if it became "merge > --ff-only", too. That is more trivially explained. I however > suspect that it would be less useful, but that is open to > discussion. It would be very hard to explain in the documentation why these are different: git -c pull.ff=only pull --merge git pull --ff-only --merge And this of course will break behavior for people that arely already relying on pull.ff=only to do --ff-only (as it was its whole purpose). Not to mention that this isn't even listed in the table: git -c pull.ff=only pull As well as *all* the configurations with no command line arguments.
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. 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. + When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase' so that the local merge commits are included in the rebase (see 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. ++ See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in linkgit:git-config[1] if you want to make `git pull` always use `--rebase` instead of merging. diff --git a/builtin/pull.c b/builtin/pull.c index d99719403d0..b355fd38794 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -109,6 +109,11 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset *value = parse_config_rebase("--rebase", arg, 0); else *value = unset ? REBASE_FALSE : REBASE_TRUE; + + /* --rebase overrides earlier --ff-only and --no-ff */ + if (*value != REBASE_FALSE) + opt_ff = "--ff"; + return *value == REBASE_INVALID ? -1 : 0; } @@ -119,6 +124,10 @@ static int parse_opt_ff(const struct option *opt, const char *arg, int unset) else opt_ff = xstrfmt("--%s", opt->long_name); + /* --ff-only and --no-ff override earlier --rebase */ + if (strcmp(opt_ff, "--ff")) + opt_rebase = REBASE_FALSE; + return 0; } @@ -984,6 +993,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase < 0) opt_rebase = config_get_rebase(&rebase_unspecified); + if (opt_rebase != REBASE_FALSE && opt_ff && strcmp(opt_ff, "--ff")) + die(_("pull.rebase and pull.ff are incompatible; please unset one")); + if (read_cache_unmerged()) die_resolve_conflict("pull"); diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 52e8ccc933a..73a0dbdf25a 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -143,6 +143,73 @@ test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' test_i18ngrep ! "Pulling without specifying how to reconcile" err ' +test_does_rebase() { + git reset --hard c2 && + git "$@" . c1 && + # Check that we actually did a rebase + git rev-list --count HEAD >actual && + git rev-list --merges --count HEAD >>actual && + test_write_lines 3 0 >expect && + test_cmp expect actual && + rm actual expect +} + +test_does_merge() { + git reset --hard c2 && + git "$@" . c1 && + # Check that we actually did a merge + git rev-list --count HEAD >actual && + git rev-list --merges --count HEAD >>actual && + test_write_lines 4 1 >expect && + test_cmp expect actual && + rm actual expect +} + +test_attempts_fast_forward() { + git reset --hard c2 && + test_must_fail git "$@" . c1 2>err && + test_i18ngrep "Not possible to fast-forward, aborting" err +} + +test_expect_success 'conflicting options: --ff-only --rebase' ' + test_does_rebase pull --ff-only --rebase +' + +test_expect_success 'conflicting options: --no-ff --rebase' ' + test_does_rebase pull --no-ff --rebase +' + +test_expect_success 'conflicting options: -c pull.ff=false --rebase' ' + test_does_rebase -c pull.ff=false pull --rebase +' + +test_expect_success 'conflicting options: -c pull.ff=only --rebase' ' + test_does_rebase -c pull.ff=only pull --rebase +' + +test_expect_success 'conflicting options: --rebase --ff-only' ' + test_attempts_fast_forward pull --rebase --ff-only +' + +test_expect_success 'conflicting options: --rebase --no-ff' ' + test_does_merge pull --rebase --no-ff +' + +test_expect_success 'conflicting options: -c pull.rebase=true --no-ff' ' + test_does_merge -c pull.rebase=true pull --no-ff +' + +test_expect_success 'conflicting options: -c pull.rebase=true --ff-only' ' + test_attempts_fast_forward -c pull.rebase=true pull --ff-only +' + +test_expect_success 'report conflicting configuration' ' + git reset --hard c2 && + test_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err && + test_i18ngrep "pull.rebase and pull.ff are incompatible; please unset one" err + +' + test_expect_success 'merge c1 with c2' ' git reset --hard c1 && test -f c0.c &&