| Message ID | 20201123224621.2573159-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Reject non-ff pulls by default | expand |
On Mon, Nov 23, 2020 at 4:46 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > It is very typical for Git newcomers to inadvertently create merges and > worse; inadvertently pushing them. This is one of the reasons many > experienced users prefer to avoid 'git pull', and recommend newcomers to > avoid it as well. > > To avoid these problems, and keep 'git pull' useful, it has been > suggested that 'git pull' barfs by default if the merge is > non-fast-forward, which unfortunately would break backwards > compatibility. > > This patch leaves everything in place to enable this new mode, but it > only gets enabled if the user specifically configures it; > > pull.rebase = ff-only. > > Later on this mode can be enabled by default. > > For the full discussion you can read: > > https://lore.kernel.org/git/5363BB9F.40102@xiplink.com/ Note: Philip Oakley's address was wrong (fixed now).
On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > This patch leaves everything in place to enable this new mode, but it > only gets enabled if the user specifically configures it; > > pull.rebase = ff-only. Why not use the existing pull.ff=only option instead of adding a new one? -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: >> >> This patch leaves everything in place to enable this new mode, but it >> only gets enabled if the user specifically configures it; >> >> pull.rebase = ff-only. > > Why not use the existing pull.ff=only option instead of adding a new one? If you have pull.rebase=false, "git -c pull.ff=only pull" would fail as desired upon a non-fast-forward. But if you have pull.rebase=true, does it fail the same way (not a rhetorical question; I didn't try)? If so, I agree we do not need a new one. Otherwise, I am on two minds. Having just a single variable would be easier to manage, so pull.rebase=ff-only that is equivalent to pull.ff=only might be claimed to be UI improvement. On the other hand, it looks quite funny for that single variable that controls the way how pull works, whether rebase or merge is used, is pull.REBASE.
On Mon, Nov 23, 2020 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > >> > >> This patch leaves everything in place to enable this new mode, but it > >> only gets enabled if the user specifically configures it; > >> > >> pull.rebase = ff-only. > > > > Why not use the existing pull.ff=only option instead of adding a new one? > > If you have pull.rebase=false, "git -c pull.ff=only pull" would fail > as desired upon a non-fast-forward. But if you have > pull.rebase=true, does it fail the same way (not a rhetorical > question; I didn't try)? If so, I agree we do not need a new one. No. It attempts the rebase, because whatever is set in pull.ff affects only the merge mode. Also, I don't think there's any way to tell git rebase to fail if it's not fast-forward (not that we should attempt the rebase anyway). > On the other hand, it looks quite funny for that single variable > that controls the way how pull works, whether rebase or merge is > used, is pull.REBASE. Which is precisely why I wanted to rename it to pull.mode. In my option git pull should have three main modes: 1. fast-forward only 2. merge 3. rebase The fast-forward only mode can be considered a merge, or a rebase, doesn't matter. Cheers.
On Mon, Nov 23, 2020 at 5:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > This patch leaves everything in place to enable this new mode, but it > > only gets enabled if the user specifically configures it; > > > > pull.rebase = ff-only. > > Why not use the existing pull.ff=only option instead of adding a new one? Because that option is only for the merge. I'd have no objection in refactoring this option so it's used for both, but this breaks existing behavior. A user that has "pull.ff=only", and does a "git pull --rebase" might expect it to work. My main interest is what happens when the user has not configured anything, which I defined with a "push.mode=default", any option (e.g. --rebase/--merge) would override that default.
On Mon Nov 23, 2020 at 7:08 PM EST, Felipe Contreras wrote: > I'd have no objection in refactoring this option so it's used for > both, but this breaks existing behavior. A user that has > "pull.ff=only", and does a "git pull --rebase" might expect it to > work. > > My main interest is what happens when the user has not configured > anything, which I defined with a "push.mode=default", any option (e.g. > --rebase/--merge) would override that default. I feel like the parsimonious change to make is simply defaulting the existing "pull.ff" to "only". I think someone who has set "pull.rebase" expects pull --rebase behavior just as much as someone who passes --rebase on the command line. The issue in question is what someone who has not made any changes to the settings expects to happen with a plain "git pull", and I certainly agree that people who are not power users expect a fast-forward (I try not to force my opinions on workflow or style when onboarding people to Git, but I do always recommend "pull.ff=only" because I know this is a perennial pitfall). The problem is that, as it stands now, this would just leave the user with a cryptic error message ("Not possible to fast-forward, aborting") when they wanted to see remote changes. I think this might warrant an even more expanded message than the short one in this patch, but I'm not sure exactly what it should say - there are a few things the user might expect, but an error message isn't the best place for a crash course.
On Mon, Nov 23, 2020 at 05:51:42PM -0600, Felipe Contreras wrote: > > On the other hand, it looks quite funny for that single variable > > that controls the way how pull works, whether rebase or merge is > > used, is pull.REBASE. > > Which is precisely why I wanted to rename it to pull.mode. > > In my option git pull should have three main modes: > > 1. fast-forward only > 2. merge > 3. rebase > > The fast-forward only mode can be considered a merge, or a rebase, > doesn't matter. I agree that is much nicer. I'd worry a bit though that it may be confusing to users to have this new mode option _and_ the existing pull.rebase and pull.ff options, which overlap (but not completely). It might be something we could solve with good documentation, though. -Peff
On Mon, Nov 23, 2020 at 5:52 PM Raymond E. Pasco <ray@ameretat.dev> wrote: > > I feel like the parsimonious change to make is simply defaulting the > existing "pull.ff" to "only". I think someone who has set "pull.rebase" > expects pull --rebase behavior just as much as someone who passes > --rebase on the command line. The issue in question is what someone who > has not made any changes to the settings expects to happen with a plain > "git pull", and I certainly agree that people who are not power users > expect a fast-forward (I try not to force my opinions on workflow or > style when onboarding people to Git, but I do always recommend > "pull.ff=only" because I know this is a perennial pitfall). > > The problem is that, as it stands now, this would just leave the user > with a cryptic error message ("Not possible to fast-forward, aborting") > when they wanted to see remote changes. I think this might warrant an > even more expanded message than the short one in this patch, but I'm > not sure exactly what it should say - there are a few things the user > might expect, but an error message isn't the best place for a crash > course. Another problem is that when pull.ff=only but you really do want to allow merging if fast-forwarding is impossible, having to type `git pull --ff` to get that behavior is *very* counterintuitive :-( -Alex
On Mon, Nov 23, 2020 at 7:51 PM Alex Henrie <alexhenrie24@gmail.com> wrote: > > On Mon, Nov 23, 2020 at 5:52 PM Raymond E. Pasco <ray@ameretat.dev> wrote: > > > > I feel like the parsimonious change to make is simply defaulting the > > existing "pull.ff" to "only". I think someone who has set "pull.rebase" > > expects pull --rebase behavior just as much as someone who passes > > --rebase on the command line. The issue in question is what someone who > > has not made any changes to the settings expects to happen with a plain > > "git pull", and I certainly agree that people who are not power users > > expect a fast-forward (I try not to force my opinions on workflow or > > style when onboarding people to Git, but I do always recommend > > "pull.ff=only" because I know this is a perennial pitfall). > > > > The problem is that, as it stands now, this would just leave the user > > with a cryptic error message ("Not possible to fast-forward, aborting") > > when they wanted to see remote changes. I think this might warrant an > > even more expanded message than the short one in this patch, but I'm > > not sure exactly what it should say - there are a few things the user > > might expect, but an error message isn't the best place for a crash > > course. > > Another problem is that when pull.ff=only but you really do want to > allow merging if fast-forwarding is impossible, having to type `git > pull --ff` to get that behavior is *very* counterintuitive :-( Indeed. It would be much more intuitive to type "git pull --merge" which would override some default (e.g. do not merge unless it's a fast-forward).
diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt index cc5f3249fc..715987c759 100644 --- a/Documentation/config/branch.txt +++ b/Documentation/config/branch.txt @@ -81,6 +81,8 @@ branch.<name>.rebase:: "git pull" is run. See "pull.rebase" for doing this in a non branch-specific manner. + +When `ff-only` (or just 'f'), the rebase will only succeed if it's fast-forward. ++ 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 linkgit:git-rebase[1] for details). diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt index 5404830609..060bacc63c 100644 --- a/Documentation/config/pull.txt +++ b/Documentation/config/pull.txt @@ -14,6 +14,8 @@ pull.rebase:: pull" is run. See "branch.<name>.rebase" for setting this on a per-branch basis. + +When `ff-only` (or just 'f'), the rebase will only succeed if it's fast-forward. ++ 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 linkgit:git-rebase[1] for details). diff --git a/builtin/pull.c b/builtin/pull.c index 17aa63cd35..ba2777c401 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1033,8 +1033,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix) } free_commit_list(list); } - if (!ran_ff) + if (!ran_ff) { + if (opt_rebase == REBASE_FF_ONLY) + die(_("The pull was not fast-forward, please either merge or rebase.\n" + "If unsure, run 'git pull --merge'.")); ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point); + } if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON || recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)) diff --git a/rebase.c b/rebase.c index f8137d859b..eba1253552 100644 --- a/rebase.c +++ b/rebase.c @@ -20,6 +20,8 @@ enum rebase_type rebase_parse_value(const char *value) return REBASE_FALSE; else if (v > 0) return REBASE_TRUE; + else if (!strcmp(value, "ff-only") || !strcmp(value, "f")) + return REBASE_FF_ONLY; else if (!strcmp(value, "preserve") || !strcmp(value, "p")) return REBASE_PRESERVE; else if (!strcmp(value, "merges") || !strcmp(value, "m")) diff --git a/rebase.h b/rebase.h index cc723d4748..127febeb7c 100644 --- a/rebase.h +++ b/rebase.h @@ -7,7 +7,8 @@ enum rebase_type { REBASE_TRUE, REBASE_PRESERVE, REBASE_MERGES, - REBASE_INTERACTIVE + REBASE_INTERACTIVE, + REBASE_FF_ONLY }; enum rebase_type rebase_parse_value(const char *value); diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 9fae07cdfa..d96bdc90cc 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -819,4 +819,40 @@ test_expect_success 'git pull --rebase against local branch' ' test_cmp expect file2 ' +test_expect_success 'git pull fast-forward (ff-only)' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.rebase ff-only && + git checkout -b other master && + >new && + git add new && + git commit -m new && + git checkout -b test -t other && + git reset --hard master && + git pull +' + +test_expect_success 'git pull non-fast-forward (ff-only)' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.rebase ff-only && + git checkout -b other master^ && + >new && + git add new && + git commit -m new && + git checkout -b test -t other && + git reset --hard master && + test_must_fail git pull +' + +test_expect_success 'git pull non-fast-forward with merge (ff-only)' ' + test_when_finished "git checkout master && git branch -D other test" && + test_config pull.rebase ff-only && + git checkout -b other master^ && + >new && + git add new && + git commit -m new && + git checkout -b test -t other && + git reset --hard master && + git pull --no-rebase +' + test_done
It is very typical for Git newcomers to inadvertently create merges and worse; inadvertently pushing them. This is one of the reasons many experienced users prefer to avoid 'git pull', and recommend newcomers to avoid it as well. To avoid these problems, and keep 'git pull' useful, it has been suggested that 'git pull' barfs by default if the merge is non-fast-forward, which unfortunately would break backwards compatibility. This patch leaves everything in place to enable this new mode, but it only gets enabled if the user specifically configures it; pull.rebase = ff-only. Later on this mode can be enabled by default. For the full discussion you can read: https://lore.kernel.org/git/5363BB9F.40102@xiplink.com/ Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/config/branch.txt | 2 ++ Documentation/config/pull.txt | 2 ++ builtin/pull.c | 6 +++++- rebase.c | 2 ++ rebase.h | 3 ++- t/t5520-pull.sh | 36 +++++++++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 2 deletions(-)