git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v3 00/16] pull: default warning improvements
@ 2020-12-05 19:52 Felipe Contreras
  2020-12-05 19:52 ` [PATCH v3 01/16] doc: pull: explain what is a fast-forward Felipe Contreras
                   ` (16 more replies)
  0 siblings, 17 replies; 33+ messages in thread
From: Felipe Contreras @ 2020-12-05 19:52 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Alex Henrie, Junio C Hamano, Jeff King,
	Philip Oakley, Felipe Contreras

The old thread "Pull is Mostly Evil" [1] came to haunt us 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


^ permalink raw reply related	[flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-12-08 21:04 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-05 19:52 [PATCH v3 00/16] pull: default warning improvements Felipe Contreras
2020-12-05 19:52 ` [PATCH v3 01/16] doc: pull: explain what is a fast-forward Felipe Contreras
2020-12-07 20:45   ` Junio C Hamano
2020-12-07 22:22     ` Felipe Contreras
2020-12-07 22:40       ` Elijah Newren
2020-12-07 23:08         ` Junio C Hamano
2020-12-08  0:20           ` Felipe Contreras
2020-12-08  0:17         ` Felipe Contreras
2020-12-08  1:23           ` Elijah Newren
2020-12-08 14:37             ` Felipe Contreras
2020-12-08 18:55               ` Felipe Contreras
2020-12-05 19:52 ` [PATCH v3 02/16] pull: improve default warning Felipe Contreras
2020-12-07 20:55   ` Junio C Hamano
2020-12-07 22:29     ` Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 03/16] pull: refactor fast-forward check Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 04/16] pull: cleanup autostash check Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 05/16] pull: trivial cleanup Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 06/16] pull: move default warning Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 07/16] pull: display default warning only when non-ff Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 08/16] pull: trivial whitespace style fix Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 09/16] pull: introduce --merge option Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 10/16] pull: show warning with --ff Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 11/16] rebase: add REBASE_DEFAULT Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 12/16] pull: move configurations fetches Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 13/16] pull: add proper error with --ff-only Felipe Contreras
2020-12-05 20:15   ` Felipe Contreras
2020-12-05 20:19     ` [PATCH 1/2] " Felipe Contreras
2020-12-05 20:19       ` [PATCH 2/2] tentative: pull: change the semantics of --ff-only Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 14/16] test: merge-pull-config: trivial cleanup Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 15/16] test: pull-options: revert unnecessary changes Felipe Contreras
2020-12-05 19:53 ` [PATCH v3 16/16] pull: trivial memory fix Felipe Contreras
2020-12-07 20:55 ` [PATCH v3 00/16] pull: default warning improvements Junio C Hamano
2020-12-07 22:33   ` Felipe Contreras

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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