Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 22, 2022, 15:34 UTC
Message-ID: <220222.868ru27vtt.gmgdl@evledraar.gmail.com>
In-Reply-To: <eb5871db95b12500cc0a6b8b0e3a82ed9e8fcfbd.1645526016.git.gitgitgadget@gmail.com>

On Tue, Feb 22 2022, Philip Oakley via GitGitGadget wrote:

Show 28 quoted lines

> From: Philip Oakley <philipoakley@iee.email>
>
> The `--preserve-merges` option was removed by v2.35.0. However
> users may not be aware that it is also a Pull option, and it is
> still offered by major IDE vendors such as Visual Studio.
>
> Extend the `--preserve-merges` die message to direct users to
> this option and it's locations.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.email>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/rebase.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 07221d0ae41..97f704bb297 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1205,7 +1205,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			     builtin_rebase_usage, 0);
>  
>  	if (preserve_merges_selected)
> -		die(_("--preserve-merges was replaced by --rebase-merges"));
> +		die(_("--preserve-merges was replaced by --rebase-merges\n"
> +			"Also, check your `pull` configuration settings\n"
> +			"`git config --show-scope --show-origin --get-regexp 'pull.*'`\n"
> +			"which may also invoke this option."));

I may be missing some subtlety, but how is the user ever going to need to check their config?

After 52f1e82178e (pull: remove support for `--rebase=preserve`, 2021-09-07) we:

    $ git -c pull.rebase=preserve pull
    error: rebase.c:29: preserve: 'preserve' superseded by 'merges'
    fatal: builtin/pull.c:45: Invalid value for pull.rebase: preserve

I.e. we'd error before this, and the "preserve_merges_selected" variable being checked here is not affected by config, i.e. we only ever got to this "via config" route if "pull" et al was invoking us.

But now that command dies.

If there is still a codepath where we call "rebase --preserve-merges" on the basis of config that I've missed, shouldn't this die() be happening there?