git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 00/19] pull: default ff-only mode
@ 2020-12-08  0:26 Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 01/19] doc: pull: explain what is a fast-forward Felipe Contreras
                   ` (19 more replies)
  0 siblings, 20 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

The old thread "Pull is Mostly Evil" [1] came to haunt us again.

This is my latest attempt to try to fix it.

This patch series is divided in 3 parts:

== Part I ==

1. Improve the current documentation
2. Improve the current default warning
3. Minimize the instances where we display the default warning
4. Add a --merge option
5. Fix the behavior of the warning with --ff

== Part II ==

1. Introduce pull.mode
2. Introduce pull.mode=ff-only
3. Advice on future changes, and suggest changing pull.mode

== Part III ==

1. Change the default mode to pull.mode=ff-only

v3 of the series only did part I, and the interdiff is only of that part.

Since then the change in semantics of --ff-only is dropped, because
that solution didn't pan out, and now I'm sending the one I think
it will: "pull.mode=ff-only".

Plus a fixed typo, and fixed a merge conflict with the latest master
(not in the interdiff).

If this is a bit overwhelming, you can check the branches in my gitlab[2].

 * fc/pull/improvements (part 1)
 * fc/pull/ff-only (part 2)
 * fc/pull/ff-only-switch (part 3)

[1] https://lore.kernel.org/git/5363BB9F.40102@xiplink.com/
[2] https://gitlab.com/felipec/git/

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index c220da143a..21b50aff77 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -44,13 +44,13 @@ Assume the following history exists and the current branch is
     D---E master
 ------------
 
-Then `git pull` will merge in a fast-foward way up to the new master.
+Then `git pull` will merge in a fast-forward way up to the new master.
 
 ------------
     D---E---A---B---C master, origin/master
 ------------
 
-However, a non-fast-foward case looks very different.
+However, a non-fast-forward case looks very different.
 
 ------------
 	  A---B---C master on origin
diff --git a/builtin/pull.c b/builtin/pull.c
index 0735c77f42..97a7657473 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -112,24 +112,6 @@ static int opt_show_forced_updates = -1;
 static char *set_upstream;
 static struct strvec opt_fetch = STRVEC_INIT;
 
-static int parse_opt_ff_only(const struct option *opt, const char *arg, int unset)
-{
-	char **value = opt->value;
-	opt_rebase = REBASE_DEFAULT;
-	free(*value);
-	*value = xstrdup_or_null("--ff-only");
-	return 0;
-}
-
-static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
-{
-	enum rebase_type *value = opt->value;
-	free(opt_ff);
-	opt_ff = NULL;
-	*value = REBASE_FALSE;
-	return 0;
-}
-
 static struct option pull_options[] = {
 	/* Shared options */
 	OPT__VERBOSITY(&opt_verbosity),
@@ -147,9 +129,8 @@ static struct option pull_options[] = {
 		"(false|true|merges|preserve|interactive)",
 		N_("incorporate changes by rebasing rather than merging"),
 		PARSE_OPT_OPTARG, parse_opt_rebase),
-	OPT_CALLBACK_F('m', "merge", &opt_rebase, NULL,
-		N_("incorporate changes by merging"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_merge),
+	OPT_SET_INT('m', "merge", &opt_rebase,
+		N_("incorporate changes by merging"), REBASE_FALSE),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
@@ -178,9 +159,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
 		N_("allow fast-forward"),
 		PARSE_OPT_NOARG),
-	OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
+	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
@@ -1027,25 +1008,19 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (!can_ff && !opt_rebase) {
-		if (opt_ff && !strcmp(opt_ff, "--ff-only"))
-			die(_("The pull was not fast-forward, please either merge or rebase."));
-
-		if (opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
-			advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
-				"you need to specify if you want a merge, a rebase, or a fast-forward.\n"
-				"You can squelch this message by running one of the following commands:\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.\n"
-				"If unsure, run \"git pull --merge\".\n"
-				"Read \"git pull --help\" for more information."
-				));
-		}
+	if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
+		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
+			"you need to specify if you want a merge, or a rebase.\n"
+			"You can squelch this message by running one of the following commands:\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.\n"
+			"If unsure, run \"git pull --merge\".\n"
+			"Read \"git pull --help\" for more information."));
 	}
 
 	if (opt_rebase >= REBASE_TRUE) {
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 0cdac4010b..9fae07cdfa 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -819,89 +819,4 @@ test_expect_success 'git pull --rebase against local branch' '
 	test_cmp expect file2
 '
 
-setup_other () {
-	test_when_finished "git checkout master && git branch -D other test" &&
-	git checkout -b other $1 &&
-	>new &&
-	git add new &&
-	git commit -m new &&
-	git checkout -b test -t other &&
-	git reset --hard master
-}
-
-setup_ff () {
-	setup_other master
-}
-
-setup_non_ff () {
-	setup_other master^
-}
-
-test_expect_success 'fast-forward (ff-only)' '
-	test_config pull.ff only &&
-	setup_ff &&
-	git pull
-'
-
-test_expect_success 'non-fast-forward (ff-only)' '
-	test_config pull.ff only &&
-	setup_non_ff &&
-	test_must_fail git pull
-'
-
-test_expect_success 'non-fast-forward with merge (ff-only)' '
-	test_config pull.ff only &&
-	setup_non_ff &&
-	git pull --merge
-'
-
-test_expect_success 'non-fast-forward with rebase (ff-only)' '
-	test_config pull.ff only &&
-	setup_non_ff &&
-	git pull --rebase
-'
-
-test_expect_success 'non-fast-forward error message (ff-only)' '
-	test_config pull.ff only &&
-	setup_non_ff &&
-	test_must_fail git pull 2> error &&
-	cat error &&
-	grep -q "The pull was not fast-forward" error
-'
-
-test_expect_success '--merge overrides --ff-only' '
-	setup_non_ff &&
-	git pull --ff-only --merge
-'
-
-test_expect_success '--rebase overrides --ff-only' '
-	setup_non_ff &&
-	git pull --ff-only --rebase
-'
-
-test_expect_success '--ff-only overrides --merge' '
-	setup_non_ff &&
-	test_must_fail git pull --merge --ff-only
-'
-
-test_expect_success '--ff-only overrides pull.rebase=false' '
-	test_config pull.rebase false &&
-	setup_non_ff &&
-	test_must_fail git pull --ff-only
-'
-
-test_expect_success 'pull.rebase=true overrides pull.ff=only' '
-	test_config pull.ff only &&
-	test_config pull.rebase true &&
-	setup_non_ff &&
-	git pull
-'
-
-test_expect_success 'pull.rebase=false overrides pull.ff=only' '
-	test_config pull.ff only &&
-	test_config pull.rebase false &&
-	setup_non_ff &&
-	test_must_fail git pull
-'
-
 test_done


Felipe Contreras (19):
  doc: pull: explain what is a fast-forward
  pull: improve default warning
  pull: refactor fast-forward check
  pull: cleanup autostash check
  pull: trivial cleanup
  pull: move default warning
  pull: display default warning only when non-ff
  pull: trivial whitespace style fix
  pull: introduce --merge option
  pull: show warning with --ff
  rebase: add REBASE_DEFAULT
  pull: move configurations fetches
  test: merge-pull-config: trivial cleanup
  test: pull-options: revert unnecessary changes
  pull: trivial memory fix
  pull: add pull.mode
  pull: add pull.mode=ff-only
  pull: advice of future changes
  future: pull: enable ff-only mode by default

 Documentation/config/branch.txt |   6 ++
 Documentation/config/pull.txt   |   6 ++
 Documentation/git-pull.txt      |  24 ++++-
 builtin/pull.c                  | 157 +++++++++++++++++++++-----------
 builtin/remote.c                |  22 ++++-
 rebase.c                        |  12 +++
 rebase.h                        |  13 ++-
 t/t5520-pull.sh                 |  87 ++++++++++++++++++
 t/t5521-pull-options.sh         |  22 ++---
 t/t7601-merge-pull-config.sh    |  60 ------------
 10 files changed, 282 insertions(+), 127 deletions(-)

-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 01/19] doc: pull: explain what is a fast-forward
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 02/19] pull: improve default warning Felipe Contreras
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

We want users to know what is a fast-forward in order to understand the
default warning.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c01..e1605a81b3 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -38,6 +38,20 @@ as set by linkgit:git-branch[1] `--track`.
 Assume the following history exists and the current branch is
 "`master`":
 
+------------
+	  A---B---C master on origin
+	 /
+    D---E master
+------------
+
+Then `git pull` will merge in a fast-forward way up to the new master.
+
+------------
+    D---E---A---B---C master, origin/master
+------------
+
+However, a non-fast-forward case looks very different.
+
 ------------
 	  A---B---C master on origin
 	 /
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 02/19] pull: improve default warning
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 01/19] doc: pull: explain what is a fast-forward Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 03/19] pull: refactor fast-forward check Felipe Contreras
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

We want to:

1. Be clear about what "specifying" means; merge, or rebase.
2. Mention a direct shortcut for people that just want to get on with
   their lives: git pull --no-rebase.
3. Have a quick reference for users to understand what this
   "fast-forward" business means.

This patch does all three.

Thanks to the previous patch now "git pull --help" explains what a
fast-forward is, and a further patch changes --no-rebase to --merge so
it's actually user friendly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index aa56ebcdd0..d6e707f8f9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -345,18 +345,18 @@ static enum rebase_type config_get_rebase(void)
 		return parse_config_rebase("pull.rebase", value, 1);
 
 	if (opt_verbosity >= 0 && !opt_ff) {
-		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"));
+		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
+			"you need to specify if you want a merge, or a rebase.\n"
+			"You can squelch this message by running one of the following commands:\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.\n"
+			"If unsure, run \"git pull --no-rebase\".\n"
+			"Read \"git pull --help\" for more information."));
 	}
 
 	return REBASE_FALSE;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 03/19] pull: refactor fast-forward check
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 01/19] doc: pull: explain what is a fast-forward Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 02/19] pull: improve default warning Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 04/19] pull: cleanup autostash check Felipe Contreras
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

This way we will be able to do the check unconditionally (merge or
rebase).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d6e707f8f9..e2964a0b5f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -924,6 +924,20 @@ static int run_rebase(const struct object_id *newbase,
 	return ret;
 }
 
+static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head)
+{
+	int ret;
+	struct commit_list *list = NULL;
+	struct commit *merge_head, *head;
+
+	head = lookup_commit_reference(the_repository, orig_head);
+	commit_list_insert(head, &list);
+	merge_head = lookup_commit_reference(the_repository, orig_merge_head);
+	ret = repo_is_descendant_of(the_repository, merge_head, list);
+	free_commit_list(list);
+	return ret;
+}
+
 int cmd_pull(int argc, const char **argv, const char *prefix)
 {
 	const char *repo, **refspecs;
@@ -1040,22 +1054,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
 		if (!autostash) {
-			struct commit_list *list = NULL;
-			struct commit *merge_head, *head;
-
-			head = lookup_commit_reference(the_repository,
-						       &orig_head);
-			commit_list_insert(head, &list);
-			merge_head = lookup_commit_reference(the_repository,
-							     &merge_heads.oid[0]);
-			if (repo_is_descendant_of(the_repository,
-						  merge_head, list)) {
+			if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
 				/* we can fast-forward this without invoking rebase */
 				opt_ff = "--ff-only";
 				ran_ff = 1;
 				ret = run_merge();
 			}
-			free_commit_list(list);
 		}
 		if (!ran_ff)
 			ret = run_rebase(&newbase, &upstream);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 04/19] pull: cleanup autostash check
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (2 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 03/19] pull: refactor fast-forward check Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 05/19] pull: trivial cleanup Felipe Contreras
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Currently "git pull --rebase" takes a shortcut in the case a
fast-forward merge is possible; run_merge() is called with --ff-only.

However, "git merge" didn't have an --autostash option, so, when "git
pull --rebase --autostash" was called *and* the fast-forward merge
shortcut was taken, then the pull failed.

This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
works in dirty repo, 2017-06-01) by simply skipping the fast-forward
merge shortcut.

Later on "git merge" learned the --autostash option [a03b55530a
(merge: teach --autostash option, 2020-04-07)], and so did "git pull"
[d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].

Therefore it's not necessary to skip the fast-forward merge shortcut
anymore when called with --rebase --autostash.

Let's always take the fast-forward merge shortcut by essentially
reverting f15e7cf5cc.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index e2964a0b5f..d44ca2ffde 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -944,7 +944,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct oid_array merge_heads = OID_ARRAY_INIT;
 	struct object_id orig_head, curr_head;
 	struct object_id rebase_fork_point;
-	int autostash;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -977,8 +976,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	autostash = config_autostash;
 	if (opt_rebase) {
+		int autostash = config_autostash;
 		if (opt_autostash != -1)
 			autostash = opt_autostash;
 
@@ -1053,13 +1052,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
-		if (!autostash) {
-			if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
-				/* we can fast-forward this without invoking rebase */
-				opt_ff = "--ff-only";
-				ran_ff = 1;
-				ret = run_merge();
-			}
+		if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			ran_ff = 1;
+			ret = run_merge();
 		}
 		if (!ran_ff)
 			ret = run_rebase(&newbase, &upstream);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 05/19] pull: trivial cleanup
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (3 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 04/19] pull: cleanup autostash check Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 06/19] pull: move default warning Felipe Contreras
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

There's no need to store ran_ff. Now it's obvious from the conditionals.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d44ca2ffde..6aeca4aeae 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1041,7 +1041,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	if (opt_rebase) {
 		int ret = 0;
-		int ran_ff = 0;
 
 		struct object_id newbase;
 		struct object_id upstream;
@@ -1052,14 +1051,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
+
 		if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
 			/* we can fast-forward this without invoking rebase */
 			opt_ff = "--ff-only";
-			ran_ff = 1;
 			ret = run_merge();
-		}
-		if (!ran_ff)
+		} else {
 			ret = run_rebase(&newbase, &upstream);
+		}
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 06/19] pull: move default warning
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (4 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 05/19] pull: trivial cleanup Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 07/19] pull: display default warning only when non-ff Felipe Contreras
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Eventually we want to be able to display the warning only when
fast-forward merges are not possible.

In order to do so we need to move the default warning up to the point
where we can check if we can fast-forward or not.

Additionally, config_get_rebase() was probably never its true home.

This requires a temporary variable to check if we are in the
"default mode" (no --rebase or --no-rebase specified). But this is only
temporary; another patch in the series gets rid of that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 6aeca4aeae..3b84ebf100 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,6 +27,8 @@
 #include "commit-reach.h"
 #include "sequencer.h"
 
+static int default_mode;
+
 /**
  * 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
@@ -344,20 +346,7 @@ static enum rebase_type config_get_rebase(void)
 	if (!git_config_get_value("pull.rebase", &value))
 		return parse_config_rebase("pull.rebase", value, 1);
 
-	if (opt_verbosity >= 0 && !opt_ff) {
-		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
-			"you need to specify if you want a merge, or a rebase.\n"
-			"You can squelch this message by running one of the following commands:\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.\n"
-			"If unsure, run \"git pull --no-rebase\".\n"
-			"Read \"git pull --help\" for more information."));
-	}
+	default_mode = 1;
 
 	return REBASE_FALSE;
 }
@@ -944,6 +933,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct oid_array merge_heads = OID_ARRAY_INIT;
 	struct object_id orig_head, curr_head;
 	struct object_id rebase_fork_point;
+	int can_ff;
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1039,6 +1029,23 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_rebase && merge_heads.nr > 1)
 		die(_("Cannot rebase onto multiple branches."));
 
+	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+
+	if (default_mode && opt_verbosity >= 0 && !opt_ff) {
+		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
+			"you need to specify if you want a merge, or a rebase.\n"
+			"You can squelch this message by running one of the following commands:\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.\n"
+			"If unsure, run \"git pull --no-rebase\".\n"
+			"Read \"git pull --help\" for more information."));
+	}
+
 	if (opt_rebase) {
 		int ret = 0;
 
@@ -1052,7 +1059,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
 
-		if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
+		if (can_ff) {
 			/* we can fast-forward this without invoking rebase */
 			opt_ff = "--ff-only";
 			ret = run_merge();
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 07/19] pull: display default warning only when non-ff
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (5 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 06/19] pull: move default warning Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 08/19] pull: trivial whitespace style fix Felipe Contreras
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

There's no need to display the annoying warning on every pull... only
the ones that are not fast-forward.

This requires the tests to pick another base, so the merge is not
fast-forward. And in the cases where --ff-only is specified add
test_must_fail (since now they are non-fast-forward).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt   |  3 +++
 builtin/pull.c               |  2 +-
 t/t7601-merge-pull-config.sh | 28 +++++++++++++++++-----------
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index e1605a81b3..2fb184ab5f 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -60,6 +60,9 @@ However, a non-fast-forward case looks very different.
 	origin/master in your repository
 ------------
 
+By default `git pull` will warn about these situations, however, most likely
+you would want to force a merge, which you can do with `git pull --no-rebase`.
+
 Then "`git pull`" will fetch and replay the changes from the remote
 `master` branch since it diverged from the local `master` (i.e., `E`)
 until its current commit (`C`) on top of `master` and record the
diff --git a/builtin/pull.c b/builtin/pull.c
index 3b84ebf100..ab410d675f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1031,7 +1031,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (default_mode && opt_verbosity >= 0 && !opt_ff) {
+	if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
 		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
 			"you need to specify if you want a merge, or a rebase.\n"
 			"You can squelch this message by running one of the following commands:\n"
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6774e9d86f..6b4adab8b1 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -28,7 +28,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'pull.rebase not set' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	git -c color.advice=always pull . c1 2>err &&
 	test_decode_color <err >decoded &&
 	test_i18ngrep "<YELLOW>hint: " decoded &&
@@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
 
 '
 
-test_expect_success 'pull.rebase not set and pull.ff=true' '
+test_expect_success 'pull.rebase not set (fast-forward)' '
 	git reset --hard c0 &&
+	git pull . c1 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'pull.rebase not set and pull.ff=true' '
+	git reset --hard c2 &&
 	test_config pull.ff true &&
 	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	test_config pull.ff only &&
-	git pull . c1 2>err &&
+	test_must_fail git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	git pull --rebase . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	git pull --no-rebase . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	git pull --ff . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
-	git reset --hard c0 &&
+	git reset --hard c2 &&
 	git pull --no-ff . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
-	git reset --hard c0 &&
-	git pull --ff-only . c1 2>err &&
+	git reset --hard c2 &&
+	test_must_fail git pull --ff-only . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 08/19] pull: trivial whitespace style fix
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (6 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 07/19] pull: display default warning only when non-ff Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 09/19] pull: introduce --merge option Felipe Contreras
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Two spaces unaligned to anything is not part of the coding-style. A
single tab is.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index ab410d675f..4c91dd291b 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -128,9 +128,9 @@ static struct option pull_options[] = {
 	/* Options passed to git-merge or git-rebase */
 	OPT_GROUP(N_("Options related to merging")),
 	OPT_CALLBACK_F('r', "rebase", &opt_rebase,
-	  "(false|true|merges|preserve|interactive)",
-	  N_("incorporate changes by rebasing rather than merging"),
-	  PARSE_OPT_OPTARG, parse_opt_rebase),
+		"(false|true|merges|preserve|interactive)",
+		N_("incorporate changes by rebasing rather than merging"),
+		PARSE_OPT_OPTARG, parse_opt_rebase),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 09/19] pull: introduce --merge option
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (7 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 08/19] pull: trivial whitespace style fix Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 10/19] pull: show warning with --ff Felipe Contreras
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Previously --no-rebase (which still works for backwards compatibility).

Now we can update the default warning, and the git-pull(1) man page to
use --merge instead of the non-intuitive --no-rebase.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt   | 9 ++++++---
 builtin/pull.c               | 4 +++-
 t/t7601-merge-pull-config.sh | 4 ++--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 2fb184ab5f..21b50aff77 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -61,7 +61,7 @@ However, a non-fast-forward case looks very different.
 ------------
 
 By default `git pull` will warn about these situations, however, most likely
-you would want to force a merge, which you can do with `git pull --no-rebase`.
+you would want to force a merge, which you can do with `git pull --merge`.
 
 Then "`git pull`" will fetch and replay the changes from the remote
 `master` branch since it diverged from the local `master` (i.e., `E`)
@@ -148,8 +148,11 @@ It rewrites history, which does not bode well when you
 published that history already.  Do *not* use this option
 unless you have read linkgit:git-rebase[1] carefully.
 
---no-rebase::
-	Override earlier --rebase.
+-m::
+--merge::
+	Force a merge.
++
+Previously this was --no-rebase, but that usage has been deprecated.
 
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/pull.c b/builtin/pull.c
index 4c91dd291b..3874434421 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -131,6 +131,8 @@ static struct option pull_options[] = {
 		"(false|true|merges|preserve|interactive)",
 		N_("incorporate changes by rebasing rather than merging"),
 		PARSE_OPT_OPTARG, parse_opt_rebase),
+	OPT_SET_INT('m', "merge", &opt_rebase,
+		N_("incorporate changes by merging"), REBASE_FALSE),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
@@ -1042,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			"\n"
 			"You can replace \"git config\" with \"git config --global\" to set a default\n"
 			"preference for all repositories.\n"
-			"If unsure, run \"git pull --no-rebase\".\n"
+			"If unsure, run \"git pull --merge\".\n"
 			"Read \"git pull --help\" for more information."));
 	}
 
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b4adab8b1..1de64e6cc5 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -69,9 +69,9 @@ test_expect_success 'pull.rebase not set and --rebase given' '
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
-test_expect_success 'pull.rebase not set and --no-rebase given' '
+test_expect_success 'pull.rebase not set and --merge given' '
 	git reset --hard c2 &&
-	git pull --no-rebase . c1 2>err &&
+	git pull --merge . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 10/19] pull: show warning with --ff
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (8 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 09/19] pull: introduce --merge option Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 11/19] rebase: add REBASE_DEFAULT Felipe Contreras
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

It's unclear why --ff should remove the warning, since:

  git pull --ff

Is implicitly the same as:

  git pull

Unless of course pull.ff is specified otherwise.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c               | 2 +-
 t/t7601-merge-pull-config.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 3874434421..f5bdae680f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1033,7 +1033,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
+	if (default_mode && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
 		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
 			"you need to specify if you want a merge, or a rebase.\n"
 			"You can squelch this message by running one of the following commands:\n"
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 1de64e6cc5..d709799f8b 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -46,7 +46,7 @@ test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c2 &&
 	test_config pull.ff true &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
@@ -78,7 +78,7 @@ test_expect_success 'pull.rebase not set and --merge given' '
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c2 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 11/19] rebase: add REBASE_DEFAULT
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (9 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 10/19] pull: show warning with --ff Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 12/19] pull: move configurations fetches Felipe Contreras
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

By introducing a default we can distinguish when the user has forced an
option.

Therefore there's no need for an extra "default_mode" variable (it's the
same as opt_rebase == REBASE_DEFAULT), not is there any need to
initialize opt_rebase to an invalid value.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 24 ++++++++++--------------
 rebase.h       |  3 ++-
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index f5bdae680f..350df6f3c5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,8 +27,6 @@
 #include "commit-reach.h"
 #include "sequencer.h"
 
-static int default_mode;
-
 /**
  * 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
@@ -76,7 +74,7 @@ static char *opt_progress;
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 
 /* Options passed to git-merge or git-rebase */
-static enum rebase_type opt_rebase = -1;
+static enum rebase_type opt_rebase;
 static char *opt_diffstat;
 static char *opt_log;
 static char *opt_signoff;
@@ -348,9 +346,7 @@ static enum rebase_type config_get_rebase(void)
 	if (!git_config_get_value("pull.rebase", &value))
 		return parse_config_rebase("pull.rebase", value, 1);
 
-	default_mode = 1;
-
-	return REBASE_FALSE;
+	return REBASE_DEFAULT;
 }
 
 /**
@@ -445,7 +441,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 	const char *remote = curr_branch ? curr_branch->remote_name : NULL;
 
 	if (*refspecs) {
-		if (opt_rebase)
+		if (opt_rebase >= REBASE_TRUE)
 			fprintf_ln(stderr, _("There is no candidate for rebasing against among the refs that you just fetched."));
 		else
 			fprintf_ln(stderr, _("There are no candidates for merging among the refs that you just fetched."));
@@ -458,7 +454,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			repo);
 	} else if (!curr_branch) {
 		fprintf_ln(stderr, _("You are not currently on a branch."));
-		if (opt_rebase)
+		if (opt_rebase >= REBASE_TRUE)
 			fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
 		else
 			fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
@@ -473,7 +469,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs
 			remote_name = _("<remote>");
 
 		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
-		if (opt_rebase)
+		if (opt_rebase >= REBASE_TRUE)
 			fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
 		else
 			fprintf_ln(stderr, _("Please specify which branch you want to merge with."));
@@ -956,7 +952,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!opt_ff)
 		opt_ff = xstrdup_or_null(config_get_ff());
 
-	if (opt_rebase < 0)
+	if (!opt_rebase)
 		opt_rebase = config_get_rebase();
 
 	if (read_cache_unmerged())
@@ -968,7 +964,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	if (opt_rebase) {
+	if (opt_rebase >= REBASE_TRUE) {
 		int autostash = config_autostash;
 		if (opt_autostash != -1)
 			autostash = opt_autostash;
@@ -1028,12 +1024,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(merge_heads.oid, &curr_head);
 	}
-	if (opt_rebase && merge_heads.nr > 1)
+	if (opt_rebase >= REBASE_TRUE && merge_heads.nr > 1)
 		die(_("Cannot rebase onto multiple branches."));
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (default_mode && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
+	if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
 		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
 			"you need to specify if you want a merge, or a rebase.\n"
 			"You can squelch this message by running one of the following commands:\n"
@@ -1048,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			"Read \"git pull --help\" for more information."));
 	}
 
-	if (opt_rebase) {
+	if (opt_rebase >= REBASE_TRUE) {
 		int ret = 0;
 
 		struct object_id newbase;
diff --git a/rebase.h b/rebase.h
index cc723d4748..34d4acfd74 100644
--- a/rebase.h
+++ b/rebase.h
@@ -3,7 +3,8 @@
 
 enum rebase_type {
 	REBASE_INVALID = -1,
-	REBASE_FALSE = 0,
+	REBASE_DEFAULT = 0,
+	REBASE_FALSE,
 	REBASE_TRUE,
 	REBASE_PRESERVE,
 	REBASE_MERGES,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 12/19] pull: move configurations fetches
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (10 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 11/19] rebase: add REBASE_DEFAULT Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 13/19] test: merge-pull-config: trivial cleanup Felipe Contreras
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Now that we have FETCH_DEFAULT we can fetch the configuration before
parsing the argument options.

The options will override the configuration, and if they don't;
opt_rebase will remain being FETCH_DEFAULT.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 350df6f3c5..addb454e63 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -933,6 +933,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct object_id rebase_fork_point;
 	int can_ff;
 
+	opt_ff = xstrdup_or_null(config_get_ff());
+	opt_rebase = config_get_rebase();
+
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
 
@@ -949,12 +952,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
-	if (!opt_ff)
-		opt_ff = xstrdup_or_null(config_get_ff());
-
-	if (!opt_rebase)
-		opt_rebase = config_get_rebase();
-
 	if (read_cache_unmerged())
 		die_resolve_conflict("pull");
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 13/19] test: merge-pull-config: trivial cleanup
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (11 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 12/19] pull: move configurations fetches Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 14/19] test: pull-options: revert unnecessary changes Felipe Contreras
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Commit e01ae2a4a7 introduced an extra space.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t7601-merge-pull-config.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index d709799f8b..8a6aae564a 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -33,7 +33,6 @@ test_expect_success 'pull.rebase not set' '
 	test_decode_color <err >decoded &&
 	test_i18ngrep "<YELLOW>hint: " decoded &&
 	test_i18ngrep "Pulling without specifying how to reconcile" decoded
-
 '
 
 test_expect_success 'pull.rebase not set (fast-forward)' '
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 14/19] test: pull-options: revert unnecessary changes
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (12 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 13/19] test: merge-pull-config: trivial cleanup Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 15/19] pull: trivial memory fix Felipe Contreras
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Commit d18c950a69 (pull: warn if the user didn't say whether to
rebase or to merge, 2020-03-09) changed a number of tests in t5521
and added some new tests in t7601, but it was not explained why the
changes in t5521 were made.

The reason seems to be to silence the warnings while running the tests,
but we want to see the warnings if they happen.

Cc: Alex Henrie <alexhenrie24@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5521-pull-options.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index db1a381cd9..1a4fe2583a 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -11,10 +11,10 @@ test_expect_success 'setup' '
 	 git commit -m one)
 '
 
-test_expect_success 'git pull -q --no-rebase' '
+test_expect_success 'git pull -q' '
 	mkdir clonedq &&
 	(cd clonedq && git init &&
-	git pull -q --no-rebase "../parent" >out 2>err &&
+	git pull -q "../parent" >out 2>err &&
 	test_must_be_empty err &&
 	test_must_be_empty out)
 '
@@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull --no-rebase' '
+test_expect_success 'git pull' '
 	mkdir cloned &&
 	(cd cloned && git init &&
-	git pull --no-rebase "../parent" >out 2>err &&
+	git pull "../parent" >out 2>err &&
 	test -s err &&
 	test_must_be_empty out)
 '
@@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v --no-rebase' '
+test_expect_success 'git pull -v' '
 	mkdir clonedv &&
 	(cd clonedv && git init &&
-	git pull -v --no-rebase "../parent" >out 2>err &&
+	git pull -v "../parent" >out 2>err &&
 	test -s err &&
 	test_must_be_empty out)
 '
@@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '
 	test_must_be_empty out)
 '
 
-test_expect_success 'git pull -v -q --no-rebase' '
+test_expect_success 'git pull -v -q' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&
-	git pull -v -q --no-rebase "../parent" >out 2>err &&
+	git pull -v -q "../parent" >out 2>err &&
 	test_must_be_empty out &&
 	test_must_be_empty err)
 '
 
-test_expect_success 'git pull -q -v --no-rebase' '
+test_expect_success 'git pull -q -v' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
-	git pull -q -v --no-rebase "../parent" >out 2>err &&
+	git pull -q -v "../parent" >out 2>err &&
 	test_must_be_empty out &&
 	test -s err)
 '
 test_expect_success 'git pull --cleanup errors early on invalid argument' '
 	mkdir clonedcleanup &&
 	(cd clonedcleanup && git init &&
-	test_must_fail git pull --no-rebase --cleanup invalid "../parent" >out 2>err &&
+	test_must_fail git pull --cleanup invalid "../parent" >out 2>err &&
 	test_must_be_empty out &&
 	test -s err)
 '
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 15/19] pull: trivial memory fix
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (13 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 14/19] test: pull-options: revert unnecessary changes Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 16/19] pull: add pull.mode Felipe Contreras
                   ` (4 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

The opt_ff variable is supposed to have an allocated string (strdup), we
can't just overwrite it with a const char *.

Functionally it doesn't matter, since after this point opt_ff is never
freed, only accessed, but still...

It's better to be consistent.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index addb454e63..118fbdeb62 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1056,7 +1056,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (can_ff) {
 			/* we can fast-forward this without invoking rebase */
-			opt_ff = "--ff-only";
+			free(opt_ff);
+			opt_ff = xstrdup_or_null("--ff-only");
 			ret = run_merge();
 		} else {
 			ret = run_rebase(&newbase, &upstream);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 16/19] pull: add pull.mode
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (14 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 15/19] pull: trivial memory fix Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 17/19] pull: add pull.mode=ff-only Felipe Contreras
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

This way we can add more modes and the default can be something else,
namely it can be set to "ff-only", so eventually we can reject
non-fast-forward merges by default.

Also 'branch.<name>.pullmode'.

For now we simply override the rebase mode.

And when any --rebase or --merge options are specified the mode is
updated accordingly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config/branch.txt |  6 +++
 Documentation/config/pull.txt   |  6 +++
 builtin/pull.c                  | 70 +++++++++++++++++++++++++++++++--
 builtin/remote.c                | 19 ++++++++-
 rebase.c                        | 10 +++++
 rebase.h                        |  9 +++++
 t/t5520-pull.sh                 | 42 ++++++++++++++++++++
 t/t7601-merge-pull-config.sh    |  7 ++++
 8 files changed, 164 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt
index cc5f3249fc..b57bf2c308 100644
--- a/Documentation/config/branch.txt
+++ b/Documentation/config/branch.txt
@@ -96,6 +96,12 @@ mode.
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).
 
+branch.<name>.pullmode::
+	When "git pull" is run, this determines if it would either merge or
+	rebase the fetched branch. The possible values are 'merge', and
+	'rebase'. See "pull.mode" for doing this in a non
+	branch-specific manner.
+
 branch.<name>.description::
 	Branch description, can be edited with
 	`git branch --edit-description`. Branch description is
diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt
index 5404830609..f4385cde33 100644
--- a/Documentation/config/pull.txt
+++ b/Documentation/config/pull.txt
@@ -29,6 +29,12 @@ mode.
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).
 
+pull.mode::
+	When "git pull" is run, this determines if it would either merge or
+	rebase the fetched branch. The possible values are 'merge',
+	and 'rebase'. See "branch.<name>.pullmode" for setting this on a
+	per-branch basis.
+
 pull.octopus::
 	The default merge strategy to use when pulling multiple branches
 	at once.
diff --git a/builtin/pull.c b/builtin/pull.c
index 118fbdeb62..7756c8aea1 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,6 +27,8 @@
 #include "commit-reach.h"
 #include "sequencer.h"
 
+static enum pull_mode_type mode;
+
 /**
  * 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
@@ -49,6 +51,14 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value,
 	return REBASE_INVALID;
 }
 
+static enum pull_mode_type parse_config_pull_mode(const char *key, const char *value)
+{
+	enum pull_mode_type v = pull_mode_parse_value(value);
+	if (v == PULL_MODE_INVALID)
+		die(_("Invalid value for %s: %s"), key, value);
+	return v;
+}
+
 /**
  * Callback for --rebase, which parses arg with parse_config_rebase().
  */
@@ -60,9 +70,21 @@ 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;
+
+	if (*value > 0)
+		mode = *value >= REBASE_TRUE ? PULL_MODE_REBASE : PULL_MODE_MERGE;
+
 	return *value == REBASE_INVALID ? -1 : 0;
 }
 
+static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
+{
+	enum rebase_type *value = opt->value;
+	mode = PULL_MODE_MERGE;
+	*value = REBASE_FALSE;
+	return 0;
+}
+
 static const char * const pull_usage[] = {
 	N_("git pull [<options>] [<repository> [<refspec>...]]"),
 	NULL
@@ -129,8 +151,9 @@ static struct option pull_options[] = {
 		"(false|true|merges|preserve|interactive)",
 		N_("incorporate changes by rebasing rather than merging"),
 		PARSE_OPT_OPTARG, parse_opt_rebase),
-	OPT_SET_INT('m', "merge", &opt_rebase,
-		N_("incorporate changes by merging"), REBASE_FALSE),
+	OPT_CALLBACK_F('m', "merge", &opt_rebase, NULL,
+		N_("incorporate changes by merging"),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_merge),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
@@ -349,6 +372,29 @@ static enum rebase_type config_get_rebase(void)
 	return REBASE_DEFAULT;
 }
 
+static enum pull_mode_type config_get_pull_mode(void)
+{
+	struct branch *curr_branch = branch_get("HEAD");
+	const char *value;
+
+	if (curr_branch) {
+		char *key = xstrfmt("branch.%s.pullmode", curr_branch->name);
+
+		if (!git_config_get_value(key, &value)) {
+			enum pull_mode_type ret = parse_config_pull_mode(key, value);
+			free(key);
+			return ret;
+		}
+
+		free(key);
+	}
+
+	if (!git_config_get_value("pull.mode", &value))
+		return parse_config_pull_mode("pull.mode", value);
+
+	return PULL_MODE_DEFAULT;
+}
+
 /**
  * Read config variables.
  */
@@ -935,6 +981,22 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	opt_ff = xstrdup_or_null(config_get_ff());
 	opt_rebase = config_get_rebase();
+	mode = config_get_pull_mode();
+
+	if (!opt_rebase && mode) {
+		switch (mode) {
+		case PULL_MODE_MERGE:
+			opt_rebase = REBASE_FALSE;
+			break;
+		case PULL_MODE_REBASE:
+			opt_rebase = REBASE_TRUE;
+			break;
+		default:
+			break;
+		}
+	} else if (opt_rebase > 0) {
+		mode = opt_rebase >= REBASE_TRUE ? PULL_MODE_REBASE : PULL_MODE_MERGE;
+	}
 
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
@@ -1031,8 +1093,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			"you need to specify if you want a merge, or a rebase.\n"
 			"You can squelch this message by running one of the following commands:\n"
 			"\n"
-			"  git config pull.rebase false  # merge (the default strategy)\n"
-			"  git config pull.rebase true   # rebase\n"
+			"  git config pull.mode merge    # (the default strategy)\n"
+			"  git config pull.mode 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"
diff --git a/builtin/remote.c b/builtin/remote.c
index c1b211b272..51b1e675e3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -269,7 +269,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 	char *name;
 	struct string_list_item *item;
 	struct branch_info *info;
-	enum { REMOTE, MERGE, REBASE, PUSH_REMOTE } type;
+	enum { REMOTE, MERGE, REBASE, PUSH_REMOTE, PULL_MODE } type;
 	size_t key_len;
 
 	if (!starts_with(key, "branch."))
@@ -284,6 +284,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		type = REBASE;
 	else if (strip_suffix(key, ".pushremote", &key_len))
 		type = PUSH_REMOTE;
+	else if (strip_suffix(key, ".pullmode", &key_len))
+		type = PULL_MODE;
 	else
 		return 0;
 	name = xmemdupz(key, key_len);
@@ -324,6 +326,21 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 			warning(_("more than one %s"), orig_key);
 		info->push_remote_name = xstrdup(value);
 		break;
+	case PULL_MODE: {
+		int mode = pull_mode_parse_value(value);
+		switch (mode) {
+		case PULL_MODE_MERGE:
+			info->rebase = REBASE_FALSE;
+			break;
+		case PULL_MODE_REBASE:
+			info->rebase = REBASE_TRUE;
+			break;
+		default:
+			info->rebase = REBASE_INVALID;
+			break;
+		}
+		break;
+	}
 	default:
 		BUG("unexpected type=%d", type);
 	}
diff --git a/rebase.c b/rebase.c
index f8137d859b..bdfca49886 100644
--- a/rebase.c
+++ b/rebase.c
@@ -33,3 +33,13 @@ enum rebase_type rebase_parse_value(const char *value)
 
 	return REBASE_INVALID;
 }
+
+enum pull_mode_type pull_mode_parse_value(const char *value)
+{
+	if (!strcmp(value, "merge") || !strcmp(value, "m"))
+		return PULL_MODE_MERGE;
+	else if (!strcmp(value, "rebase") || !strcmp(value, "r"))
+		return PULL_MODE_REBASE;
+
+	return PULL_MODE_INVALID;
+}
diff --git a/rebase.h b/rebase.h
index 34d4acfd74..5ab8f4ddd5 100644
--- a/rebase.h
+++ b/rebase.h
@@ -13,4 +13,13 @@ enum rebase_type {
 
 enum rebase_type rebase_parse_value(const char *value);
 
+enum pull_mode_type {
+	PULL_MODE_INVALID = -1,
+	PULL_MODE_DEFAULT = 0,
+	PULL_MODE_MERGE,
+	PULL_MODE_REBASE
+};
+
+enum pull_mode_type pull_mode_parse_value(const char *value);
+
 #endif /* REBASE */
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9fae07cdfa..eb0086bd1c 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -449,6 +449,16 @@ test_expect_success 'pull.rebase' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pull.mode rebase' '
+	git reset --hard before-rebase &&
+	test_config pull.mode rebase &&
+	git pull . copy &&
+	test_cmp_rev HEAD^ copy &&
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pull --autostash & pull.rebase=true' '
 	test_config pull.rebase true &&
 	test_pull_autostash 1 --autostash
@@ -523,6 +533,17 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pull.mode=merge create a new merge commit' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.mode merge &&
+	git pull . copy &&
+	test_cmp_rev HEAD^1 before-preserve-rebase &&
+	test_cmp_rev HEAD^2 copy &&
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pull.rebase=true flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
@@ -552,6 +573,16 @@ test_expect_success REBASE_P \
 	test_cmp_rev HEAD^2 keep-merge
 '
 
+test_expect_success REBASE_P \
+	'pull.rebase=preserve rebases and merges keep-merge with pull.mode' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.mode rebase &&
+	test_config pull.rebase preserve &&
+	git pull . copy &&
+	test_cmp_rev HEAD^^ copy &&
+	test_cmp_rev HEAD^2 keep-merge
+'
+
 test_expect_success 'pull.rebase=interactive' '
 	write_script "$TRASH_DIRECTORY/fake-editor" <<-\EOF &&
 	echo I was here >fake.out &&
@@ -593,6 +624,17 @@ test_expect_success '--rebase=false create a new merge commit' '
 	test_cmp expect actual
 '
 
+test_expect_success '--rebase=false create a new merge commit with pull.mode' '
+	git reset --hard before-preserve-rebase &&
+	test_config pull.mode rebase &&
+	git pull --rebase=false . copy &&
+	test_cmp_rev HEAD^1 before-preserve-rebase &&
+	test_cmp_rev HEAD^2 copy &&
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 8a6aae564a..25ca239c17 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -41,6 +41,13 @@ test_expect_success 'pull.rebase not set (fast-forward)' '
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
+test_expect_success 'pull.mode set' '
+	git reset --hard c2 &&
+	test_config pull.mode merge &&
+	git pull . c1 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c2 &&
 	test_config pull.ff true &&
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 17/19] pull: add pull.mode=ff-only
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (15 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 16/19] pull: add pull.mode Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 18/19] pull: advice of future changes Felipe Contreras
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

It is very typical for Git newcomers to inadvertently create merges and
worse; pushing them. This is one of the reasons many experienced users
prefer to avoid 'git pull', and recommend newcomers to avoid it as well.

To escape these problems--and keep 'git pull' useful--it has been
suggested that 'git pull' barfs by default if the merge is
non-fast-forward, which unfortunately would break backwards
compatibility.

This patch leaves everything in place to enable this new mode, but it
only gets enabled if the user specifically configures it;

  pull.mode = ff-only.

Later on this mode can be enabled by default.

For the full discussion you can read:

https://lore.kernel.org/git/5363BB9F.40102@xiplink.com/

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config/branch.txt |  4 +--
 Documentation/config/pull.txt   |  4 +--
 builtin/pull.c                  |  5 +++-
 builtin/remote.c                |  3 +++
 rebase.c                        |  2 ++
 rebase.h                        |  3 ++-
 t/t5520-pull.sh                 | 45 +++++++++++++++++++++++++++++++++
 7 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt
index b57bf2c308..39f60cd8f7 100644
--- a/Documentation/config/branch.txt
+++ b/Documentation/config/branch.txt
@@ -98,8 +98,8 @@ for details).
 
 branch.<name>.pullmode::
 	When "git pull" is run, this determines if it would either merge or
-	rebase the fetched branch. The possible values are 'merge', and
-	'rebase'. See "pull.mode" for doing this in a non
+	rebase the fetched branch. The possible values are 'merge',
+	'rebase', and 'ff-only'. See "pull.mode" for doing this in a non
 	branch-specific manner.
 
 branch.<name>.description::
diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt
index f4385cde33..40778d9120 100644
--- a/Documentation/config/pull.txt
+++ b/Documentation/config/pull.txt
@@ -32,8 +32,8 @@ for details).
 pull.mode::
 	When "git pull" is run, this determines if it would either merge or
 	rebase the fetched branch. The possible values are 'merge',
-	and 'rebase'. See "branch.<name>.pullmode" for setting this on a
-	per-branch basis.
+	'rebase', and 'ff-only'. See "branch.<name>.pullmode" for setting
+	this on a per-branch basis.
 
 pull.octopus::
 	The default merge strategy to use when pulling multiple branches
diff --git a/builtin/pull.c b/builtin/pull.c
index 7756c8aea1..5a67667b79 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1088,6 +1088,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
+	if (mode == PULL_MODE_FF_ONLY && !can_ff)
+		die(_("The pull was not fast-forward, please either merge or rebase.\n"));
+
 	if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
 		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
 			"you need to specify if you want a merge, or a rebase.\n"
@@ -1095,7 +1098,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			"\n"
 			"  git config pull.mode merge    # (the default strategy)\n"
 			"  git config pull.mode rebase\n"
-			"  git config pull.ff only       # fast-forward only\n"
+			"  git config pull.mode 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.\n"
diff --git a/builtin/remote.c b/builtin/remote.c
index 51b1e675e3..34d3ea9d38 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -335,6 +335,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		case PULL_MODE_REBASE:
 			info->rebase = REBASE_TRUE;
 			break;
+		case PULL_MODE_FF_ONLY:
+			info->rebase = REBASE_TRUE;
+			break;
 		default:
 			info->rebase = REBASE_INVALID;
 			break;
diff --git a/rebase.c b/rebase.c
index bdfca49886..c6233e888f 100644
--- a/rebase.c
+++ b/rebase.c
@@ -40,6 +40,8 @@ enum pull_mode_type pull_mode_parse_value(const char *value)
 		return PULL_MODE_MERGE;
 	else if (!strcmp(value, "rebase") || !strcmp(value, "r"))
 		return PULL_MODE_REBASE;
+	else if (!strcmp(value, "ff-only") || !strcmp(value, "f"))
+		return PULL_MODE_FF_ONLY;
 
 	return PULL_MODE_INVALID;
 }
diff --git a/rebase.h b/rebase.h
index 5ab8f4ddd5..432bcb55c4 100644
--- a/rebase.h
+++ b/rebase.h
@@ -17,7 +17,8 @@ enum pull_mode_type {
 	PULL_MODE_INVALID = -1,
 	PULL_MODE_DEFAULT = 0,
 	PULL_MODE_MERGE,
-	PULL_MODE_REBASE
+	PULL_MODE_REBASE,
+	PULL_MODE_FF_ONLY
 };
 
 enum pull_mode_type pull_mode_parse_value(const char *value);
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index eb0086bd1c..b5b007b175 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -861,4 +861,49 @@ test_expect_success 'git pull --rebase against local branch' '
 	test_cmp expect file2
 '
 
+setup_other () {
+	test_when_finished "git checkout master && git branch -D other test" &&
+	git checkout -b other $1 &&
+	>new &&
+	git add new &&
+	git commit -m new &&
+	git checkout -b test -t other &&
+	git reset --hard master
+}
+
+setup_ff () {
+	setup_other master
+}
+
+setup_non_ff () {
+	setup_other master^
+}
+
+test_expect_success 'fast-forward (pull.mode=ff-only)' '
+	setup_ff &&
+	git -c pull.mode=ff-only pull
+'
+
+test_expect_success 'non-fast-forward (pull.mode=ff-only)' '
+	setup_non_ff &&
+	test_must_fail git -c pull.mode=ff-only pull
+'
+
+test_expect_success 'non-fast-forward with merge (pull.mode=ff-only)' '
+	setup_non_ff &&
+	git -c pull.mode=ff-only pull --merge
+'
+
+test_expect_success 'non-fast-forward with rebase (pull.mode=ff-only)' '
+	setup_non_ff &&
+	git -c pull.mode=ff-only pull --rebase
+'
+
+test_expect_success 'non-fast-forward error message (pull.mode=ff-only)' '
+	setup_non_ff &&
+	test_must_fail git -c pull.mode=ff-only pull 2> error &&
+	cat error &&
+	test_i18ngrep "The pull was not fast-forward" error
+'
+
 test_done
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 18/19] pull: advice of future changes
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (16 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 17/19] pull: add pull.mode=ff-only Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:26 ` [PATCH v4 19/19] future: pull: enable ff-only mode by default Felipe Contreras
  2020-12-08  0:57 ` [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

Now that we have "pull.mode=ff-only", we can make it the default any
time we want to.

For now, simply explain the upcoming changes in the default warning, and
mention how to turn on the future default mode.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c               | 20 +++++++++++---------
 t/t5520-pull.sh              |  8 ++++++++
 t/t7601-merge-pull-config.sh | 22 +++++++++++-----------
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 5a67667b79..734a2c04b8 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1092,17 +1092,19 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		die(_("The pull was not fast-forward, please either merge or rebase.\n"));
 
 	if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
-		advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
-			"you need to specify if you want a merge, or a rebase.\n"
-			"You can squelch this message by running one of the following commands:\n"
+		advise(_("The pull was not fast-forward, in the future you will have to choose a merge, or a rebase.\n"
 			"\n"
-			"  git config pull.mode merge    # (the default strategy)\n"
-			"  git config pull.mode rebase\n"
-			"  git config pull.mode ff-only  # fast-forward only\n"
+			"To quell this message you have two main options:\n"
 			"\n"
-			"You can replace \"git config\" with \"git config --global\" to set a default\n"
-			"preference for all repositories.\n"
-			"If unsure, run \"git pull --merge\".\n"
+			"1. Adopt the new behavior:\n"
+			"\n"
+			"  git config --global pull.mode ff-only\n"
+			"\n"
+			"2. Maintain the current behavior:\n"
+			"\n"
+			"  git config --global pull.mode merge\n"
+			"\n"
+			"For now we will fall back to the traditional behavior (merge).\n"
 			"Read \"git pull --help\" for more information."));
 	}
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index b5b007b175..d4718dbc02 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -906,4 +906,12 @@ test_expect_success 'non-fast-forward error message (pull.mode=ff-only)' '
 	test_i18ngrep "The pull was not fast-forward" error
 '
 
+test_expect_success 'non-fast-forward warning (default)' '
+	setup_non_ff &&
+	git pull 2> error &&
+	cat error &&
+	test_i18ngrep "The pull was not fast-forward" error &&
+	test_i18ngrep "in the future you will have to choose" error
+'
+
 test_done
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 25ca239c17..149fc2a009 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -32,71 +32,71 @@ test_expect_success 'pull.rebase not set' '
 	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_i18ngrep "in the future you will have to choose" decoded
 '
 
 test_expect_success 'pull.rebase not set (fast-forward)' '
 	git reset --hard c0 &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.mode set' '
 	git reset --hard c2 &&
 	test_config pull.mode merge &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c2 &&
 	test_config pull.ff true &&
 	git pull . c1 2>err &&
-	test_i18ngrep "Pulling without specifying how to reconcile" err
+	test_i18ngrep "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
 	git reset --hard c2 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only' '
 	git reset --hard c2 &&
 	test_config pull.ff only &&
 	test_must_fail git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
 	git reset --hard c2 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and --merge given' '
 	git reset --hard c2 &&
 	git pull --merge . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c2 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep "Pulling without specifying how to reconcile" err
+	test_i18ngrep "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
 	git reset --hard c2 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
 	git reset --hard c2 &&
 	test_must_fail git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "in the future you will have to choose" err
 '
 
 test_expect_success 'merge c1 with c2' '
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v4 19/19] future: pull: enable ff-only mode by default
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (17 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 18/19] pull: advice of future changes Felipe Contreras
@ 2020-12-08  0:26 ` Felipe Contreras
  2020-12-08  0:57 ` [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:26 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

The user has been warned that this change was coming and should have
already configured his/her preference.

It's time to flip the switch and make ff-only the default.

There's no need for the annoying warning anymore.

TODO: Do we want to keep the "pull.mode=ff-only" tests and essentially run
them twice?

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt   |  4 +-
 builtin/pull.c               | 19 +---------
 t/t5520-pull.sh              | 28 +++++---------
 t/t7601-merge-pull-config.sh | 72 ------------------------------------
 4 files changed, 13 insertions(+), 110 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 21b50aff77..672371e989 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -60,8 +60,8 @@ However, a non-fast-forward case looks very different.
 	origin/master in your repository
 ------------
 
-By default `git pull` will warn about these situations, however, most likely
-you would want to force a merge, which you can do with `git pull --merge`.
+By default `git pull` will fail on these situations, you will have to
+specify whether you want `--merge` or `--rebase`.
 
 Then "`git pull`" will fetch and replay the changes from the remote
 `master` branch since it diverged from the local `master` (i.e., `E`)
diff --git a/builtin/pull.c b/builtin/pull.c
index 734a2c04b8..7ddc11a4bc 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -392,7 +392,7 @@ static enum pull_mode_type config_get_pull_mode(void)
 	if (!git_config_get_value("pull.mode", &value))
 		return parse_config_pull_mode("pull.mode", value);
 
-	return PULL_MODE_DEFAULT;
+	return PULL_MODE_FF_ONLY;
 }
 
 /**
@@ -1091,23 +1091,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (mode == PULL_MODE_FF_ONLY && !can_ff)
 		die(_("The pull was not fast-forward, please either merge or rebase.\n"));
 
-	if (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
-		advise(_("The pull was not fast-forward, in the future you will have to choose a merge, or a rebase.\n"
-			"\n"
-			"To quell this message you have two main options:\n"
-			"\n"
-			"1. Adopt the new behavior:\n"
-			"\n"
-			"  git config --global pull.mode ff-only\n"
-			"\n"
-			"2. Maintain the current behavior:\n"
-			"\n"
-			"  git config --global pull.mode merge\n"
-			"\n"
-			"For now we will fall back to the traditional behavior (merge).\n"
-			"Read \"git pull --help\" for more information."));
-	}
-
 	if (opt_rebase >= REBASE_TRUE) {
 		int ret = 0;
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index d4718dbc02..84685786da 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -879,39 +879,31 @@ setup_non_ff () {
 	setup_other master^
 }
 
-test_expect_success 'fast-forward (pull.mode=ff-only)' '
+test_expect_success 'fast-forward (default)' '
 	setup_ff &&
-	git -c pull.mode=ff-only pull
+	git pull
 '
 
-test_expect_success 'non-fast-forward (pull.mode=ff-only)' '
+test_expect_success 'non-fast-forward (default)' '
 	setup_non_ff &&
-	test_must_fail git -c pull.mode=ff-only pull
+	test_must_fail git pull
 '
 
-test_expect_success 'non-fast-forward with merge (pull.mode=ff-only)' '
+test_expect_success 'non-fast-forward with merge (default)' '
 	setup_non_ff &&
-	git -c pull.mode=ff-only pull --merge
+	git pull --merge
 '
 
-test_expect_success 'non-fast-forward with rebase (pull.mode=ff-only)' '
+test_expect_success 'non-fast-forward with rebase (default)' '
 	setup_non_ff &&
-	git -c pull.mode=ff-only pull --rebase
+	git pull --rebase
 '
 
-test_expect_success 'non-fast-forward error message (pull.mode=ff-only)' '
+test_expect_success 'non-fast-forward error message (default)' '
 	setup_non_ff &&
-	test_must_fail git -c pull.mode=ff-only pull 2> error &&
+	test_must_fail git pull 2> error &&
 	cat error &&
 	test_i18ngrep "The pull was not fast-forward" error
 '
 
-test_expect_success 'non-fast-forward warning (default)' '
-	setup_non_ff &&
-	git pull 2> error &&
-	cat error &&
-	test_i18ngrep "The pull was not fast-forward" error &&
-	test_i18ngrep "in the future you will have to choose" error
-'
-
 test_done
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 149fc2a009..c6c44ec570 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -27,78 +27,6 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'pull.rebase not set' '
-	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 "in the future you will have to choose" decoded
-'
-
-test_expect_success 'pull.rebase not set (fast-forward)' '
-	git reset --hard c0 &&
-	git pull . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.mode set' '
-	git reset --hard c2 &&
-	test_config pull.mode merge &&
-	git pull . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and pull.ff=true' '
-	git reset --hard c2 &&
-	test_config pull.ff true &&
-	git pull . c1 2>err &&
-	test_i18ngrep "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and pull.ff=false' '
-	git reset --hard c2 &&
-	test_config pull.ff false &&
-	git pull . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and pull.ff=only' '
-	git reset --hard c2 &&
-	test_config pull.ff only &&
-	test_must_fail git pull . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and --rebase given' '
-	git reset --hard c2 &&
-	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and --merge given' '
-	git reset --hard c2 &&
-	git pull --merge . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and --ff given' '
-	git reset --hard c2 &&
-	git pull --ff . c1 2>err &&
-	test_i18ngrep "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and --no-ff given' '
-	git reset --hard c2 &&
-	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
-test_expect_success 'pull.rebase not set and --ff-only given' '
-	git reset --hard c2 &&
-	test_must_fail git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "in the future you will have to choose" err
-'
-
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v4 00/19] pull: default ff-only mode
  2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
                   ` (18 preceding siblings ...)
  2020-12-08  0:26 ` [PATCH v4 19/19] future: pull: enable ff-only mode by default Felipe Contreras
@ 2020-12-08  0:57 ` Felipe Contreras
  19 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2020-12-08  0:57 UTC (permalink / raw)
  To: Git, Junio C Hamano; +Cc: Elijah Newren, Alex Henrie, Jeff King, Philip Oakley

Hi,

Junio, this is intended for you, I will describe step by step how the
warning evolves.

Step 0: This is what we have today:

  Pulling without specifying how to reconcile divergent branches is
  discouraged. You can squelch this message by running one of the following
  commands sometime before your next pull:

    git config pull.rebase false  # merge (the default strategy)
    git config pull.rebase true   # rebase
    git config pull.ff only       # fast-forward only

  You can replace "git config" with "git config --global" to set a default
  preference for all repositories. You can also pass --rebase, --no-rebase,
  or --ff-only on the command line to override the configured default per
  invocation.

> Felipe Contreras (19):
>   doc: pull: explain what is a fast-forward
>   pull: improve default warning

Step 1: This is low-hanging fruit that can be fixed today without any
change in behavior:

  Pulling without specifying how to reconcile divergent branches is discouraged;
  you need to specify if you want a merge, or a rebase.
  You can squelch this message by running one of the following commands:

    git config pull.rebase false  # merge (the default strategy)
    git config pull.rebase true   # rebase
    git config pull.ff only       # fast-forward only

  You can replace "git config" with "git config --global" to set a default
  preference for all repositories.
  If unsure, run "git pull --no-rebase".
  Read "git pull --help" for more information.

>   pull: refactor fast-forward check
>   pull: cleanup autostash check
>   pull: trivial cleanup
>   pull: move default warning
>   pull: display default warning only when non-ff

At this point we can update the warning to mention that we are inside
a non-fast-forward case. But it's not necessary.

>   pull: trivial whitespace style fix
>   pull: introduce --merge option

s/--no-rebase/--merge/

>   pull: show warning with --ff
>   rebase: add REBASE_DEFAULT
>   pull: move configurations fetches
>   test: merge-pull-config: trivial cleanup
>   test: pull-options: revert unnecessary changes
>   pull: trivial memory fix

This is the end of part I. At this point the default mode is still
"merge", and the only behavior change is that the warning is printed
only on the non-fast-forward case.

========================================================================

>   pull: add pull.mode

Step 2: pull.mode={merge,rebase} are specified

  Pulling without specifying how to reconcile divergent branches is discouraged;
  you need to specify if you want a merge, or a rebase.
  You can squelch this message by running one of the following commands:

    git config pull.mode merge    # (the default strategy)
    git config pull.mode rebase
    git config pull.ff only       # fast-forward only

  You can replace "git config" with "git config --global" to set a default
  preference for all repositories.
  If unsure, run "git pull --merge".
  Read "git pull --help" for more information.

>   pull: add pull.mode=ff-only

Step 3: Now pull.mode=ff-only

  Pulling without specifying how to reconcile divergent branches is discouraged;
  you need to specify if you want a merge, or a rebase.
  You can squelch this message by running one of the following commands:

    git config pull.mode merge    # (the default strategy)
    git config pull.mode rebase
    git config pull.mode ff-only  # fast-forward only

  You can replace "git config" with "git config --global" to set a default
  preference for all repositories.
  If unsure, run "git pull --merge".
  Read "git pull --help" for more information.

However, now in addition to the warning, there's an error message:

  The pull was not fast-forward, please either merge or rebase.

This error message is *only* triggered when the user has manually
configured "pull.mode=ff-only". And it is an error. The program dies.

And it's not meant to be temporary; it's permanent behavior.

>   pull: advice of future changes

Step 4: Now that pull.mode=ff-only is in place, we can aim for it
being the default, and we can tell our users that it will be the
default in the future.

  The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.

  To quell this message you have two main options:

  1. Adopt the new behavior:

    git config --global pull.mode ff-only

  2. Maintain the current behavior:

    git config --global pull.mode merge

  For now we will fall back to the traditional behavior (merge).
  Read "git pull --help" for more information.

This is the end of part II. At this point the default is still "merge".

Unlike part I, in part II we have committed to pull.mode=ff-only to be
the new default, and we are already telling users that they can use
this new mode to test the new behavior.

We should probably stay a couple of releases at this point, with this
warning, and the new behavior already configurable.

Elijah: notice how there's no mention of `git pull --merge`, because
in my opinion now we are telling users this is a temporary *warning*,
and the way to get rid of it properly should be very clear.

========================================================================

>   future: pull: enable ff-only mode by default

The last patch finally enables the ff-only mode by default, and the
warning is removed forever.

The only thing that remains now is the fatal error:

  The pull was not fast-forward, please either merge or rebase.

This fatal error is avoided by pull.mode=merge, pull.mode=rebase,
pull.rebase=true, pull.rebase=false, pull.rebase=$other, --merge, or
--rebase.

It *only* happens when the user does a vanilla "git pull", it's a
non-fast-forward update, the user has configured pull.mode=ff-only or
has not configured any of the above.

Is it more clear what is my proposal?

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-12-08  0:58 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-08  0:26 [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 01/19] doc: pull: explain what is a fast-forward Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 02/19] pull: improve default warning Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 03/19] pull: refactor fast-forward check Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 04/19] pull: cleanup autostash check Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 05/19] pull: trivial cleanup Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 06/19] pull: move default warning Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 07/19] pull: display default warning only when non-ff Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 08/19] pull: trivial whitespace style fix Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 09/19] pull: introduce --merge option Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 10/19] pull: show warning with --ff Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 11/19] rebase: add REBASE_DEFAULT Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 12/19] pull: move configurations fetches Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 13/19] test: merge-pull-config: trivial cleanup Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 14/19] test: pull-options: revert unnecessary changes Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 15/19] pull: trivial memory fix Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 16/19] pull: add pull.mode Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 17/19] pull: add pull.mode=ff-only Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 18/19] pull: advice of future changes Felipe Contreras
2020-12-08  0:26 ` [PATCH v4 19/19] future: pull: enable ff-only mode by default Felipe Contreras
2020-12-08  0:57 ` [PATCH v4 00/19] pull: default ff-only mode Felipe Contreras

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).