Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>,
	Alex Henrie <alexhenrie24@gmail.com>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Philip Oakley <philipoakley@iee.email>,
	Felipe Contreras <felipe.contreras@gmail.com>
Subject: [PATCH v4 00/19] pull: default ff-only mode
Date: Mon,  7 Dec 2020 18:26:29 -0600
Message-ID: <20201208002648.1370414-1-felipe.contreras@gmail.com> (raw)

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


             reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-08  0:26 Felipe Contreras [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201208002648.1370414-1-felipe.contreras@gmail.com \
    --to=felipe.contreras@gmail.com \
    --cc=alexhenrie24@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.email \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git