Re: [PATCH 2/3] rebase: help users when dying with `preserve-merges`

[Philip Oakley <philipoakley@iee.email>]

On 26/05/2022 21:42, Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.email> writes:
>
>>>> Make the `rebase --abort` option available to allow users to remove
>>>> traces of any preserve-merges rebase, even if they had upgraded
>>>> during a rebase.
> This patch does not make it "available", though.

Yes it does. Sorry if the terminology or explanation was poor (here we 
are looking at the commit message, not the user facing message?).

Currently, if the user has an in-progress rebase with preserve-merges, 
and now using the latest Git, they will reach the fatal die(), even if 
they try any of the git status suggestions of --abort, --continue, etc.  
Essentially, it's a 'you shouldn't be here', lets stop right now, go 
straight to jail condition. We do want to permit the `rebase --abort` 
command option.

I can swap around the && condition so that it's clearer that we check 
the user isn't requesting an --abort before checking the internal 
directory and then dying.
> 	Suggest using `--abort` to get out of the situation after a
> 	failed preserve-rebase and remove traces of ...
>
> perhaps?
>
> I do think the suggestion is worth doing if a user ever gets into
> the situation, but how likely does it happen?  A user has to start
> "rebase -p" with older Git,

.. hit a conflict, seeks help. Helper bring a personal portable Git with 
latest version - Oops.

Or Helper, says "Oh, your version is old, upgrade, and that'll fix it", 
again Oops.

> wait until Git gets updated to a future
> version of Git that includes this change, and then say "rebase -p
> --continue"?
You don't need the -p there ;-)

For this change, the "git rebase --continue" will still die() with the 
fatal: message. We do not have a way to continue. However..

After this change, the "git rebase --abort" will properly clear and 
clean the repo/status so that the user can then choose what to do.

>
>>>>   	} else if (is_directory(merge_dir())) {
>>>>   		strbuf_reset(&buf);
>>>>   		strbuf_addf(&buf, "%s/rewritten", merge_dir());
>>>> -		if (is_directory(buf.buf)) {
>>>> -			die("`rebase -p` is no longer supported");
>>>> +		if (is_directory(buf.buf) && !(action == ACTION_ABORT)) {
>>>> +			die("`rebase --preserve-merges` (-p) is no longer supported.\n"
>>>> +			"Use `git rebase --abort` to terminate current rebase.\n"
>>>> +			"Or downgrade to v2.33, or earlier, to complete the rebase.\n");
>>>>   		} else {
>>>>   			strbuf_reset(&buf);
>>>>   			strbuf_addf(&buf, "%s/interactive", merge_dir());
>>> Existing issue: No _(), shouldn't we add it?
>> This `strbuf_addf` is forming a path for internal use. It just happens
>> to look like legible English ;-)
> I do not think Ævar meant "%s/interactive"; the enhanced message
> above that you inherited from the original "no longer supported"
> that was not marked for translation.
Ok.
>
>>> I wonder if we should use die_message() + advise() in these cases,
>>> i.e. stick to why we died in die_message() and have the advise() make
>>> suggestions, as e4921d877ab (tracking branches: add advice to ambiguous
>>> refspec error, 2022-04-01) does.
>> Ah, maybe it's my message.. that needs translating.
> Yup.
Ok, I'd add a separate patch for that.

> This whole '-p' business will go away in a few releases down, so a
> longer message give to the existing die() should be sufficient and
> there is no need for the choice between "yes, I am still weaning
> myself off of rebase -p and want to keep seeing the advice" and
> "thanks, I saw the message often enough, you no longer need to tell
> me how to get out", I would think.
I think it will take a long while for all the users, tools providers and 
distros to get beyond 2.33, so while each user may be weaned quickly, 
the generic problem is likely to continue to linger.


I hope to re-roll later next week. In general it's mainly tweaks and 
finesse.

Philip