threads / patch / 54697

v7Reject non-ff pulls by default

Subject: [PATCH v7 0/1] Reject non-ff pulls by default

## tl;dr

11 messages between Nov 23, 2020 and Nov 24, 2020. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

Felipe Contreras· Nov 23, 2020, 22:46 UTC · lore

This is an attempt to revive the old patch series [1], except the only thing it's doing is the important change: adding the option to reject non-ff pulls.

Following this patch the next one should add a new default mode that warns instead of fail, and the next one change the default to ff-only.

This leaves the interface in an inconsistent state, IMO, since I believe these should work:
  * git pull --rebase
  * git pull --merge
  * git -c pull.mode=rebase pull
  * git -c pull.mode=merge pull
  * git -c pull.mode=ff-only pull
But that can be fixed later.
Cheers.
[1] https://lore.kernel.org/git/1398988808-29678-1-git-send-email-felipe.contreras@gmail.com/
Felipe Contreras (1):
  pull: add ff-only option
 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(-)
-- 
2.29.2
Felipe Contreras· Nov 23, 2020, 22:46 UTC · re: Felipe Contreras · lore

[PATCH v7 1/1] pull: add ff-only option

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(-)
Show changes to 6 files +49 −2

Documentation/config/branch.txt, Documentation/config/pull.txt, builtin/pull.c, rebase.c, rebase.h, t/t5520-pull.sh

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
-- 
2.29.2
Felipe Contreras· Nov 23, 2020, 23:02 UTC · re: Felipe Contreras · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 4:46 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 21 quoted lines
>
> 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).
-- 
Felipe Contreras
Alex Henrie· Nov 23, 2020, 23:22 UTC · re: Felipe Contreras · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 3:46 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 5 quoted lines
>
> 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
Junio C Hamano· Nov 23, 2020, 23:32 UTC · re: Alex Henrie · lore

Re: [PATCH v7 1/1] pull: add ff-only option

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 9 quoted lines
> 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.

Felipe Contreras· Nov 23, 2020, 23:51 UTC · re: Junio C Hamano · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
>
> 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.
-- 
Felipe Contreras
Jeff King· Nov 24, 2020, 01:45 UTC · re: Felipe Contreras · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 05:51:42PM -0600, Felipe Contreras wrote:
Show 14 quoted lines
> > 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
Felipe Contreras· Nov 24, 2020, 00:08 UTC · re: Alex Henrie · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 5:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
Show 10 quoted lines
>
> 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.

-- 
Felipe Contreras
Raymond E. Pasco· Nov 24, 2020, 00:32 UTC · re: Felipe Contreras · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon Nov 23, 2020 at 7:08 PM EST, Felipe Contreras wrote:
Show 8 quoted lines
> 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.

Alex Henrie· Nov 24, 2020, 01:51 UTC · re: Raymond E. Pasco · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 5:52 PM Raymond E. Pasco <ray@ameretat.dev> wrote:
Show 18 quoted lines
>
> 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
Felipe Contreras· Nov 24, 2020, 03:45 UTC · re: Alex Henrie · lore

Re: [PATCH v7 1/1] pull: add ff-only option

On Mon, Nov 23, 2020 at 7:51 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
Show 24 quoted lines
>
> 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).

-- 
Felipe Contreras

← back to recent threads