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@xxxxxxxxxxx/ [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
