RE: [PATCH] pull: abort by default if fast-forwarding is impossible

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 27, 2021, 04:12 UTC
Message-ID: <60d7faaf5311a_b8dfe208bd@natae.notmuch>
In-Reply-To: <20210627000855.530985-1-alexhenrie24@gmail.com>

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.

Show 10 quoted lines

> --- 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`?

Show 6 quoted lines

>  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?

Show 24 quoted lines

> --- 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