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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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(-)

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

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

[Felipe Contreras]

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).

-- 
Felipe Contreras

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

[Alex Henrie]

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

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

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.

-- 
Felipe Contreras

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

[Jeff King]

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

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

[Felipe Contreras]

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.

-- 
Felipe Contreras

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

[Raymond E. Pasco]

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.

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

[Alex Henrie]

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

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

[Felipe Contreras]

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).

-- 
Felipe Contreras