[PATCH] pull: abort if --ff-only is given and fast-forwarding is impossible

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Signed-off-by: Alex Henrie <redacted>
---
 advice.c                     |  5 +++++
 advice.h                     |  1 +
 builtin/merge.c              |  2 +-
 builtin/pull.c               | 11 ++++++++---
 t/t7601-merge-pull-config.sh | 24 ++++++++++++++++++++++++
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/advice.c b/advice.c
index 0b9c89c48a..337e8f342b 100644
--- a/advice.c
+++ b/advice.c

@@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)
 	die(_("Exiting because of unfinished merge."));
 }
 
+void NORETURN die_ff_impossible(void)
+{
+	die(_("Not possible to fast-forward, aborting."));
+}
+
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 {
 	struct string_list_item *item;

diff --git a/advice.h b/advice.h
index bd26c385d0..1624043838 100644
--- a/advice.h
+++ b/advice.h

@@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 

diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..aa920ac524 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fast_forward == FF_ONLY)
-		die(_("Not possible to fast-forward, aborting."));
+		die_ff_impossible();
 
 	if (autostash)
 		create_autostash(the_repository,

diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f81084..d979660482 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
+	if (!can_ff) {
+		if (opt_ff) {
+			if (!strcmp(opt_ff, "--ff-only"))
+				die_ff_impossible();
+		} else {
+			if (rebase_unspecified && opt_verbosity >= 0)
+				show_advice_pull_non_ff();
+		}
 	}
 
 	if (opt_rebase) {

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 52e8ccc933..b5a09a60f9 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh

@@ -183,6 +183,30 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
 	test_must_fail git pull . c3
 '
 
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase true &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase false &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --rebase --ff-only . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --no-rebase --ff-only . c3
+'
+
 test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
 	git reset --hard c1 &&
 	git config pull.twohead ours &&

-- 
2.32.0.171.gfa4a44ff46

Alex Henrie wrote:

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

I don't know where that is being said, but it's wrong: --ff-only is
meant for merge only.

As I've mentioned multiple times already, this is wrong.

The advice clearly says:

  You can also pass --rebase, --no-rebase, or --ff-only on the command
  line to override the configured default per invocation.

With your patch now this is even less true:

  git -c pull.ff=only pull --rebase

Can you add this test [1] so I don't have to explain the same thing over
and over?

  test_expect_success 'pull allows non-fast-forward with "only" in pull.ff if --rebase' '
    git reset --hard c1 &&
    test_config pull.ff only &&
    git pull --rebase . c3
  '

Cheers.

+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase true &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '
+	git reset --hard c1 &&
+	test_config pull.ff only &&
+	test_config pull.rebase false &&
+	test_must_fail git pull . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --rebase --ff-only . c3
+'
+
+test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '
+	git reset --hard c1 &&
+	test_must_fail git pull --no-rebase --ff-only . c3
+'
+
 test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
 	git reset --hard c1 &&
 	git config pull.twohead ours &&
-- 

[1] https://lore.kernel.org/git/20210711170703.651081-1-felipe.contreras@gmail.com/

-- 
Felipe Contreras

On Sun, Jul 11, 2021 at 11:08 AM Felipe Contreras
[off-list ref] wrote:

Alex Henrie wrote:

quoted

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

I don't know where that is being said, but it's wrong: --ff-only is
meant for merge only.

quoted

--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)

      can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);

-     if (rebase_unspecified && !opt_ff && !can_ff) {
-             if (opt_verbosity >= 0)
-                     show_advice_pull_non_ff();
+     if (!can_ff) {
+             if (opt_ff) {
+                     if (!strcmp(opt_ff, "--ff-only"))
+                             die_ff_impossible();

As I've mentioned multiple times already, this is wrong.

The advice clearly says:

  You can also pass --rebase, --no-rebase, or --ff-only on the command
  line to override the configured default per invocation.

With your patch now this is even less true:

  git -c pull.ff=only pull --rebase

I think it's an improvement over the current situation. --no-rebase
does not override pull.ff=only, so it makes sense that --rebase does
not override pull.ff=only either. Besides, it's generally better to
abort instead of rewriting history if it's not perfectly clear that
the user meant to rewrite the history.

-Alex

Alex Henrie wrote:

On Sun, Jul 11, 2021 at 11:08 AM Felipe Contreras
[off-list ref] wrote:

quoted

Alex Henrie wrote:

quoted

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

I don't know where that is being said, but it's wrong: --ff-only is
meant for merge only.

quoted

--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)

      can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);

-     if (rebase_unspecified && !opt_ff && !can_ff) {
-             if (opt_verbosity >= 0)
-                     show_advice_pull_non_ff();
+     if (!can_ff) {
+             if (opt_ff) {
+                     if (!strcmp(opt_ff, "--ff-only"))
+                             die_ff_impossible();

As I've mentioned multiple times already, this is wrong.

The advice clearly says:

  You can also pass --rebase, --no-rebase, or --ff-only on the command
  line to override the configured default per invocation.

With your patch now this is even less true:

  git -c pull.ff=only pull --rebase

I think it's an improvement over the current situation. --no-rebase
does not override pull.ff=only, so it makes sense that --rebase does
not override pull.ff=only either.

I disagree, but that's not the point, the point is that now the advice
message is wrong since --rebase doesn't override pull.ff=only.

Additionally the documentation is inaccurate too because at no point
does pull.ff mention anything about rebase:

pull.ff::
	By default, Git does not create an extra merge commit when merging
	a commit that is a descendant of the current commit. Instead, the
	tip of the current branch is fast-forwarded. When set to `false`,
	this variable tells Git to create an extra merge commit in such
	a case (equivalent to giving the `--no-ff` option from the command
	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.

-- 
Felipe Contreras

Hi Alex


On 11/07/2021 02:26, Alex Henrie wrote:

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Thanks for revising this patch, I like this approach much better. I do 
however have some concerns about the interaction of pull.ff with the 
rebase config and command line options. I'd naively expect the following 
behavior (where rebase can fast-forward if possible)

   pull.ff  pull.rebase  commandline  action
    only     not false                rebase
    only     not false   --no-rebase  fast-forward only
     *       not false    --ff-only   fast-forward only
    only     not false    --ff        merge --ff
    only     not false    --no-ff     merge --no-ff
    only       false                  fast-forward only
    only       false      --rebase    rebase
    only       false      --ff        merge --ff
    only       false      --no-ff     merge --no-ff

I don't think enforcing fast-forward only for rebases makes sense unless 
it is given on the command line. If the user gives `--rebase` 
`--ff-only` on the command line then we should either error out or take 
the last one in which case `pull --rebase --ff-only` would fast-forward 
only but `pull --ff-only --rebase` would rebase. We should also decide 
what to do when the user has pull.ff set to something other than only 
and also has pull.rebase to something other than false set - I'd guess 
we'd want to rebase unless there is a merge option on the command line 
but I haven't thought about those cases.

Best Wishes

Phillip

Phillip Wood wrote:

On 11/07/2021 02:26, Alex Henrie wrote:

quoted

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Thanks for revising this patch, I like this approach much better. I do 
however have some concerns about the interaction of pull.ff with the 
rebase config and command line options. I'd naively expect the following 
behavior (where rebase can fast-forward if possible)

   pull.ff  pull.rebase  commandline  action
    only     not false                rebase

Agreed. (pull.ff applies only for --merge)

    only     not false   --no-rebase  fast-forward only

Agreed. (--no-rebase is --merge, and pull.ff applies)

     *       not false    --ff-only   fast-forward only

Disagree. (--ff-only is for --merge)

We would need to change the documentation and the advice warning for
this to be correct.

    only     not false    --ff        merge --ff

Disagree.

This is a rebase, --ff should be ignored.

Junio already proposed --ff and other options to imply a merge [1], but
I already explained why that is problematic [2].

    only     not false    --no-ff     merge --no-ff

Disagree. (ditto)

    only       false                  fast-forward only
    only       false      --rebase    rebase
    only       false      --ff        merge --ff
    only       false      --no-ff     merge --no-ff

Agreed.

I don't think enforcing fast-forward only for rebases makes sense unless 
it is given on the command line.

But why? This is inconsistent.

Everywhere else in git the configuration is another way of specifying
the command line. This would be the first instance where it would not be
the case.

If the user gives `--rebase` `--ff-only` on the command line then we
should either error out or take the last one in which case `pull
--rebase --ff-only` would fast-forward only but `pull --ff-only
--rebase` would rebase.

Following the same logic `pull --ff-only --merge` would ignore the
previous --ff-only, wouldn't it?


This is a pretty significant semantic change and nowhere in this patch
it's explained who this is supposed to help, or what is the motivtion
behind it.

[1] https://lore.kernel.org/git/xmqqmtyf8hfm.fsf@gitster.c.googlers.com/
[2] https://lore.kernel.org/git/5fd8aa6a52e81_190cd7208c8@natae.notmuch/

-- 
Felipe Contreras

On Mon, Jul 12, 2021 at 4:21 AM Phillip Wood [off-list ref] wrote:

On 11/07/2021 02:26, Alex Henrie wrote:

quoted

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Thanks for revising this patch, I like this approach much better. I do
however have some concerns about the interaction of pull.ff with the
rebase config and command line options. I'd naively expect the following
behavior (where rebase can fast-forward if possible)

   pull.ff  pull.rebase  commandline  action
    only     not false                rebase
    only     not false   --no-rebase  fast-forward only
     *       not false    --ff-only   fast-forward only
    only     not false    --ff        merge --ff
    only     not false    --no-ff     merge --no-ff
    only       false                  fast-forward only
    only       false      --rebase    rebase
    only       false      --ff        merge --ff
    only       false      --no-ff     merge --no-ff

I don't think enforcing fast-forward only for rebases makes sense unless
it is given on the command line. If the user gives `--rebase`
`--ff-only` on the command line then we should either error out or take
the last one in which case `pull --rebase --ff-only` would fast-forward
only but `pull --ff-only --rebase` would rebase. We should also decide
what to do when the user has pull.ff set to something other than only
and also has pull.rebase to something other than false set - I'd guess
we'd want to rebase unless there is a merge option on the command line
but I haven't thought about those cases.

I was thinking of --rebase and --ff-only as orthogonal variables.
Nevertheless, we could make --rebase imply --ff, which would be pretty
easy to explain in the documentation for the command-line options.
That way, even though pull.rebase=true with pull.ff=only would enforce
fast-forward-only, the user could easily override it with `git pull
-r`. Would you accept that compromise?

-Alex

Alex Henrie wrote:

On Mon, Jul 12, 2021 at 4:21 AM Phillip Wood [off-list ref] wrote:

quoted

On 11/07/2021 02:26, Alex Henrie wrote:

quoted

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Thanks for revising this patch, I like this approach much better. I do
however have some concerns about the interaction of pull.ff with the
rebase config and command line options. I'd naively expect the following
behavior (where rebase can fast-forward if possible)

   pull.ff  pull.rebase  commandline  action
    only     not false                rebase
    only     not false   --no-rebase  fast-forward only
     *       not false    --ff-only   fast-forward only
    only     not false    --ff        merge --ff
    only     not false    --no-ff     merge --no-ff
    only       false                  fast-forward only
    only       false      --rebase    rebase
    only       false      --ff        merge --ff
    only       false      --no-ff     merge --no-ff

I don't think enforcing fast-forward only for rebases makes sense unless
it is given on the command line. If the user gives `--rebase`
`--ff-only` on the command line then we should either error out or take
the last one in which case `pull --rebase --ff-only` would fast-forward
only but `pull --ff-only --rebase` would rebase. We should also decide
what to do when the user has pull.ff set to something other than only
and also has pull.rebase to something other than false set - I'd guess
we'd want to rebase unless there is a merge option on the command line
but I haven't thought about those cases.

I was thinking of --rebase and --ff-only as orthogonal variables.
Nevertheless, we could make --rebase imply --ff, which would be pretty
easy to explain in the documentation for the command-line options.
That way, even though pull.rebase=true with pull.ff=only would enforce
fast-forward-only, the user could easily override it with `git pull
-r`. Would you accept that compromise?

What happens if the user has configured `pull.ff=no`?

-- 
Felipe Contreras

Hi folks,

I am out of the loop in this thread but I have been seeing strange behaviors
with pull.rebase=true in the 'next' branch and also in the 'master'
branch in recent days.

  > git version
  git version 2.32.0.432.gabb21c7263
  > git config -l | grep pull
  pull.rebase=true
  pull.ff=false

But a git pull would still run fast-forward.
Some of our users (including myself) rely on disabling fast-forward to emit the
per-file change log summary after each git-pull

  Updating 245f278cb729..5e8d960db7b3
  Fast-forward
   some/file/dir.ext         | 44 ++++++++++++++++++++++++++++++++++++++++++++
   another/file/dir.ext     |  6 +++---
  2 files changed, 47 insertions(+), 3 deletions(-)

In a big, fast moving monorepo, this summary is a lot of noise and
switching to pull.rebase=true
used to be the way to turn it off. If the change is intended for next
version release, is there a
workaround for this?

Cheers,
Son Luong

Hello,

Son Luong Ngoc wrote:

I am out of the loop in this thread but I have been seeing strange behaviors
with pull.rebase=true in the 'next' branch and also in the 'master'
branch in recent days.

  > git version
  git version 2.32.0.432.gabb21c7263
  > git config -l | grep pull
  pull.rebase=true
  pull.ff=false

But a git pull would still run fast-forward.
Some of our users (including myself) rely on disabling fast-forward to emit the
per-file change log summary after each git-pull

  Updating 245f278cb729..5e8d960db7b3
  Fast-forward
   some/file/dir.ext         | 44 ++++++++++++++++++++++++++++++++++++++++++++
   another/file/dir.ext     |  6 +++---
  2 files changed, 47 insertions(+), 3 deletions(-)

In a big, fast moving monorepo, this summary is a lot of noise and
switching to pull.rebase=true
used to be the way to turn it off.

This is probably due to 340062243a (pull: cleanup autostash check,
2021-06-17).

I bet you have `rebase.autostash=true` configured as well.

It seems to me you were relying on a bug.

If the change is intended for next
version release, is there a
workaround for this?

git pull -n ?

-- 
Felipe Contreras

On Wed, Jul 14, 2021 at 1:37 AM Son Luong Ngoc [off-list ref] wrote:

Hi folks,

I am out of the loop in this thread but I have been seeing strange behaviors
with pull.rebase=true in the 'next' branch and also in the 'master'
branch in recent days.

I'm not surprised it happens with recent versions, but I'd expect this
to have happened with older versions too.  Is this not reproducible
with git-2.32.0 or older git versions?

  > git version
  git version 2.32.0.432.gabb21c7263
  > git config -l | grep pull
  pull.rebase=true
  pull.ff=false

So, you have conflicting configuration options set.  pull.ff=false
maps to --no-ff which is documented to create a merge.
pull.rebase=true maps to --rebase which says to run a rebase.

You probably want to drop one of these.

But a git pull would still run fast-forward.
Some of our users (including myself) rely on disabling fast-forward to emit the
per-file change log summary after each git-pull

  Updating 245f278cb729..5e8d960db7b3
  Fast-forward
   some/file/dir.ext         | 44 ++++++++++++++++++++++++++++++++++++++++++++
   another/file/dir.ext     |  6 +++---
  2 files changed, 47 insertions(+), 3 deletions(-)

In a big, fast moving monorepo, this summary is a lot of noise and
switching to pull.rebase=true
used to be the way to turn it off. If the change is intended for next
version release, is there a
workaround for this?

Thanks for the report.  This particular commit has not yet been picked
up, not even in seen.  But it's a good example of how conflicting
configuration really ought to result in an error rather than randomly
picking one to trump, and suggests why we should complete the patch.

However, since I'm commenting on this and the stat information appears
to be important to you, note that there are also merge.stat and
rebase.stat configuration variables for controlling whether those are
shown at the end of merge and rebase operations.

Hope that helps,
Elijah

Elijah Newren wrote:

On Wed, Jul 14, 2021 at 1:37 AM Son Luong Ngoc [off-list ref] wrote:

quoted

I am out of the loop in this thread but I have been seeing strange behaviors
with pull.rebase=true in the 'next' branch and also in the 'master'
branch in recent days.

I'm not surprised it happens with recent versions, but I'd expect this
to have happened with older versions too.  Is this not reproducible
with git-2.32.0 or older git versions?

I already provided an accurate target [1].

quoted

  > git version
  git version 2.32.0.432.gabb21c7263
  > git config -l | grep pull
  pull.rebase=true
  pull.ff=false

So, you have conflicting configuration options set.  pull.ff=false
maps to --no-ff which is documented to create a merge.
pull.rebase=true maps to --rebase which says to run a rebase.

You probably want to drop one of these.

`pull.ff` will be honored by `git pull --merge`.

[1] https://lore.kernel.org/git/60eeff69293fb_10e52087a@natae.notmuch/

-- 
Felipe Contreras