[PATCH v3 00/16] pull: default warning improvements

[PATCH v3 00/16] pull: default warning improvements

[Felipe Contreras]

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

The main solution--reject non-fast-forward merges by default--seems to have lost traction (again).

There are multiple approaches floating around, but no clear path forward.

This patch series attempts to fix as much as possible of the current situation without committing to
any particular solution.

It does:

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. Improve the error message with --ff-only
6. Fix the behavior of the warning with --ff

And tentatively (and this should be the only controversial change):

7. Change the semantics of --ff-only

It does not:

* Introduce the suggested pull.mode
* Change the current default (still --ff)

It is not a complete solution, but should improve the current situation.

Since v2:

1. The warning is updated to say "a merge, a rebase, or a fast-forward"
2. A suggestion 'If unsure, run "git pull --merge".' is removed
3. A new REBASE_DEFAULT is introduced
4. Tests are revamped using helpers
5. The logic of --merge and --ff-only is updated so one can override the other
6. A bunch of commit messages are updated
7. A memory inconsistency is fixed

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

diff --git a/builtin/pull.c b/builtin/pull.c
index e389ffcdc3..0735c77f42 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;
@@ -114,6 +112,24 @@ 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),
@@ -131,8 +147,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"), 0),
+	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),
@@ -161,9 +178,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
 		N_("allow fast-forward"),
 		PARSE_OPT_NOARG),
-	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
+	OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
@@ -348,9 +365,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 +460,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 +473,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 +488,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."));
@@ -919,6 +934,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);
 
@@ -935,12 +953,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 < 0)
-		opt_rebase = config_get_rebase();
-
 	if (read_cache_unmerged())
 		die_resolve_conflict("pull");
 
@@ -950,7 +962,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;
@@ -1010,19 +1022,18 @@ 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 (!can_ff && default_mode) {
-		if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
-			die(_("The pull was not fast-forward, please either merge or rebase.\n"
-				"If unsure, run \"git pull --merge\"."));
-		}
+	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\n"
-				"discouraged; you need to specify if you want a merge, or a rebase.\n"
+			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"
@@ -1037,11 +1048,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	/* Disable --ff-only when --merge is specified */
-	if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only"))
-		opt_ff = NULL;
-
-	if (opt_rebase) {
+	if (opt_rebase >= REBASE_TRUE) {
 		int ret = 0;
 		if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
@@ -1050,7 +1057,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(&curr_head, merge_heads.oid, &rebase_fork_point);
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,
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index eec6224fb0..0cdac4010b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -819,66 +819,89 @@ test_expect_success 'git pull --rebase against local branch' '
 	test_cmp expect file2
 '
 
-test_expect_success 'git pull fast-forward (ff-only)' '
+setup_other () {
 	test_when_finished "git checkout master && git branch -D other test" &&
-	test_config pull.ff only &&
-	git checkout -b other master &&
+	git checkout -b other $1 &&
 	>new &&
 	git add new &&
 	git commit -m new &&
 	git checkout -b test -t other &&
-	git reset --hard master &&
+	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 'git pull non-fast-forward (ff-only)' '
-	test_when_finished "git checkout master && git branch -D other test" &&
+test_expect_success 'non-fast-forward (ff-only)' '
 	test_config pull.ff only &&
-	git checkout -b other master^ &&
-	>new &&
-	git add new &&
-	git commit -m new &&
-	git checkout -b test -t other &&
-	git reset --hard master &&
+	setup_non_ff &&
 	test_must_fail git pull
 '
 
-test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
-	test_when_finished "git checkout master && git branch -D other test" &&
+test_expect_success 'non-fast-forward with merge (ff-only)' '
 	test_config pull.ff only &&
-	git checkout -b other master^ &&
-	>new &&
-	git add new &&
-	git commit -m new &&
-	git checkout -b test -t other &&
-	git reset --hard master &&
-	git pull --no-rebase
+	setup_non_ff &&
+	git pull --merge
 '
 
-test_expect_success 'git pull non-fast-forward with rebase (ff-only)' '
-	test_when_finished "git checkout master && git branch -D other test" &&
+test_expect_success 'non-fast-forward with rebase (ff-only)' '
 	test_config pull.ff only &&
-	git checkout -b other master^ &&
-	>new &&
-	git add new &&
-	git commit -m new &&
-	git checkout -b test -t other &&
-	git reset --hard master &&
+	setup_non_ff &&
 	git pull --rebase
 '
 
-test_expect_success 'git pull non-fast-forward error message' '
-	test_when_finished "git checkout master && git branch -D other test" &&
+test_expect_success 'non-fast-forward error message (ff-only)' '
 	test_config pull.ff only &&
-	git checkout -b other master^ &&
-	>new &&
-	git add new &&
-	git commit -m new &&
-	git checkout -b test -t other &&
-	git reset --hard master &&
+	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 (16):
  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
  pull: add proper error with --ff-only
  test: merge-pull-config: trivial cleanup
  test: pull-options: revert unnecessary changes
  pull: trivial memory fix

 Documentation/git-pull.txt   |  24 +++++-
 builtin/pull.c               | 140 +++++++++++++++++++++--------------
 rebase.h                     |   3 +-
 t/t5520-pull.sh              |  85 +++++++++++++++++++++
 t/t5521-pull-options.sh      |  22 +++---
 t/t7601-merge-pull-config.sh |  35 +++++----
 6 files changed, 224 insertions(+), 85 deletions(-)

-- 
2.29.2

[PATCH v3 01/16] doc: pull: explain what is a fast-forward

[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..dc812139f4 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-foward way up to the new master.
+
+------------
+    D---E---A---B---C master, origin/master
+------------
+
+However, a non-fast-foward case looks very different.
+
 ------------
 	  A---B---C master on origin
 	 /
-- 
2.29.2

[PATCH v3 02/16] pull: improve default warning

[Felipe Contreras]

We want to:

1. Be clear about what "specifying" means; merge, rebase, or
   fast-forward.
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 | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 1034372f8b..d71344fe28 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -345,18 +345,19 @@ 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, 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 --no-rebase\".\n"
+			"Read \"git pull --help\" for more information."
+			));
 	}
 
 	return REBASE_FALSE;
-- 
2.29.2

[PATCH v3 03/16] pull: refactor fast-forward check

[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 d71344fe28..7a5b343fe2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -907,6 +907,20 @@ static int run_rebase(const struct object_id *curr_head,
 	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;
@@ -1017,22 +1031,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &rebase_fork_point, &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(&curr_head, merge_heads.oid, &rebase_fork_point);
-- 
2.29.2

[PATCH v3 04/16] pull: cleanup autostash check

[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 7a5b343fe2..d2f80e8615 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -927,7 +927,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);
@@ -960,8 +959,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;
 
@@ -1030,13 +1029,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &rebase_fork_point, &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(&curr_head, merge_heads.oid, &rebase_fork_point);
-- 
2.29.2

[PATCH v3 05/16] pull: trivial cleanup

[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 d2f80e8615..feadded1b6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1024,19 +1024,18 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	if (opt_rebase) {
 		int ret = 0;
-		int ran_ff = 0;
 		if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &rebase_fork_point, &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(&curr_head, merge_heads.oid, &rebase_fork_point);
+		}
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
-- 
2.29.2

[PATCH v3 06/16] pull: move default warning

[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 | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index feadded1b6..3fd9bd1bd2 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,21 +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, 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 --no-rebase\".\n"
-			"Read \"git pull --help\" for more information."
-			));
-	}
+	default_mode = 1;
 
 	return REBASE_FALSE;
 }
@@ -927,6 +915,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);
@@ -1022,6 +1011,24 @@ 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, 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 --no-rebase\".\n"
+			"Read \"git pull --help\" for more information."
+			));
+	}
+
 	if (opt_rebase) {
 		int ret = 0;
 		if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
@@ -1029,7 +1036,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &rebase_fork_point, &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

[PATCH v3 07/16] pull: display default warning only when non-ff

[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 dc812139f4..ad33d2472c 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -60,6 +60,9 @@ However, a non-fast-foward 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 3fd9bd1bd2..300b17b962 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1013,7 +1013,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, a rebase, or a fast-forward.\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

[PATCH v3 08/16] pull: trivial whitespace style fix

[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 300b17b962..cecbacc549 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

[PATCH v3 09/16] pull: introduce --merge option

[Felipe Contreras]

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

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 ad33d2472c..c220da143a 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -61,7 +61,7 @@ However, a non-fast-foward 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 cecbacc549..da91d78a22 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),
@@ -1024,7 +1026,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

[PATCH v3 10/16] pull: show warning with --ff

[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 da91d78a22..2bd6ee9d19 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1015,7 +1015,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, a rebase, or a fast-forward.\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

[PATCH v3 11/16] rebase: add REBASE_DEFAULT

[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 2bd6ee9d19..347ac89eee 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."));
@@ -938,7 +934,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())
@@ -950,7 +946,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;
@@ -1010,12 +1006,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, a rebase, or a fast-forward.\n"
 			"You can squelch this message by running one of the following commands:\n"
@@ -1031,7 +1027,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			));
 	}
 
-	if (opt_rebase) {
+	if (opt_rebase >= REBASE_TRUE) {
 		int ret = 0;
 		if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
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

[PATCH v3 12/16] pull: move configurations fetches

[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 347ac89eee..e0157d013f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -915,6 +915,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);
 
@@ -931,12 +934,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

[PATCH v3 13/16] pull: add proper error with --ff-only

[Felipe Contreras]

The current error is not user-friendly:

  fatal: not possible to fast-forward, aborting.

We want something that actually explains what is going on:

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

The user can get rid of the warning by doing either --merge or
--rebase.

Except: doing "git pull --merge" is not actually enough; we would return
to the previous behavior: "fatal: not possible to fast-forward,
aborting". In order to do the right thing we will have to change the
semantics of --ff-only.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c  | 60 +++++++++++++++++++++++-----------
 t/t5520-pull.sh | 85 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+), 18 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index e0157d013f..54c58618e9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -112,6 +112,24 @@ 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),
@@ -129,8 +147,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),
@@ -159,9 +178,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
 		N_("allow fast-forward"),
 		PARSE_OPT_NOARG),
-	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
+	OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
@@ -1008,20 +1027,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	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, 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 (!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 >= REBASE_TRUE) {
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9fae07cdfa..0cdac4010b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -819,4 +819,89 @@ 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
-- 
2.29.2

[PATCH v3 14/16] test: merge-pull-config: trivial cleanup

[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

[PATCH v3 15/16] test: pull-options: revert unnecessary changes

[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

[PATCH v3 16/16] pull: trivial memory fix

[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 54c58618e9..0735c77f42 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1057,7 +1057,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(&curr_head, merge_heads.oid, &rebase_fork_point);
-- 
2.29.2

Re: [PATCH v3 13/16] pull: add proper error with --ff-only

[Felipe Contreras]

On Sat, Dec 5, 2020 at 1:53 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> The current error is not user-friendly:
>
>   fatal: not possible to fast-forward, aborting.
>
> We want something that actually explains what is going on:
>
>   The pull was not fast-forward, please either merge or rebase.
>
> The user can get rid of the warning by doing either --merge or
> --rebase.
>
> Except: doing "git pull --merge" is not actually enough; we would return
> to the previous behavior: "fatal: not possible to fast-forward,
> aborting". In order to do the right thing we will have to change the
> semantics of --ff-only.

This commit is wrong, the next one somehow got squashed into it.

I'm sending the two patches separately.

-- 
Felipe Contreras

[PATCH 1/2] pull: add proper error with --ff-only

[Felipe Contreras]

The current error is not user-friendly:

  fatal: not possible to fast-forward, aborting.

We want something that actually explains what is going on:

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

The user can get rid of the warning by doing either --merge or
--rebase.

Except: doing "git pull --merge" is not actually enough; we would return
to the previous behavior: "fatal: not possible to fast-forward,
aborting". In order to do the right thing we will have to change the
semantics of --ff-only.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c  | 33 ++++++++++++++++++--------------
 t/t5520-pull.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index e0157d013f..44ec6e7216 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1008,20 +1008,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	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, 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 (!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 >= REBASE_TRUE) {
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9fae07cdfa..067780e658 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -819,4 +819,54 @@ 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_failure '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_done
-- 
2.29.2

[PATCH 2/2] tentative: pull: change the semantics of --ff-only

[Felipe Contreras]

We want the last thing specified to override everything previously.

For example --merge should override a previous "pull.ff = only"
configuration.

And --ff-only should override a previous "pull.rebase = true"
configuration.

Currently "git pull --ff-only --merge" fails with:

  fatal: not possible to fast-forward, aborting.

But that's not what we want; we want --merge to override --ff-only and
do a --no-ff merge.

This is a backwards-incompatible change that achieves that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c  | 27 +++++++++++++++++++++++----
 t/t5520-pull.sh | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 44ec6e7216..54c58618e9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -112,6 +112,24 @@ 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),
@@ -129,8 +147,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),
@@ -159,9 +178,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
 		N_("allow fast-forward"),
 		PARSE_OPT_NOARG),
-	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
+	OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 067780e658..0cdac4010b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -849,7 +849,7 @@ test_expect_success 'non-fast-forward (ff-only)' '
 	test_must_fail git pull
 '
 
-test_expect_failure 'non-fast-forward with merge (ff-only)' '
+test_expect_success 'non-fast-forward with merge (ff-only)' '
 	test_config pull.ff only &&
 	setup_non_ff &&
 	git pull --merge
@@ -869,4 +869,39 @@ test_expect_success 'non-fast-forward error message (ff-only)' '
 	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
-- 
2.29.2

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

The intention is very good, but ...

> +------------
> +	  A---B---C master on origin
> +	 /
> +    D---E master
> +------------
> +
> +Then `git pull` will merge in a fast-foward way up to the new master.

... I find the phrase "in a fast-forward way" a bit awkward.
Perhaps use the 'fast-forward' as a verb, i.e.

	Then `git pull` notices that what is being merged is a
	descendant of our current branch, and fast-forwards our
	'master' branch to the commit.

or something like that?  It should be in line with the spirit in
which glossary defines fast-forward, I would think.

> +
> +------------
> +    D---E---A---B---C master, origin/master
> +------------
> +
> +However, a non-fast-foward case looks very different.

s/foward/forward/ (the same typo exists above);

>  ------------
>  	  A---B---C master on origin
>  	 /

Re: [PATCH v3 02/16] pull: improve default warning

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We want to:
>
> 1. Be clear about what "specifying" means; merge, rebase, or
>    fast-forward.
> 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 | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1034372f8b..d71344fe28 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -345,18 +345,19 @@ 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, 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 --no-rebase\".\n"
> +			"Read \"git pull --help\" for more information."
> +			));
>  	}

It is an improvement to say what they can do from the command line
in order to get out of the current situation, before giving them a
configuration advice.

But the exact instruction for the command line in the original seems
to have been lost during the rewrite, which I think makes the "what
to do now" advice less valuable than it could be.

I personally think it is backwards to update this message before
fixing the condition under which it triggers (I think by now
everybody involved in the thread understands that we do not want to
give this advise that does not stop to behave as-is---it should be
made not to trigger if we know the history would fast-forward, and
when it triggers it should stop the operation).  It may appear OK as
long as we get the right end-state, but because this message must be
rewritten when the triggering condition changes (i.e. when we stop
giving this message when the history fast-forwards, there is no
point in offering the fast-forward-only as a viable option), it
seems to me a needless detour.

Re: [PATCH v3 00/16] pull: default warning improvements

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> It is not a complete solution, but should improve the current situation.
>
> Since v2:
>
> 1. The warning is updated to say "a merge, a rebase, or a fast-forward"
> 2. A suggestion 'If unsure, run "git pull --merge".' is removed
> 3. A new REBASE_DEFAULT is introduced
> 4. Tests are revamped using helpers
> 5. The logic of --merge and --ff-only is updated so one can override the other
> 6. A bunch of commit messages are updated
> 7. A memory inconsistency is fixed

On what base has this series been built?  Applying to 'master' seems
to stop at the "refactor fast-forward check" step.

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Felipe Contreras]

On Mon, Dec 7, 2020 at 2:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We want users to know what is a fast-forward in order to understand the
> > default warning.
>
> The intention is very good, but ...
>
> > +------------
> > +       A---B---C master on origin
> > +      /
> > +    D---E master
> > +------------
> > +
> > +Then `git pull` will merge in a fast-foward way up to the new master.
>
> ... I find the phrase "in a fast-forward way" a bit awkward.
> Perhaps use the 'fast-forward' as a verb, i.e.
>
>         Then `git pull` notices that what is being merged is a
>         descendant of our current branch, and fast-forwards our
>         'master' branch to the commit.
>
> or something like that?  It should be in line with the spirit in
> which glossary defines fast-forward, I would think.

The glossary defines a fast-forward as:

  A fast-forward is a special type of `merge`

So, if you consider "merge" a noun, then a fast-forward is an
adjective. If you consider it a verb, then it's an adverb. But it's
not a verb.

If it was a verb, then we should have `git fast-forward`, which may
not be a terrible idea, but right now a fast-forward is a modifier.

At least that's what I have in my mind, and the glossary seems to agree.

> > +
> > +------------
> > +    D---E---A---B---C master, origin/master
> > +------------
> > +
> > +However, a non-fast-foward case looks very different.
>
> s/foward/forward/ (the same typo exists above);

All right.

-- 
Felipe Contreras

Re: [PATCH v3 02/16] pull: improve default warning

[Felipe Contreras]

On Mon, Dec 7, 2020 at 2:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We want to:
> >
> > 1. Be clear about what "specifying" means; merge, rebase, or
> >    fast-forward.
> > 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 | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 1034372f8b..d71344fe28 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -345,18 +345,19 @@ 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, 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 --no-rebase\".\n"
> > +                     "Read \"git pull --help\" for more information."
> > +                     ));
> >       }
>
> It is an improvement to say what they can do from the command line
> in order to get out of the current situation, before giving them a
> configuration advice.
>
> But the exact instruction for the command line in the original seems
> to have been lost during the rewrite, which I think makes the "what
> to do now" advice less valuable than it could be.

In my opinion we should not dump the entire contents of the manpage in
a warning. The warning should be as brief as possible while giving
these:

1. A description of what's going on
2. A suggested way to move forward
3. A quick way to opt-out
4. A way to find more information

That's it.

Of course when it comes to succinctness other developers tend to
disagree with me, but that's my opinion.

> I personally think it is backwards to update this message before
> fixing the condition under which it triggers (I think by now
> everybody involved in the thread understands that we do not want to
> give this advise that does not stop to behave as-is---it should be
> made not to trigger if we know the history would fast-forward, and
> when it triggers it should stop the operation).  It may appear OK as
> long as we get the right end-state, but because this message must be
> rewritten when the triggering condition changes (i.e. when we stop
> giving this message when the history fast-forwards, there is no
> point in offering the fast-forward-only as a viable option), it
> seems to me a needless detour.

Except it's not necessary to rewrite it further, not in step 1.

Maybe it will be clearer when I send all the patches.

-- 
Felipe Contreras

Re: [PATCH v3 00/16] pull: default warning improvements

[Felipe Contreras]

On Mon, Dec 7, 2020 at 2:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > It is not a complete solution, but should improve the current situation.
> >
> > Since v2:
> >
> > 1. The warning is updated to say "a merge, a rebase, or a fast-forward"
> > 2. A suggestion 'If unsure, run "git pull --merge".' is removed
> > 3. A new REBASE_DEFAULT is introduced
> > 4. Tests are revamped using helpers
> > 5. The logic of --merge and --ff-only is updated so one can override the other
> > 6. A bunch of commit messages are updated
> > 7. A memory inconsistency is fixed
>
> On what base has this series been built?  Applying to 'master' seems
> to stop at the "refactor fast-forward check" step.

Ninth batch.

The 'pb/pull-rebase-recurse-submodules' merge is the one causing the
conflicts (I think).

-- 
Felipe Contreras

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Elijah Newren]

Hi,

On Mon, Dec 7, 2020 at 2:22 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Mon, Dec 7, 2020 at 2:45 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> > > We want users to know what is a fast-forward in order to understand the
> > > default warning.
> >
> > The intention is very good, but ...
> >
> > > +------------
> > > +       A---B---C master on origin
> > > +      /
> > > +    D---E master
> > > +------------
> > > +
> > > +Then `git pull` will merge in a fast-foward way up to the new master.
> >
> > ... I find the phrase "in a fast-forward way" a bit awkward.
> > Perhaps use the 'fast-forward' as a verb, i.e.
> >
> >         Then `git pull` notices that what is being merged is a
> >         descendant of our current branch, and fast-forwards our
> >         'master' branch to the commit.
> >
> > or something like that?  It should be in line with the spirit in
> > which glossary defines fast-forward, I would think.
>
> The glossary defines a fast-forward as:
>
>   A fast-forward is a special type of `merge`
>
> So, if you consider "merge" a noun, then a fast-forward is an
> adjective. If you consider it a verb, then it's an adverb. But it's
> not a verb.

A square is a special type of a rectangle, but that doesn't make
"square" an adjective; both square and rectangle are nouns.

> If it was a verb, then we should have `git fast-forward`, which may
> not be a terrible idea, but right now a fast-forward is a modifier.
>
> At least that's what I have in my mind, and the glossary seems to agree.

If you read the release notes and even various messages printed by
git, "fast-forwards", "fast-forwarded", "fast-forwarding", and "to
fast-forward" all appear multiple times.  And yes, "fast-forward" also
appears multiple times as a noun in addition to the various uses as a
verb.  So, I'd say the glossary just isn't comprehensive because in
this case we have a word that serves as both a noun and a verb.


Going back to the text Junio highlighted, I agree with him that the
phrase looks really awkward, and much prefer his suggestion
(regardless of whether it aligns with the current glossary).

> > > +
> > > +------------
> > > +    D---E---A---B---C master, origin/master
> > > +------------
> > > +
> > > +However, a non-fast-foward case looks very different.
> >
> > s/foward/forward/ (the same typo exists above);
>
> All right.
>
> --
> Felipe Contreras

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> > ... I find the phrase "in a fast-forward way" a bit awkward.
>> > Perhaps use the 'fast-forward' as a verb, i.e.
>> >
>> >         Then `git pull` notices that what is being merged is a
>> >         descendant of our current branch, and fast-forwards our
>> >         'master' branch to the commit.
>> >
>> > or something like that?  It should be in line with the spirit in
>> > which glossary defines fast-forward, I would think.
>> ...
> If you read the release notes and even various messages printed by
> git, "fast-forwards", "fast-forwarded", "fast-forwarding", and "to
> fast-forward" all appear multiple times.  And yes, "fast-forward" also
> appears multiple times as a noun in addition to the various uses as a
> verb.  So, I'd say the glossary just isn't comprehensive because in
> this case we have a word that serves as both a noun and a verb.

Ah, sorry, I didn't mean noun-vs-verb when I mentioned the glossary.

I thought that the idea that the word can be used as a verb, after
discussing advise() messages that tells the users that they can
"merge, rebase or fast-forward", was given and not something anybody
needs to be explained about.

The other half of what I suggested was to explain what situation is
fast-forwardable, i.e. "notices ... is a descendant of", and I made
sure that the explanation was in line with the grossary.  Without it
explained in-place in the text, readers who need to be told what a
fast-forward is needs to go to and come back from the glossary while
reading this page, which was what I tried to improve while we are
trying to find a better phrasing.

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Felipe Contreras]

On Mon, Dec 7, 2020 at 4:40 PM Elijah Newren <newren@gmail.com> wrote:
> On Mon, Dec 7, 2020 at 2:22 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> > The glossary defines a fast-forward as:
> >
> >   A fast-forward is a special type of `merge`
> >
> > So, if you consider "merge" a noun, then a fast-forward is an
> > adjective. If you consider it a verb, then it's an adverb. But it's
> > not a verb.
>
> A square is a special type of a rectangle, but that doesn't make
> "square" an adjective; both square and rectangle are nouns.

Words have multiple definitions. The word "square" is both a noun, and
an adjective [1]. It's perfectly fine to say "square corner".

Just like it's perfectly fine to say "fast-forward merge", or "quick sort".

And that's how many people use it:

https://git-scm.com/docs/git-merge#_fast_forward_merge
https://docs.gitlab.com/ee/user/project/merge_requests/fast_forward_merge.html
https://www.atlassian.com/git/tutorials/using-branches/git-merge#:~:text=Fast%20Forward%20Merge

Plus there's many instances in the documentation:

* non-fast-forward update
* fast-forward merges
* fast-forward check
* "fast-forward-ness"
* fast-forward situation
* fast-forward push
* non-fast-forward pushes
* non-fast-forward reference
* fast-forward cases
* "fast-forward" merge

> > If it was a verb, then we should have `git fast-forward`, which may
> > not be a terrible idea, but right now a fast-forward is a modifier.
> >
> > At least that's what I have in my mind, and the glossary seems to agree.
>
> If you read the release notes and even various messages printed by
> git, "fast-forwards", "fast-forwarded", "fast-forwarding", and "to
> fast-forward" all appear multiple times.  And yes, "fast-forward" also
> appears multiple times as a noun in addition to the various uses as a
> verb.  So, I'd say the glossary just isn't comprehensive because in
> this case we have a word that serves as both a noun and a verb.

It can be a noun, a verb, an adjective, and an adverb. But the
question is not what it can be, but what it actually is. I'm just
telling you my rationale:

1. noun: it doesn't make sense because you don't create, pick, show,
or push a "fast-forward"
2. verb: there's no idiom to tell git "do fast-forward"
3. adjective: there are merge nouns (commits), but no instances of
fast-forward merge commits, like say octopus merge commits
4. adverb: you can tell git "do merge", and "do fast-forward merge"

So, in my opinion a fast-forward today can only logically be an adverb.

Like a bubble sort is a special type of sort, and theoretically you
can say "do a bubble", but it's just weird. My mind is left hanging: a
bubble $what? Likewise, when people say "do me a solid", I'm
annoyed... A solid $what?! They mean "a solid favor". People do it, so
it's part of language, but it doesn't stop it from being weird in my
opinion.

An adverb typically answers the question "in what way?". Do me a
favor... In what way? In a solid way. Do a sort... In what way? In a
quick way. Do a move... In what way? In a bold way. Do a merge... In
what way? In a fast-forward way.

> Going back to the text Junio highlighted, I agree with him that the
> phrase looks really awkward, and much prefer his suggestion
> (regardless of whether it aligns with the current glossary).

Normally I don't show credentials, but in this case I think it might
be relevant. I've read multiple linguists, like Noam Chomsky, and
Steven Pinker. I follow many others and read their articles. I also
read The Pinker's Sense of Style: The Thinking Person's Guide to
Writing in the 21st Century [2], which I can't recommend enough for
people writing technical documents or any other classic style. I have
an arguably successful blog with more than 200 articles and more than
1 million views, which is regularly linked from other blogs, and
technical resources. I constantly get thanked both in person, and
online for what I write. And at some point I was asked by a publisher
to write a book about Git (which I didn't feel prepared for at that
time).

So, clearly at least some people value the way I write.

I'm not trying to be stubborn here, I just honestly put effort into
the art of writing, and I do care deeply about language.

Of course I might be wrong in this particular instance, but if I am,
it's not because of lack of effort.

I think fast-forward is mainly an adverb, but even if it isn't the
main usage; it's still clearly an usage.

Cheers.

[1] https://www.merriam-webster.com/dictionary/square
[2] https://www.amazon.com/Sense-Style-Thinking-Persons-Writing/dp/0143127799

-- 
Felipe Contreras

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Felipe Contreras]

On Mon, Dec 7, 2020 at 5:08 PM Junio C Hamano <gitster@pobox.com> wrote:

> I thought that the idea that the word can be used as a verb, after
> discussing advise() messages that tells the users that they can
> "merge, rebase or fast-forward", was given and not something anybody
> needs to be explained about.

I removed that in the latest version, precisely because you can't tell
git pull to resolve a diverging branch situation by doing a
"fast-forward". It's only merge or rebase.

-- 
Felipe Contreras

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Elijah Newren]

On Mon, Dec 7, 2020 at 4:17 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Mon, Dec 7, 2020 at 4:40 PM Elijah Newren <newren@gmail.com> wrote:
> > On Mon, Dec 7, 2020 at 2:22 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
>
> > > The glossary defines a fast-forward as:
> > >
> > >   A fast-forward is a special type of `merge`
> > >
> > > So, if you consider "merge" a noun, then a fast-forward is an
> > > adjective. If you consider it a verb, then it's an adverb. But it's
> > > not a verb.
> >
> > A square is a special type of a rectangle, but that doesn't make
> > "square" an adjective; both square and rectangle are nouns.
>
> Words have multiple definitions. The word "square" is both a noun, and
> an adjective [1]. It's perfectly fine to say "square corner".
>
> Just like it's perfectly fine to say "fast-forward merge", or "quick sort".
>
> And that's how many people use it:
>
> https://git-scm.com/docs/git-merge#_fast_forward_merge
> https://docs.gitlab.com/ee/user/project/merge_requests/fast_forward_merge.html
> https://www.atlassian.com/git/tutorials/using-branches/git-merge#:~:text=Fast%20Forward%20Merge
>
> Plus there's many instances in the documentation:
>
> * non-fast-forward update
> * fast-forward merges
> * fast-forward check
> * "fast-forward-ness"
> * fast-forward situation
> * fast-forward push
> * non-fast-forward pushes
> * non-fast-forward reference
> * fast-forward cases
> * "fast-forward" merge

Yeah, and the number of "fast-forward merge" instances suggest I'm
losing the battle on "fast-forward" not being a merge but a different
thing.  So maybe I'm losing multiple battles here.  :-)

> > > If it was a verb, then we should have `git fast-forward`, which may
> > > not be a terrible idea, but right now a fast-forward is a modifier.
> > >
> > > At least that's what I have in my mind, and the glossary seems to agree.
> >
> > If you read the release notes and even various messages printed by
> > git, "fast-forwards", "fast-forwarded", "fast-forwarding", and "to
> > fast-forward" all appear multiple times.  And yes, "fast-forward" also
> > appears multiple times as a noun in addition to the various uses as a
> > verb.  So, I'd say the glossary just isn't comprehensive because in
> > this case we have a word that serves as both a noun and a verb.
>
> It can be a noun, a verb, an adjective, and an adverb. But the
> question is not what it can be, but what it actually is. I'm just
> telling you my rationale:
>
> 1. noun: it doesn't make sense because you don't create, pick, show,
> or push a "fast-forward"
> 2. verb: there's no idiom to tell git "do fast-forward"
> 3. adjective: there are merge nouns (commits), but no instances of
> fast-forward merge commits, like say octopus merge commits
> 4. adverb: you can tell git "do merge", and "do fast-forward merge"
>
> So, in my opinion a fast-forward today can only logically be an adverb.
>
> Like a bubble sort is a special type of sort, and theoretically you
> can say "do a bubble", but it's just weird. My mind is left hanging: a
> bubble $what? Likewise, when people say "do me a solid", I'm
> annoyed... A solid $what?! They mean "a solid favor". People do it, so
> it's part of language, but it doesn't stop it from being weird in my
> opinion.
>
> An adverb typically answers the question "in what way?". Do me a
> favor... In what way? In a solid way. Do a sort... In what way? In a
> quick way. Do a move... In what way? In a bold way. Do a merge... In
> what way? In a fast-forward way.
>
> > Going back to the text Junio highlighted, I agree with him that the
> > phrase looks really awkward, and much prefer his suggestion
> > (regardless of whether it aligns with the current glossary).
>
> Normally I don't show credentials, but in this case I think it might
> be relevant. I've read multiple linguists, like Noam Chomsky, and
> Steven Pinker. I follow many others and read their articles. I also
> read The Pinker's Sense of Style: The Thinking Person's Guide to
> Writing in the 21st Century [2], which I can't recommend enough for
> people writing technical documents or any other classic style. I have
> an arguably successful blog with more than 200 articles and more than
> 1 million views, which is regularly linked from other blogs, and
> technical resources. I constantly get thanked both in person, and
> online for what I write. And at some point I was asked by a publisher
> to write a book about Git (which I didn't feel prepared for at that
> time).
>
> So, clearly at least some people value the way I write.
>
> I'm not trying to be stubborn here, I just honestly put effort into
> the art of writing, and I do care deeply about language.
>
> Of course I might be wrong in this particular instance, but if I am,
> it's not because of lack of effort.
>
> I think fast-forward is mainly an adverb, but even if it isn't the
> main usage; it's still clearly an usage.
>
> Cheers.
>
> [1] https://www.merriam-webster.com/dictionary/square
> [2] https://www.amazon.com/Sense-Style-Thinking-Persons-Writing/dp/0143127799

You have very compelling arguments that fast-forward often serves as
an adverb (and if I'd thought a little closer, I would have remembered
that I use "fast-forward update" myself).  You have me convinced.

However, I am somewhat less convinced that "fast-forward" doesn't also
serve as a noun or a verb.  Perhaps you are trying to argue how it
*should* be used rather than how it *is* used, in which case I don't
have any counter-arguments for you (I'm less well linguistically
trained).  But if you look at how it is used, the number of times "a
fast-forward update" is shortened to "a fast-forward" in the
documentation suggests to me it's often a noun, and the number of
times that variants such as "fast-forwards", "fast-forwarded",
"fast-forwarding", and "to fast-forward" appear in both the docs and
output messages means that it's also frequently used as a verb as
well.  The fact that Junio expressed surprise upthread ("I thought
that the idea that the word can be used as a verb...was given and not
something anybody needs to be explained about") also suggests that
usage of fast-forward as a verb is common.  Anyway, I think trying to
treat "fast-forward" as solely an adverb results in awkward phrases
like "in a fast-forward way" instead of just using the much simpler
verb form.

Also, re-reading my earlier email, it looks like it could easily come
across as curt.  My apologies if it did read that way.

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Felipe Contreras]

On Mon, Dec 7, 2020 at 7:23 PM Elijah Newren <newren@gmail.com> wrote:

> Yeah, and the number of "fast-forward merge" instances suggest I'm
> losing the battle on "fast-forward" not being a merge but a different
> thing.  So maybe I'm losing multiple battles here.  :-)

In theory it could be both. I just don't see how.

> You have very compelling arguments that fast-forward often serves as
> an adverb (and if I'd thought a little closer, I would have remembered
> that I use "fast-forward update" myself).  You have me convinced.

Great!

> However, I am somewhat less convinced that "fast-forward" doesn't also
> serve as a noun or a verb.

I'm not saying it doesn't serve as a noun or a verb. It is certainly
used in that way *in addition* to being an adverb. I'm just saying in
my opinion it's primarily an adverb.

> Perhaps you are trying to argue how it
> *should* be used rather than how it *is* used, in which case I don't
> have any counter-arguments for you (I'm less well linguistically
> trained).

Not quite. I agree it is used both as a noun and a verb. And I
wouldn't attempt to mandate how words should be used (I'm against
prescriptiveness).

I'm just arguing from the point of view of the mental model. There is
no such thing as a fast-forward object.

Take for example the word "calibration". It is a noun, but you can't
point to any calibration thing. It comes from the verb calibrating,
and such conversions are called nominalizations.

I'm currently re-reading The Sense of Style, and it's interesting that
in Chapter 2 Steven Pinker mentions precisely these nouns, which he
calls "zombie nouns". They certainly do exist, and people use them,
but they suck the lifeblood out of prose. Take for example
"comprehension checks were used as exclusion criteria" (zombie nouns),
compared to "we excluded people who failed to understand the
instructions" (live verbs).

This article from writing expert Helen Sword does a great job of
explaining them:

https://opinionator.blogs.nytimes.com/2012/07/23/zombie-nouns/

Yes, "a merge done in a fast-forward way", is a "fast-forward merge",
which can be nouned as "a fast-forward". It's just not very alive.

> The fact that Junio expressed surprise upthread ("I thought
> that the idea that the word can be used as a verb...was given and not
> something anybody needs to be explained about") also suggests that
> usage of fast-forward as a verb is common.  Anyway, I think trying to
> treat "fast-forward" as solely an adverb results in awkward phrases
> like "in a fast-forward way" instead of just using the much simpler
> verb form.

Yes, it is common also. I'm just saying unless there's a "git
fast-forward" command you can't really tell git "do a fast-forward",
what you tell git is: "do a merge in a fast-forward way". It's a valid
way of thinking, just not the way I think.

BTW, I find it interesting that there are many instances of
"fast-forward update" in the documentation, and back then I did create
a "git update" tool (essentially a copy of "git pull"), so this
suggests there's a void that such a tool certainly would fill.

With such a tool we would have "fast-forward merge", "fast-forward
pull", and "fast-forward update", which indicates that adverb is the
most natural notion.

> Also, re-reading my earlier email, it looks like it could easily come
> across as curt.  My apologies if it did read that way.

No worries. I didn't take it as such. Same from my side; I'm just
stating my opinion.

Cheers.

[1] https://github.com/felipec/git/commit/d38f1641fc33535aa3c92cf6d3a30334324d3488

-- 
Felipe Contreras

Re: [PATCH v3 01/16] doc: pull: explain what is a fast-forward

[Felipe Contreras]

On Tue, Dec 8, 2020 at 8:37 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> Take for example the word "calibration". It is a noun, but you can't
> point to any calibration thing. It comes from the verb calibrating,
> and such conversions are called nominalizations.
>
> I'm currently re-reading The Sense of Style, and it's interesting that
> in Chapter 2 Steven Pinker mentions precisely these nouns, which he
> calls "zombie nouns". They certainly do exist, and people use them,
> but they suck the lifeblood out of prose. Take for example
> "comprehension checks were used as exclusion criteria" (zombie nouns),
> compared to "we excluded people who failed to understand the
> instructions" (live verbs).

Actually I found this video, which is more digestible and enjoyable:

Zombie Nouns and the Passive Voice in Writing
https://www.youtube.com/watch?v=sS-Txm3R3v8

-- 
Felipe Contreras