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

Commit Message

Philip Oakley May 26, 2022, 9:21 a.m. UTC

From: Philip Oakley <[e-mail]>

Git will die if a "rebase --preserve-merges" is in progress.
Users cannot --quit, --abort or --continue the rebase.

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.

One trigger was an unexpectedly difficult to resolve conflict, as
reported on the `git-users` group.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)

Tell the user the options to resolve the problem manually.

Signed-off-by: Philip Oakley <[e-mail]>
---
 builtin/rebase.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 26, 2022, 9:43 a.m. UTC | #1

On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:

> From: Philip Oakley <[e-mail]>
>
> Git will die if a "rebase --preserve-merges" is in progress.
> Users cannot --quit, --abort or --continue the rebase.
>
> 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.
>
> One trigger was an unexpectedly difficult to resolve conflict, as
> reported on the `git-users` group.
> (https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)
>
> Tell the user the options to resolve the problem manually.
>
> Signed-off-by: Philip Oakley <[e-mail]>
> ---
>  builtin/rebase.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6ce7e98a6f1..aada25a8870 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1182,8 +1182,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	} 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?

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.

But then again adding new advice is currently a bit of an excercise in
boilerplate, and this seems fine for a transitory option.

I think you don't need to add a trailing \n though...

Philip Oakley May 26, 2022, 11:44 a.m. UTC | #2

On 26/05/2022 10:43, Ævar Arnfjörð Bjarmason wrote:
> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: Philip Oakley <[e-mail]>
>>
>> Git will die if a "rebase --preserve-merges" is in progress.
>> Users cannot --quit, --abort or --continue the rebase.
>>
>> 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.
>>
>> One trigger was an unexpectedly difficult to resolve conflict, as
>> reported on the `git-users` group.
>> (https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)
>>
>> Tell the user the options to resolve the problem manually.
>>
>> Signed-off-by: Philip Oakley <[e-mail]>
>> ---
>>  builtin/rebase.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 6ce7e98a6f1..aada25a8870 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1182,8 +1182,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  	} 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 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.
>
> But then again adding new advice is currently a bit of an excercise in
> boilerplate, and this seems fine for a transitory option.
I can go with that ;-)
>
> I think you don't need to add a trailing \n though...
Oops, just a little extra line spacing for emphasis maybe ?

Junio C Hamano May 26, 2022, 8:42 p.m. UTC | #3

Philip Oakley <[e-mail]> 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.  

	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, wait until Git gets updated to a future
version of Git that includes this change, and then say "rebase -p
--continue"?

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

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

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.

Thanks.

Philip Oakley May 27, 2022, 12:58 p.m. UTC | #4

On 26/05/2022 21:42, Junio C Hamano wrote:
> Philip Oakley <[e-mail]> 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

Junio C Hamano May 27, 2022, 3:54 p.m. UTC | #5

Philip Oakley <[e-mail]> writes:

> On 26/05/2022 21:42, Junio C Hamano wrote:
>> Philip Oakley <[e-mail]> 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?).

Sorry, you're right.  I misread the new "&&" condition in the patch.

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

Thanks.

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6ce7e98a6f1..aada25a8870 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1182,8 +1182,10 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} 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());