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
