git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Felipe Contreras <felipe.contreras@gmail.com>
Date
Dec 5, 2020, 19:52 UTC
Message-ID
<20201205195313.1557473-1-felipe.contreras@gmail.com>
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
Next: Felipe Contreras
Message 1 of 33 in “pull: default warning improvements”
  1. 00/16 pull: default warning improvementsFelipe Contreras, Dec 5, 2020
  2. 01/16 doc: pull: explain what is a fast-forwardFelipe Contreras, Dec 5, 2020
  3. Junio C HamanoDec 7, 2020
  4. Felipe ContrerasDec 7, 2020
  5. Elijah NewrenDec 7, 2020
  6. Junio C HamanoDec 7, 2020
  7. Felipe ContrerasDec 8, 2020
  8. Felipe ContrerasDec 8, 2020
  9. Elijah NewrenDec 8, 2020
  10. Felipe ContrerasDec 8, 2020
  11. Felipe ContrerasDec 8, 2020
  12. 03/16 pull: refactor fast-forward checkFelipe Contreras, Dec 5, 2020
  13. 05/16 pull: trivial cleanupFelipe Contreras, Dec 5, 2020
  14. 04/16 pull: cleanup autostash checkFelipe Contreras, Dec 5, 2020
  15. 02/16 pull: improve default warningFelipe Contreras, Dec 5, 2020
  16. Junio C HamanoDec 7, 2020
  17. Felipe ContrerasDec 7, 2020
  18. 16/16 pull: trivial memory fixFelipe Contreras, Dec 5, 2020
  19. 14/16 test: merge-pull-config: trivial cleanupFelipe Contreras, Dec 5, 2020
  20. 06/16 pull: move default warningFelipe Contreras, Dec 5, 2020
  21. 08/16 pull: trivial whitespace style fixFelipe Contreras, Dec 5, 2020
  22. 07/16 pull: display default warning only when non-ffFelipe Contreras, Dec 5, 2020
  23. 09/16 pull: introduce --merge optionFelipe Contreras, Dec 5, 2020
  24. 13/16 pull: add proper error with --ff-onlyFelipe Contreras, Dec 5, 2020
  25. Felipe ContrerasDec 5, 2020
  26. 1/2 pull: add proper error with --ff-onlyFelipe Contreras, Dec 5, 2020
  27. 2/2 tentative: pull: change the semantics of --ff-onlyFelipe Contreras, Dec 5, 2020
  28. 12/16 pull: move configurations fetchesFelipe Contreras, Dec 5, 2020
  29. 15/16 test: pull-options: revert unnecessary changesFelipe Contreras, Dec 5, 2020
  30. 10/16 pull: show warning with --ffFelipe Contreras, Dec 5, 2020
  31. 11/16 rebase: add REBASE_DEFAULTFelipe Contreras, Dec 5, 2020
  32. Junio C HamanoDec 7, 2020
  33. Felipe ContrerasDec 7, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.