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 [thread overview]
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
next reply other threads:[~2020-12-08 0:27 UTC|newest]
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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).