[PATCH 0/5] Handle conflicting pull options

[PATCH 0/5] Handle conflicting pull options

[Elijah Newren via GitGitGadget]

We've recently discussed how to handle conflicting configuration and command
line options in git pull, including at least a few different
proposals[1][2][3] to handle different subsets of the possibilities. We also
have a user report from someone who had conflicting configuration and got
surprised when one of the options overruled the other -- with no warning
from the command or the documentation that such would happen. Here's my
attempt to impose clear and simple rules, which can be seen in the commit
message of the third patch.

(The first two patches are just preparatory changes to make patch 3 easier
to read.)

Since the handling of conflicting options was holding up two of Alex's
patches[4][5], I also include those two patches at the end of my series,
though I've made quite a few changes and additions to the latter of those.

Possible areas of concern:

 * Documentation/git-pull.txt includes merge-options.txt. While git-rebase
   supports many of those "merge" options, I suspect there are others that
   it does not support. We are probably silently ignoring those unsupported
   options whenever one of those is specified at the same time a rebase is
   requested; we should instead likely error out and report the
   incompatibility. I have not yet addressed that, as I was focused on the
   main rebase vs. merge incompatibility and default warning/error
   associated with it.

[1]
https://lore.kernel.org/git/00e246b1-c712-e6a5-5c27-89127d796098@gmail.com/
[2] https://lore.kernel.org/git/xmqq8s2b489p.fsf@gitster.g/ [3]
https://lore.kernel.org/git/CABPp-BERS0iiiVhSsSs6dkqzBVTQgwJUjjKaZQEzRDGRUdObcQ@mail.gmail.com/
[4]
https://lore.kernel.org/git/20210711012604.947321-1-alexhenrie24@gmail.com/
[5]
https://lore.kernel.org/git/20210627000855.530985-1-alexhenrie24@gmail.com/

Alex Henrie (1):
  pull: abort if --ff-only is given and fast-forwarding is impossible

Elijah Newren (4):
  pull: move definitions of parse_config_rebase and parse_opt_rebase
  pull: convert OPT_PASSTHRU for fast-forward options to OPT_CALLBACK
  pull: handle conflicting rebase/merge options via last option wins
  pull: abort by default when fast-forwarding is not possible

 Documentation/config/pull.txt |   3 +-
 Documentation/git-pull.txt    |  19 +++--
 advice.c                      |   5 ++
 advice.h                      |   1 +
 builtin/merge.c               |   2 +-
 builtin/pull.c                | 144 ++++++++++++++++++++--------------
 t/t4013-diff-various.sh       |   2 +-
 t/t5520-pull.sh               |  20 ++---
 t/t5521-pull-options.sh       |   4 +-
 t/t5524-pull-msg.sh           |   4 +-
 t/t5553-set-upstream.sh       |  14 ++--
 t/t5604-clone-reference.sh    |   4 +-
 t/t6402-merge-rename.sh       |  18 ++---
 t/t6409-merge-subtree.sh      |   6 +-
 t/t6417-merge-ours-theirs.sh  |  10 +--
 t/t7601-merge-pull-config.sh  |  97 ++++++++++++++++++++++-
 t/t7603-merge-reduce-heads.sh |   2 +-
 17 files changed, 242 insertions(+), 113 deletions(-)


base-commit: 75ae10bc75336db031ee58d13c5037b929235912
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1047%2Fnewren%2Fhandle-conflicting-pull-options-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1047/newren/handle-conflicting-pull-options-v1
Pull-Request: https://github.com/git/git/pull/1047
-- 
gitgitgadget

[PATCH 1/5] pull: move definitions of parse_config_rebase and parse_opt_rebase

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

No code changes, just moving these so a subsequent change will have
access to some variables.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c | 72 +++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f810843..61c68e4143f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,42 +27,6 @@
 #include "commit-reach.h"
 #include "sequencer.h"
 
-/**
- * Parses the value of --rebase. If value is a false value, returns
- * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
- * "merges", returns REBASE_MERGES. If value is "preserve", returns
- * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if
- * fatal is true, otherwise returns REBASE_INVALID.
- */
-static enum rebase_type parse_config_rebase(const char *key, const char *value,
-		int fatal)
-{
-	enum rebase_type v = rebase_parse_value(value);
-	if (v != REBASE_INVALID)
-		return v;
-
-	if (fatal)
-		die(_("Invalid value for %s: %s"), key, value);
-	else
-		error(_("Invalid value for %s: %s"), key, value);
-
-	return REBASE_INVALID;
-}
-
-/**
- * Callback for --rebase, which parses arg with parse_config_rebase().
- */
-static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
-{
-	enum rebase_type *value = opt->value;
-
-	if (arg)
-		*value = parse_config_rebase("--rebase", arg, 0);
-	else
-		*value = unset ? REBASE_FALSE : REBASE_TRUE;
-	return *value == REBASE_INVALID ? -1 : 0;
-}
-
 static const char * const pull_usage[] = {
 	N_("git pull [<options>] [<repository> [<refspec>...]]"),
 	NULL
@@ -112,6 +76,42 @@ static int opt_show_forced_updates = -1;
 static char *set_upstream;
 static struct strvec opt_fetch = STRVEC_INIT;
 
+/**
+ * Parses the value of --rebase. If value is a false value, returns
+ * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
+ * "merges", returns REBASE_MERGES. If value is "preserve", returns
+ * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if
+ * fatal is true, otherwise returns REBASE_INVALID.
+ */
+static enum rebase_type parse_config_rebase(const char *key, const char *value,
+		int fatal)
+{
+	enum rebase_type v = rebase_parse_value(value);
+	if (v != REBASE_INVALID)
+		return v;
+
+	if (fatal)
+		die(_("Invalid value for %s: %s"), key, value);
+	else
+		error(_("Invalid value for %s: %s"), key, value);
+
+	return REBASE_INVALID;
+}
+
+/**
+ * Callback for --rebase, which parses arg with parse_config_rebase().
+ */
+static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)
+{
+	enum rebase_type *value = opt->value;
+
+	if (arg)
+		*value = parse_config_rebase("--rebase", arg, 0);
+	else
+		*value = unset ? REBASE_FALSE : REBASE_TRUE;
+	return *value == REBASE_INVALID ? -1 : 0;
+}
+
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
-- 
gitgitgadget

[PATCH 2/5] pull: convert OPT_PASSTHRU for fast-forward options to OPT_CALLBACK

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

This results in no functional changes, but allows us to add logic to
the callback in a subsequent commit.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 61c68e4143f..d99719403d0 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -112,6 +112,16 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset
 	return *value == REBASE_INVALID ? -1 : 0;
 }
 
+static int parse_opt_ff(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		opt_ff = "--no-ff";
+	else
+		opt_ff = xstrfmt("--%s", opt->long_name);
+
+	return 0;
+}
+
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
@@ -154,12 +164,14 @@ static struct option pull_options[] = {
 		N_("edit message before committing"),
 		PARSE_OPT_NOARG),
 	OPT_CLEANUP(&cleanup_arg),
-	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
-		N_("allow fast-forward"),
-		PARSE_OPT_NOARG),
-	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
-		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_CALLBACK_F(0, "ff", &opt_ff, NULL,
+			N_("allow fast-forward"),
+			PARSE_OPT_NOARG,
+			parse_opt_ff),
+	OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
+			N_("abort if fast-forward is not possible"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			parse_opt_ff),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
-- 
gitgitgadget

[PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren via GitGitGadget]

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.

Both sets of these flags could also be passed via configuration
options, namely pull.rebase and pull.ff.

As with elsewhere in git:
  * Make the last flag specified win
  * Treat command line flags as coming after any configuration options
  * Do not imply an order between different configuration options; if
    they conflict, just report an error.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/config/pull.txt |  3 +-
 Documentation/git-pull.txt    |  3 ++
 builtin/pull.c                | 12 +++++++
 t/t7601-merge-pull-config.sh  | 67 +++++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 1 deletion(-)

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 &&
-- 
gitgitgadget

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Eric Sunshine]

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

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren]

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

RE: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Felipe Contreras]

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

-- 
Felipe Contreras

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Junio C Hamano]

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

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Felipe Contreras]

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.

-- 
Felipe Contreras

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren]

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.

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Junio C Hamano]

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.

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren]

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.

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Junio C Hamano]

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.

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren]

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.

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Junio C Hamano]

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.

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Felipe Contreras]

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.

-- 
Felipe Contreras

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Junio C Hamano]

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"?

Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins

[Elijah Newren]

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.

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

[Alex Henrie via GitGitGadget]

From: Alex Henrie <alexhenrie24@gmail.com>

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 <alexhenrie24@gmail.com>
---
 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 0b9c89c48ab..337e8f342bc 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 bd26c385d00..16240438387 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 a8a843b1f54..aa920ac524f 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 b355fd38794..2c90bbb1588 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1070,9 +1070,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 73a0dbdf25a..6b9e80db97b 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -250,6 +250,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 &&
-- 
gitgitgadget

[PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

We have for some time shown a long warning when the user does not
specify how to reconcile divergent branches with git pull.  Make it an
error now.

Initial-patch-by: Alex Henrie <alexhenrie24@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-pull.txt    | 18 ++++++++++--------
 builtin/pull.c                | 31 +++++++++++++++----------------
 t/t4013-diff-various.sh       |  2 +-
 t/t5520-pull.sh               | 20 ++++++++++----------
 t/t5521-pull-options.sh       |  4 ++--
 t/t5524-pull-msg.sh           |  4 ++--
 t/t5553-set-upstream.sh       | 14 +++++++-------
 t/t5604-clone-reference.sh    |  4 ++--
 t/t6402-merge-rename.sh       | 18 +++++++++---------
 t/t6409-merge-subtree.sh      |  6 +++---
 t/t6417-merge-ours-theirs.sh  | 10 +++++-----
 t/t7601-merge-pull-config.sh  |  6 ++----
 t/t7603-merge-reduce-heads.sh |  2 +-
 13 files changed, 69 insertions(+), 70 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 03e8694e146..cb65d33e15e 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -15,14 +15,16 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-Incorporates changes from a remote repository into the current
-branch.  In its default mode, `git pull` is shorthand for
-`git fetch` followed by `git merge FETCH_HEAD`.
-
-More precisely, 'git pull' runs 'git fetch' with the given
-parameters and calls 'git merge' to merge the retrieved branch
-heads into the current branch.
-With `--rebase`, it runs 'git rebase' instead of 'git merge'.
+Incorporates changes from a remote repository into the current branch.
+If the current branch is behind the remote, then by default it will
+fast-forward the current branch to match the remote.  If the current
+branch and the remote have diverged, the user needs to specify how to
+reconcile the divergent branches with --no-ff or --rebase (or the
+corresponding configuration options in pull.ff or pull.rebase).
+
+More precisely, 'git pull' runs 'git fetch' with the given parameters
+and then depending on config options or command line flags, will call
+either 'git merge' or 'git rebase' to reconcile diverging branches.
 
 <repository> should be the name of a remote repository as
 passed to linkgit:git-fetch[1].  <refspec> can name an
diff --git a/builtin/pull.c b/builtin/pull.c
index 2c90bbb1588..0f8fdb7d42b 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -946,20 +946,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 	return ret;
 }
 
-static void show_advice_pull_non_ff(void)
+static void NORETURN die_pull_non_ff(void)
 {
-	advise(_("Pulling without specifying how to reconcile divergent branches is\n"
-		 "discouraged. You can squelch this message by running one of the following\n"
-		 "commands sometime before your next pull:\n"
-		 "\n"
-		 "  git config pull.rebase false  # merge (the default strategy)\n"
-		 "  git config pull.rebase true   # rebase\n"
-		 "  git config pull.ff only       # fast-forward only\n"
-		 "\n"
-		 "You can replace \"git config\" with \"git config --global\" to set a default\n"
-		 "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
-		 "or --ff-only on the command line to override the configured default per\n"
-		 "invocation.\n"));
+	die(_("You have divergent branches and need to specify how to reconcile them.\n"
+	      "You can do so by running one of the following commands sometime before\n"
+	      "your next pull:\n"
+	      "\n"
+	      "  git config pull.rebase false  # merge (the default strategy)\n"
+	      "  git config pull.rebase true   # rebase\n"
+	      "  git config pull.ff only       # fast-forward only\n"
+	      "\n"
+	      "You can replace \"git config\" with \"git config --global\" to set a default\n"
+	      "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
+	      "or --ff-only on the command line to override the configured default per\n"
+	      "invocation.\n"));
 }
 
 int cmd_pull(int argc, const char **argv, const char *prefix)
@@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (opt_ff) {
 			if (!strcmp(opt_ff, "--ff-only"))
 				die_ff_impossible();
-		} else {
-			if (rebase_unspecified && opt_verbosity >= 0)
-				show_advice_pull_non_ff();
+		} else if (rebase_unspecified) {
+			die_pull_non_ff();
 		}
 	}
 
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 7fadc985ccc..eb989f7f191 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -65,7 +65,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	git checkout master &&
-	git pull -s ours . side &&
+	git pull -s ours --no-rebase . side &&
 
 	GIT_AUTHOR_DATE="2006-06-26 00:05:00 +0000" &&
 	GIT_COMMITTER_DATE="2006-06-26 00:05:00 +0000" &&
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e2c0c510222..672001a18bd 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -136,12 +136,12 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	git reset --hard HEAD^ &&
 	echo file >expect &&
 	test_cmp expect file &&
-	git pull . second &&
+	git pull --no-rebase . second &&
 	echo modified >expect &&
 	test_cmp expect file &&
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --no-rebase . second: Fast-forward" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -226,7 +226,7 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_commit modified2 file &&
 	git ls-files -u >unmerged &&
 	test_must_be_empty unmerged &&
-	test_must_fail git pull . second &&
+	test_must_fail git pull --no-rebase . second &&
 	git ls-files -u >unmerged &&
 	test_file_not_empty unmerged &&
 	cp file expected &&
@@ -409,37 +409,37 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 
 test_expect_success 'pull succeeds with dirty working directory and merge.autostash set' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2
+	test_pull_autostash 2 --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull.rebase' '
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 63a688bdbf5..7601c919fdc 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -113,7 +113,7 @@ test_expect_success 'git pull --force' '
 	git pull two &&
 	test_commit A &&
 	git branch -f origin &&
-	git pull --all --force
+	git pull --no-rebase --all --force
 	)
 '
 
@@ -179,7 +179,7 @@ test_expect_success 'git pull --allow-unrelated-histories' '
 	(
 		cd dst &&
 		test_must_fail git pull ../src side &&
-		git pull --allow-unrelated-histories ../src side
+		git pull --no-rebase --allow-unrelated-histories ../src side
 	)
 '
 
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index c278adaa5a2..b2be3605f5a 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -28,7 +28,7 @@ test_expect_success setup '
 test_expect_success pull '
 (
 	cd cloned &&
-	git pull --log &&
+	git pull --no-rebase --log &&
 	git log -2 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result
@@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '
 	git reset --hard HEAD^ &&
 	test "$(cat afile)" = original &&
 	test "$(cat bfile)" = added &&
-	git pull --log=1 &&
+	git pull --no-rebase --log=1 &&
 	git log -3 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result &&
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index b1d614ce18c..9c12c0f8c32 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -108,27 +108,27 @@ test_expect_success 'setup commit on main and other pull' '
 
 test_expect_success 'pull --set-upstream upstream main sets branch main but not other' '
 	clear_config main other &&
-	git pull --set-upstream upstream main &&
+	git pull --no-rebase --set-upstream upstream main &&
 	check_config main upstream refs/heads/main &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream main:other2 does not set the branch other2' '
 	clear_config other2 &&
-	git pull --set-upstream upstream main:other2 &&
+	git pull --no-rebase --set-upstream upstream main:other2 &&
 	check_config_missing other2
 '
 
 test_expect_success 'pull --set-upstream upstream other sets branch main' '
 	clear_config main other &&
-	git pull --set-upstream upstream other &&
+	git pull --no-rebase --set-upstream upstream other &&
 	check_config main upstream refs/heads/other &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
 	clear_config three &&
-	git pull --tags --set-upstream upstream three &&
+	git pull --no-rebase --tags --set-upstream upstream three &&
 	check_config_missing three
 '
 
@@ -144,16 +144,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w
 
 test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
 	clear_config main other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config main upstream HEAD &&
 	git checkout other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config other upstream HEAD
 '
 
 test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
 	clear_config main three &&
-	git pull --set-upstream upstream main three &&
+	git pull --no-rebase --set-upstream upstream main three &&
 	check_config_missing main &&
 	check_config_missing three
 '
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index e845d621f61..24340e6d56e 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -87,7 +87,7 @@ test_expect_success 'updating origin' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C C pull origin
+	git -C C pull --no-rebase origin
 '
 
 # the 2 local objects are commit and tree from the merge
@@ -96,7 +96,7 @@ test_expect_success 'that alternate to origin gets used' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C D pull origin
+	git -C D pull --no-rebase origin
 '
 
 # the 5 local objects are expected; file3 blob, commit in A to add it
diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 425dad97d54..02a842697b8 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -103,7 +103,7 @@ test_expect_success 'setup' '
 test_expect_success 'pull renaming branch into unrenaming one' \
 '
 	git show-branch &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -s &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
@@ -121,7 +121,7 @@ test_expect_success 'pull renaming branch into another renaming one' \
 	rm -f B &&
 	git reset --hard &&
 	git checkout red &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
 	git ls-files -s N >n.stages &&
@@ -137,7 +137,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . main &&
+	test_expect_code 1 git pull --no-rebase . main &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
 	git ls-files -s N >n.stages &&
@@ -153,7 +153,7 @@ test_expect_success 'pull conflicting renames' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . blue &&
+	test_expect_code 1 git pull --no-rebase . blue &&
 	git ls-files -u A >a.stages &&
 	test_line_count = 1 a.stages &&
 	git ls-files -u B >b.stages &&
@@ -173,7 +173,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git reset --hard &&
 	git show-branch &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_path_is_file A
 '
 
@@ -183,7 +183,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git show-branch &&
 	rm -f A &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . red &&
+	test_expect_code 1 git pull --no-rebase . red &&
 	test_path_is_file A
 '
 
@@ -193,7 +193,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git checkout -f main &&
 	git tag -f anchor &&
 	git show-branch &&
-	git pull . yellow &&
+	git pull --no-rebase . yellow &&
 	test_path_is_missing M &&
 	git reset --hard anchor
 '
@@ -220,7 +220,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull . yellow &&
+	test_expect_code 128 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '
@@ -232,7 +232,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git tag -f anchor &&
 	git show-branch &&
 	echo >M this file should not matter &&
-	git pull . main &&
+	git pull --no-rebase . main &&
 	test_path_is_file M &&
 	! {
 		git ls-files -s |
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index d406b2343cb..ba7890ec521 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -100,7 +100,7 @@ test_expect_success 'merge update' '
 	git checkout -b topic_2 &&
 	git commit -m "update git-gui" &&
 	cd ../git &&
-	git pull -s subtree gui topic_2 &&
+	git pull --no-rebase -s subtree gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -129,7 +129,7 @@ test_expect_success 'initial ambiguous subtree' '
 test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -142,7 +142,7 @@ test_expect_success 'merge using explicit' '
 test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui2 gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o1 0	git-gui/git-gui.sh" &&
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index ac9aee9a662..ec065d6a658 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -69,11 +69,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 '
 
 test_expect_success 'pull passes -X to underlying merge' '
-	git reset --hard main && git pull -s recursive -Xours . side &&
-	git reset --hard main && git pull -s recursive -X ours . side &&
-	git reset --hard main && git pull -s recursive -Xtheirs . side &&
-	git reset --hard main && git pull -s recursive -X theirs . side &&
-	git reset --hard main && test_must_fail git pull -s recursive -X bork . side
+	git reset --hard main && git pull --no-rebase -s recursive -Xours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X ours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -Xtheirs . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X theirs . side &&
+	git reset --hard main && test_must_fail git pull --no-rebase -s recursive -X bork . side
 '
 
 test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b9e80db97b..84404f4f0c3 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -86,10 +86,8 @@ test_expect_success 'pull.rebase not set and --ff-only given' '
 
 test_expect_success 'pull.rebase not set (not-fast-forward)' '
 	git reset --hard c2 &&
-	git -c color.advice=always pull . c1 2>err &&
-	test_decode_color <err >decoded &&
-	test_i18ngrep "<YELLOW>hint: " decoded &&
-	test_i18ngrep "Pulling without specifying how to reconcile" decoded
+	test_must_fail git pull . c1 2>err &&
+	test_i18ngrep "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 98948955ae5..27cd94ad6f7 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
 
 test_expect_success 'pull c2, c3, c4, c5 into c1' '
 	git reset --hard c1 &&
-	git pull . c2 c3 c4 c5 &&
+	git pull --no-rebase . c2 c3 c4 c5 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
-- 
gitgitgadget

Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Eric Sunshine]

On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> We have for some time shown a long warning when the user does not
> specify how to reconcile divergent branches with git pull.  Make it an
> error now.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>

Teeny tiny nits below...

> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> @@ -15,14 +15,16 @@ SYNOPSIS
> -Incorporates changes from a remote repository into the current
> -branch.  In its default mode, `git pull` is shorthand for
> -`git fetch` followed by `git merge FETCH_HEAD`.
> -
> -More precisely, 'git pull' runs 'git fetch' with the given
> -parameters and calls 'git merge' to merge the retrieved branch
> -heads into the current branch.
> -With `--rebase`, it runs 'git rebase' instead of 'git merge'.

The original text has an odd mix of apostrophes and backticks...

> +Incorporates changes from a remote repository into the current branch.
> +If the current branch is behind the remote, then by default it will
> +fast-forward the current branch to match the remote.  If the current
> +branch and the remote have diverged, the user needs to specify how to
> +reconcile the divergent branches with --no-ff or --rebase (or the
> +corresponding configuration options in pull.ff or pull.rebase).
> +
> +More precisely, 'git pull' runs 'git fetch' with the given parameters
> +and then depending on config options or command line flags, will call
> +either 'git merge' or 'git rebase' to reconcile diverging branches.

... and the revised text adds "no quotes" to the selection. These
days, we'd probably backtick all of these:

    --no-ff
    --rebase
    pull.ff
    pull.rebase
    git pull
    git fetch
    git merge
    git rebase

The rest of this document is actually pretty good about using
backticks, though there are some exceptions.

There's also an odd mix of "configuration options" and "config
options" in the revised text. Perhaps stick with "configuration
options" to be a bit more formal?

>  <repository> should be the name of a remote repository as
>  passed to linkgit:git-fetch[1].  <refspec> can name an

And, as an aside, we'd backtick <repository> and <refspec>, though
your patch isn't touching that, so outside the scope of this change.

> diff --git a/builtin/pull.c b/builtin/pull.c
> @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 if (opt_ff) {
>                         if (!strcmp(opt_ff, "--ff-only"))
>                                 die_ff_impossible();
> -               } else {
> -                       if (rebase_unspecified && opt_verbosity >= 0)
> -                               show_advice_pull_non_ff();
> +               } else if (rebase_unspecified) {
> +                       die_pull_non_ff();
>                 }

When reading the previous patch, I was wondering why an `if else`
wasn't used, and here I see that it does become an `if else`. I guess
you didn't want to alter Alex's original patch?

Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Elijah Newren]

On Wed, Jul 14, 2021 at 10:19 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > We have for some time shown a long warning when the user does not
> > specify how to reconcile divergent branches with git pull.  Make it an
> > error now.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Teeny tiny nits below...
>
> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > @@ -15,14 +15,16 @@ SYNOPSIS
> > -Incorporates changes from a remote repository into the current
> > -branch.  In its default mode, `git pull` is shorthand for
> > -`git fetch` followed by `git merge FETCH_HEAD`.
> > -
> > -More precisely, 'git pull' runs 'git fetch' with the given
> > -parameters and calls 'git merge' to merge the retrieved branch
> > -heads into the current branch.
> > -With `--rebase`, it runs 'git rebase' instead of 'git merge'.
>
> The original text has an odd mix of apostrophes and backticks...
>
> > +Incorporates changes from a remote repository into the current branch.
> > +If the current branch is behind the remote, then by default it will
> > +fast-forward the current branch to match the remote.  If the current
> > +branch and the remote have diverged, the user needs to specify how to
> > +reconcile the divergent branches with --no-ff or --rebase (or the
> > +corresponding configuration options in pull.ff or pull.rebase).
> > +
> > +More precisely, 'git pull' runs 'git fetch' with the given parameters
> > +and then depending on config options or command line flags, will call
> > +either 'git merge' or 'git rebase' to reconcile diverging branches.
>
> ... and the revised text adds "no quotes" to the selection. These
> days, we'd probably backtick all of these:
>
>     --no-ff
>     --rebase
>     pull.ff
>     pull.rebase
>     git pull
>     git fetch
>     git merge
>     git rebase
>
> The rest of this document is actually pretty good about using
> backticks, though there are some exceptions.
>
> There's also an odd mix of "configuration options" and "config
> options" in the revised text. Perhaps stick with "configuration
> options" to be a bit more formal?
>
> >  <repository> should be the name of a remote repository as
> >  passed to linkgit:git-fetch[1].  <refspec> can name an
>
> And, as an aside, we'd backtick <repository> and <refspec>, though
> your patch isn't touching that, so outside the scope of this change.

I'll try to fix these up, at least those in the nearby area I'm
modifying, if others agree with the general approach towards --ff-only
& --no-ff vs. --rebase.

> > diff --git a/builtin/pull.c b/builtin/pull.c
> > @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                 if (opt_ff) {
> >                         if (!strcmp(opt_ff, "--ff-only"))
> >                                 die_ff_impossible();
> > -               } else {
> > -                       if (rebase_unspecified && opt_verbosity >= 0)
> > -                               show_advice_pull_non_ff();
> > +               } else if (rebase_unspecified) {
> > +                       die_pull_non_ff();
> >                 }
>
> When reading the previous patch, I was wondering why an `if else`
> wasn't used, and here I see that it does become an `if else`. I guess
> you didn't want to alter Alex's original patch?

Yep.  :-)

RE: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> We have for some time shown a long warning when the user does not
> specify how to reconcile divergent branches with git pull.  Make it an
> error now.

A warning doesn't imply deprecation.

Users have been warnged *zero* days that the default is going to change,
to what it's going to change, how to configure the new behavior, or how
to keep the old behavior.

NAK.

-- 
Felipe Contreras

Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 15 2021, Elijah Newren via GitGitGadget wrote:

> -static void show_advice_pull_non_ff(void)
> +static void NORETURN die_pull_non_ff(void)
>  {
> -	advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> -		 "discouraged. You can squelch this message by running one of the following\n"
> -		 "commands sometime before your next pull:\n"
> -		 "\n"
> -		 "  git config pull.rebase false  # merge (the default strategy)\n"
> -		 "  git config pull.rebase true   # rebase\n"
> -		 "  git config pull.ff only       # fast-forward only\n"
> -		 "\n"
> -		 "You can replace \"git config\" with \"git config --global\" to set a default\n"
> -		 "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> -		 "or --ff-only on the command line to override the configured default per\n"
> -		 "invocation.\n"));
> +	die(_("You have divergent branches and need to specify how to reconcile them.\n"
> +	      "You can do so by running one of the following commands sometime before\n"
> +	      "your next pull:\n"
> +	      "\n"
> +	      "  git config pull.rebase false  # merge (the default strategy)\n"
> +	      "  git config pull.rebase true   # rebase\n"
> +	      "  git config pull.ff only       # fast-forward only\n"
> +	      "\n"
> +	      "You can replace \"git config\" with \"git config --global\" to set a default\n"
> +	      "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> +	      "or --ff-only on the command line to override the configured default per\n"
> +	      "invocation.\n"));
>  }

Dying and advise() shouldn't be mutually exclusive, we should reword the
advise() message to idate for this being a die(), and then:

>  int cmd_pull(int argc, const char **argv, const char *prefix)
> @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  		if (opt_ff) {
>  			if (!strcmp(opt_ff, "--ff-only"))
>  				die_ff_impossible();
> -		} else {
> -			if (rebase_unspecified && opt_verbosity >= 0)
> -				show_advice_pull_non_ff();
> +		} else if (rebase_unspecified) {
> +			die_pull_non_ff();
>  		}

Here we should:

    show_advice_pull_non_ff();
    die(_("some much briefer summary"))

I.e. we should not being showing giantic advice-y die() messages, the
die messages should always be brief, but we might also show advice just
before dying.

Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> On Thu, Jul 15 2021, Elijah Newren via GitGitGadget wrote:

> >  int cmd_pull(int argc, const char **argv, const char *prefix)
> > @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >  		if (opt_ff) {
> >  			if (!strcmp(opt_ff, "--ff-only"))
> >  				die_ff_impossible();
> > -		} else {
> > -			if (rebase_unspecified && opt_verbosity >= 0)
> > -				show_advice_pull_non_ff();
> > +		} else if (rebase_unspecified) {
> > +			die_pull_non_ff();
> >  		}
> 
> Here we should:
> 
>     show_advice_pull_non_ff();
>     die(_("some much briefer summary"))
> 
> I.e. we should not being showing giantic advice-y die() messages, the
> die messages should always be brief, but we might also show advice just
> before dying.

Indeed, just like my proposal does:

		diverging_advice();
		die(_("The pull was not fast-forward, either merge or rebase.\n"));

-- 
Felipe Contreras

RE: [PATCH 0/5] Handle conflicting pull options

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> We've recently discussed how to handle conflicting configuration and command
> line options in git pull, including at least a few different
> proposals[1][2][3] to handle different subsets of the possibilities. We also
> have a user report from someone who had conflicting configuration and got
> surprised when one of the options overruled the other -- with no warning
> from the command or the documentation that such would happen.

No, that's not what happened, you are assuming wrongly.

It's perfectly fine to have pull.ff and pull.rebase, because the former
will be honored when running `git pull --merge`, as I already explained
[1].

[1] http://lore.kernel.org/git/60ef1f6a83b61_9460a208cc@natae.notmuch

-- 
Felipe Contreras