RE: [PATCH] pull: abort by default if fast-forwarding is impossible
From: Felipe Contreras <hidden>
Date: 2021-06-27 04:17:01
Alex Henrie wrote:
The behavior of warning but merging anyway is difficult to explain to users because it sends mixed signals. End the confusion by firmly requiring the user to specify whether they want a merge or a rebase.
When were users warned about this change? Generally before making such big UI changes to core commands there needs to be a deprecation period before pulling the trigger. Second, supposing this was a proposal to add such warning, we would need a configuration so the user can decide to retain the old behavior, or move to the new one. What is that configuration? How can a user choose to enable the new behavior in v2.32 (or any other version)? Also, a bunch of tests are broken after this change: t4013-diff-various.sh t5521-pull-options.sh t5524-pull-msg.sh t5520-pull.sh t5553-set-upstream.sh t5604-clone-reference.sh t6409-merge-subtree.sh t6402-merge-rename.sh t6417-merge-ours-theirs.sh t7601-merge-pull-config.sh t7603-merge-reduce-heads.sh If you didn't mean this patch to be applied then perhaps add the RFC prefix.
quoted hunk ↗ jump to hunk
--- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt@@ -16,13 +16,17 @@ DESCRIPTION ----------- Incorporates changes from a remote repository into the current -branch. In its default mode, `git pull` is shorthand for -`git fetch` followed by `git merge FETCH_HEAD`. +branch. `git pull` is shorthand for `git fetch` followed by +`git merge FETCH_HEAD` or `git rebase FETCH_HEAD`.
Is it? If I have a pull.rebase=merges is `git pull` the same as doing `git rebase FETCH_HEAD`?
More precisely, 'git pull' runs 'git fetch' with the given -parameters and calls 'git merge' to merge the retrieved branch -heads into the current branch. -With `--rebase`, it runs 'git rebase' instead of 'git merge'. +parameters and then, by default, attempts to fast-forward the +current branch to the remote branch head.
OK.
+If fast-forwarding +is not possible because the local and remote branches have +diverged, the `--rebase` option causes 'git rebase' to be +called to rebase the local branch upon the remote branch,
Does --rebase only work when a fast-forward isn't possible.
+and +the `--no-rebase` option causes 'git merge' to be called to +merge the retrieved branch heads into the current branch.
Isn't merge called when a fast-forward is possible?
quoted hunk ↗ jump to hunk
--- a/builtin/pull.c +++ b/builtin/pull.c@@ -925,20 +925,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_ return ret; } -static void show_advice_pull_non_ff(void) +static void die_pull_non_ff(void) { - advise(_("Pulling without specifying how to reconcile divergent branches is\n" - "discouraged. You can squelch this message by running one of the following\n" - "commands sometime before your next pull:\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. You can also pass --rebase, --no-rebase,\n" - "or --ff-only on the command line to override the configured default per\n" - "invocation.\n")); + die(_("Pulling without specifying how to reconcile divergent branches is not\n" + "possible. You can squelch this message by running one of the following\n" + "commands sometime before your next pull:\n"
Is squelching this message really what we are aming for?
+ " git config pull.rebase false # merge\n" + " git config pull.rebase true # rebase\n" + " git config pull.ff only # fast-forward only\n"
`git config pull.ff only` will get rid of this messge, but the pull would still fail, only that it will be a different message: fatal: Not possible to fast-forward, aborting. Doesn't seem very user-friendly.
+ "You can replace \"git config\" with \"git config --global\" to set a default\n" + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" + "or --ff-only on the command line to override the configured default per\n" + "invocation.\n"));
Can I? git -c pull.rebase=true pull --ff-only `--ff-only` doesn't seem to be overriding the configuration. In addition to all the issues I've already pointed out, the message seems rather big for a die(). I think this is close to the end-goal we should be aiming for, but there's a lot that is missing before we should even consider flipping the switch. Cheers. -- Felipe Contreras