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

Hi Ævar,

On 22/02/2022 15:56, Philip Oakley wrote:

On 22/02/2022 15:34, Ævar Arnfjörð Bjarmason wrote:

quoted

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

quoted

From: Philip Oakley <redacted>

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 <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---
 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?

Hi Ævar,

The preserve merges is a pull config item, still supported by Visual
Studio, so could easily be set for many users. When they click on the
"Pull" menu and get the old die() fatal: message they won't know why
this happened or how to resolve it. Hence the extra info.

If the user was actually running a rebase command then, yes, they
would/should be reasonably OK with the original error message telling
them what was wrong with their command line. But a `git pull`?
especially as the manual says nothing about preserve merges any more.

If we are to remove a capability the `give up and die` messages should
be helpful in the edge cases. The issue (needing to check the pull, as
well as the rebase configs) has already shown on the git-users list. It
took a while to workout how it all happened.

Not sure if I've covered your concerns properly, but this is one of the
places that can and warn the user.

I don't think that came out well. For this message, it's about reminding
users that they may *also* be using the `preserve` option within their
pull options, and not just within their rebase command's options.

A case of "don't use it here, and don't use it there either, please check".

Philip