| Message ID | 20201208002648.1370414-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Junio, this is intended for you, I will describe step by step how the
warning evolves.
Step 0: This is what we have today:
Pulling without specifying how to reconcile divergent branches is
discouraged. You can squelch this message by running one of the following
commands sometime before your next pull:
git config pull.rebase false # merge (the default strategy)
git config pull.rebase true # rebase
git config pull.ff only # fast-forward only
You can replace "git config" with "git config --global" to set a default
preference for all repositories. You can also pass --rebase, --no-rebase,
or --ff-only on the command line to override the configured default per
invocation.
> Felipe Contreras (19):
> doc: pull: explain what is a fast-forward
> pull: improve default warning
Step 1: This is low-hanging fruit that can be fixed today without any
change in behavior:
Pulling without specifying how to reconcile divergent branches is discouraged;
you need to specify if you want a merge, or a rebase.
You can squelch this message by running one of the following commands:
git config pull.rebase false # merge (the default strategy)
git config pull.rebase true # rebase
git config pull.ff only # fast-forward only
You can replace "git config" with "git config --global" to set a default
preference for all repositories.
If unsure, run "git pull --no-rebase".
Read "git pull --help" for more information.
> pull: refactor fast-forward check
> pull: cleanup autostash check
> pull: trivial cleanup
> pull: move default warning
> pull: display default warning only when non-ff
At this point we can update the warning to mention that we are inside
a non-fast-forward case. But it's not necessary.
> pull: trivial whitespace style fix
> pull: introduce --merge option
s/--no-rebase/--merge/
> 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
This is the end of part I. At this point the default mode is still
"merge", and the only behavior change is that the warning is printed
only on the non-fast-forward case.
========================================================================
> pull: add pull.mode
Step 2: pull.mode={merge,rebase} are specified
Pulling without specifying how to reconcile divergent branches is discouraged;
you need to specify if you want a merge, or a rebase.
You can squelch this message by running one of the following commands:
git config pull.mode merge # (the default strategy)
git config pull.mode rebase
git config pull.ff only # fast-forward only
You can replace "git config" with "git config --global" to set a default
preference for all repositories.
If unsure, run "git pull --merge".
Read "git pull --help" for more information.
> pull: add pull.mode=ff-only
Step 3: Now pull.mode=ff-only
Pulling without specifying how to reconcile divergent branches is discouraged;
you need to specify if you want a merge, or a rebase.
You can squelch this message by running one of the following commands:
git config pull.mode merge # (the default strategy)
git config pull.mode rebase
git config pull.mode ff-only # fast-forward only
You can replace "git config" with "git config --global" to set a default
preference for all repositories.
If unsure, run "git pull --merge".
Read "git pull --help" for more information.
However, now in addition to the warning, there's an error message:
The pull was not fast-forward, please either merge or rebase.
This error message is *only* triggered when the user has manually
configured "pull.mode=ff-only". And it is an error. The program dies.
And it's not meant to be temporary; it's permanent behavior.
> pull: advice of future changes
Step 4: Now that pull.mode=ff-only is in place, we can aim for it
being the default, and we can tell our users that it will be the
default in the future.
The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.
To quell this message you have two main options:
1. Adopt the new behavior:
git config --global pull.mode ff-only
2. Maintain the current behavior:
git config --global pull.mode merge
For now we will fall back to the traditional behavior (merge).
Read "git pull --help" for more information.
This is the end of part II. At this point the default is still "merge".
Unlike part I, in part II we have committed to pull.mode=ff-only to be
the new default, and we are already telling users that they can use
this new mode to test the new behavior.
We should probably stay a couple of releases at this point, with this
warning, and the new behavior already configurable.
Elijah: notice how there's no mention of `git pull --merge`, because
in my opinion now we are telling users this is a temporary *warning*,
and the way to get rid of it properly should be very clear.
========================================================================
> future: pull: enable ff-only mode by default
The last patch finally enables the ff-only mode by default, and the
warning is removed forever.
The only thing that remains now is the fatal error:
The pull was not fast-forward, please either merge or rebase.
This fatal error is avoided by pull.mode=merge, pull.mode=rebase,
pull.rebase=true, pull.rebase=false, pull.rebase=$other, --merge, or
--rebase.
It *only* happens when the user does a vanilla "git pull", it's a
non-fast-forward update, the user has configured pull.mode=ff-only or
has not configured any of the above.
Is it more clear what is my proposal?
Cheers.
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