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

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

This is my latest attempt to try to fix it.

This patch series is divided in 3 parts:

== Part I ==

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

== Part II ==

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

== Part III ==

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

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

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

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

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

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

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

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


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

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

-- 
2.29.2


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

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

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