Update the die() preserve-merges messages to help some users
cover Update the die() preserve-merges messages to help some users
This small update to the die() preserve-merges messages is a response to the reported edge case in the Git-for-Windows googlegroups thread
[https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM] where even git rebase --continue would die.
It is most relevant for Windows because Visual Studio still offers the option to run git pull --preserve, therefore Git for Windows already applied these patches. The improvements are not specific to Windows, though, and should therefore also get into core Git, albeit at a more leisurely pace.
This is a companion patch series to
https://github.com/git-for-windows/git/pull/3708
Philip Oakley (2):
rebase: help user when dying with preserve-merges`
rebase: preserve is also a pull option, tell dying users
builtin/rebase.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1155%2Fdscho%2Fdie_preserve-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1155/dscho/die_preserve-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1155
Signature · 1 line
gitgitgadget
PATCH 1/2 rebase: help user when dying with preserve-merges`
From: Philip Oakley <[email protected]>
Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.
This sceario can occur if the user updates their Git, or switches to another newer version, after starting a preserve-merges rebase, commonly via the pull setting.
One trigger is 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 cause, i.e. the existence of the directory. The problem must be resolved manually, git rebase --<option> commands will die, or the user must downgrade. Also, note that the deleted options are no longer shown in the documentation.
--- builtin/rebase.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/rebase.c +3 -1
@@ -1181,7 +1181,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)strbuf_reset (&buf);strbuf_addf (&buf,"%s/rewritten" ,merge_dir ());if (is_directory (buf.buf)) { -die ("`rebase -p` is no longer supported" ); +die ("`rebase --preserve-merges` (-p) is no longer supported.\n" +"You still have a `.git/rebase-merge/rewritten` directory, \n" +"indicating a `rebase preserve-merge` is still in progress.\n" ); }else {strbuf_reset (&buf);strbuf_addf (&buf,"%s/interactive" ,merge_dir ());
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] rebase: help user when dying with preserve-merges`
Quoted 9 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.
This sceario can occur if the user updates their Git, or switches to another newer version, after starting a preserve-merges rebase, commonly via the pull setting.
Quoted 8 lines
One trigger is an unexpectedly difficult to resolve conflict, as reported on the
git-usersgroup.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)Tell the user the cause, i.e. the existence of the directory. The problem must be resolved manually,
git rebase --<option>commands will die, or the user must downgrade. Also, note that the deleted options are no longer shown in the documentation.
I can go and read the linked thread for the answer, but:
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1189
Quoted 8 lines
builtin/rebase.c +3
-1if (is_directory (buf.buf)) { -die ("`rebase -p` is no longer supported" ); +die ("`rebase --preserve-merges` (-p) is no longer supported.\n" +"You still have a `.git/rebase-merge/rewritten` directory, \n" +"indicating a `rebase preserve-merge` is still in progress.\n" ); }else {strbuf_reset (&buf);strbuf_addf (&buf,"%s/interactive" ,merge_dir ());
As much of an improvement this is, I'd be no closer to knowing what I should do at this point.
Should I "rm -rf" that directory, downgrade my version of git if I'd like to recover my work (as the message alludes to).
In either case I'd think that this is getting a bit past the length where we'd have just a die() v.s. splitting it into a die()/advise() pair. I.e. to have the advise() carry some bullet-point list about X/Y/Z solutions, with the die() being a brief ~"we did because xyz dir is still here".
Re: [PATCH 1/2] rebase: help user when dying with preserve-merges`
Quoted 32 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.
This sceario can occur if the user updates their Git, or switches to another newer version, after starting a preserve-merges rebase, commonly via the pull setting.
Quoted 8 lines
One trigger is an unexpectedly difficult to resolve conflict, as reported on the
git-usersgroup.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)Tell the user the cause, i.e. the existence of the directory. The problem must be resolved manually,
git rebase --<option>commands will die, or the user must downgrade. Also, note that the deleted options are no longer shown in the documentation.I can go and read the linked thread for the answer, but:
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1189
Quoted 8 lines
builtin/rebase.c +3
-1if (is_directory (buf.buf)) { -die ("`rebase -p` is no longer supported" ); +die ("`rebase --preserve-merges` (-p) is no longer supported.\n" +"You still have a `.git/rebase-merge/rewritten` directory, \n" +"indicating a `rebase preserve-merge` is still in progress.\n" ); }else {strbuf_reset (&buf);strbuf_addf (&buf,"%s/interactive" ,merge_dir ());As much of an improvement this is, I'd be no closer to knowing what I should do at this point.
Quoted 10 lines
Should I "rm -rf" that directory, downgrade my version of git if I'd like to recover my work (as the message alludes to).
In either case I'd think that this is getting a bit past the length where we'd have just a
die()v.s. splitting it into a die()/advise() pair. I.e. to have theadvise()carry some bullet-point list about X/Y/Z solutions, with thedie()being a brief ~"we did because xyz dir is still here".
Hi Ævar,
Exactly. This is a slightly special, but real, case. The previous message was essentially totally opaque to users. An "If I were you I wouldn't start from here" response is somewhat true, so we simply tell the user how they got to receive the fatal message. They can then take any of the options they choose.
Ultimately the user downgraded and managed to use "rebase --continue", as advised by Git, without the response "fatal:" to complete their old preserve-merges rebase.
They'll hit a similar fault in short order because when they next pull they'll be slipped into trying the preserve-merge rebase again - that's the 2/2 patch, making sure they know why.
Philip
Re: [PATCH 1/2] rebase: help user when dying with preserve-merges`
Quoted 51 lines · Philip Oakley
Quoted 32 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.
This sceario can occur if the user updates their Git, or switches to another newer version, after starting a preserve-merges rebase, commonly via the pull setting.
Quoted 8 lines
One trigger is an unexpectedly difficult to resolve conflict, as reported on the
git-usersgroup.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)Tell the user the cause, i.e. the existence of the directory. The problem must be resolved manually,
git rebase --<option>commands will die, or the user must downgrade. Also, note that the deleted options are no longer shown in the documentation.I can go and read the linked thread for the answer, but:
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1189
Quoted 8 lines
builtin/rebase.c +3
-1if (is_directory (buf.buf)) { -die ("`rebase -p` is no longer supported" ); +die ("`rebase --preserve-merges` (-p) is no longer supported.\n" +"You still have a `.git/rebase-merge/rewritten` directory, \n" +"indicating a `rebase preserve-merge` is still in progress.\n" ); }else {strbuf_reset (&buf);strbuf_addf (&buf,"%s/interactive" ,merge_dir ());As much of an improvement this is, I'd be no closer to knowing what I should do at this point.
Quoted 10 lines
Should I "rm -rf" that directory, downgrade my version of git if I'd like to recover my work (as the message alludes to).
In either case I'd think that this is getting a bit past the length where we'd have just a
die()v.s. splitting it into a die()/advise() pair. I.e. to have theadvise()carry some bullet-point list about X/Y/Z solutions, with thedie()being a brief ~"we did because xyz dir is still here".Hi Ævar,
Exactly. This is a slightly special, but real, case. The previous message was essentially totally opaque to users. An "If I were you I wouldn't start from here" response is somewhat true, so we simply tell the user how they got to receive the fatal message. They can then take any of the options they choose.
Quoted 3 lines
Ultimately the user downgraded and managed to use "rebase --continue", as advised by Git, without the response "fatal:" to complete their old preserve-merges rebase.
Right. I'm pointing out that in this proposed version of the die() message we stop just short of actually telling the user how to proceed.
I.e. just that they have a X directory, not that they should either remove X and lose their work, or downgrade git, proceed, and then upgrade git.
Quoted 3 lines
They'll hit a similar fault in short order because when they next
pullthey'll be slipped into trying the preserve-merge rebase again - that's the 2/2 patch, making sure they know why.
Well, this is "rebase". You can have been running rebases in a repository without ever having any interactions with remotes.
And even if you had interactions with remotes you might be doing so via "git fetch" followed by "git rebase", and might not ever invoke "git pull".
And even if you did a "git pull" later shouldn't the error you got here be sufficiently stand-alone as to tell you what to do, without needing a later "pull"?
Re: [PATCH 1/2] rebase: help user when dying with preserve-merges`
Quoted 56 lines · Ævar Arnfjörð Bjarmason
Quoted 51 lines · Philip Oakley
Quoted 32 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.
This sceario can occur if the user updates their Git, or switches to another newer version, after starting a preserve-merges rebase, commonly via the pull setting.
Quoted 8 lines
One trigger is an unexpectedly difficult to resolve conflict, as reported on the
git-usersgroup.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)Tell the user the cause, i.e. the existence of the directory. The problem must be resolved manually,
git rebase --<option>commands will die, or the user must downgrade. Also, note that the deleted options are no longer shown in the documentation.I can go and read the linked thread for the answer, but:
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1189
Quoted 8 lines
builtin/rebase.c +3
-1if (is_directory (buf.buf)) { -die ("`rebase -p` is no longer supported" ); +die ("`rebase --preserve-merges` (-p) is no longer supported.\n" +"You still have a `.git/rebase-merge/rewritten` directory, \n" +"indicating a `rebase preserve-merge` is still in progress.\n" ); }else {strbuf_reset (&buf);strbuf_addf (&buf,"%s/interactive" ,merge_dir ());As much of an improvement this is, I'd be no closer to knowing what I should do at this point.
Quoted 10 lines
Should I "rm -rf" that directory, downgrade my version of git if I'd like to recover my work (as the message alludes to).
In either case I'd think that this is getting a bit past the length where we'd have just a
die()v.s. splitting it into a die()/advise() pair. I.e. to have theadvise()carry some bullet-point list about X/Y/Z solutions, with thedie()being a brief ~"we did because xyz dir is still here".Hi Ævar,
Exactly. This is a slightly special, but real, case. The previous message was essentially totally opaque to users. An "If I were you I wouldn't start from here" response is somewhat true, so we simply tell the user how they got to receive the fatal message. They can then take any of the options they choose.
Quoted 3 lines
Ultimately the user downgraded and managed to use "rebase --continue", as advised by Git, without the response "fatal:" to complete their old preserve-merges rebase.
Quoted 6 lines
Right. I'm pointing out that in this proposed version of the
die()message we stop just short of actually telling the user how to proceed.I.e. just that they have a X directory, not that they should either remove X and lose their work, or downgrade git, proceed, and then upgrade git.
In a sense, that is it. They are in a difficult place, but with at least a little information to seek further information and start making their choices. Before, they (users in difficulty) were rather uninformed.
Quoted 5 lines
Quoted 3 lines
They'll hit a similar fault in short order because when they next
pullthey'll be slipped into trying the preserve-merge rebase again - that's the 2/2 patch, making sure they know why.Well, this is "rebase". You can have been running rebases in a repository without ever having any interactions with remotes.
True. That is a possibility. But we have also removed the preserve option for interaction with remotes as well.
Quoted 8 lines
And even if you had interactions with remotes you might be doing so via "git fetch" followed by "git rebase", and might not ever invoke "git pull".
And even if you did a "git pull" later shouldn't the error you got here be sufficiently stand-alone as to tell you what to do, without needing a later "pull"?
Why are we delaying telling the user that they would have problems there as well? It shouldn't be a game about how many ways we can trip up the user.
It's a pity the problem has split into the different ways into disaster.
Re: [PATCH 1/2] rebase: help user when dying with preserve-merges`
Quoted 60 lines · Philip Oakley
Quoted 59 lines · Ævar Arnfjörð Bjarmason
Quoted 51 lines · Philip Oakley
Quoted 32 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.
This sceario can occur if the user updates their Git, or switches to another newer version, after starting a preserve-merges rebase, commonly via the pull setting.
Quoted 8 lines
One trigger is an unexpectedly difficult to resolve conflict, as reported on the
git-usersgroup.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)Tell the user the cause, i.e. the existence of the directory. The problem must be resolved manually,
git rebase --<option>commands will die, or the user must downgrade. Also, note that the deleted options are no longer shown in the documentation.I can go and read the linked thread for the answer, but:
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1189
Quoted 8 lines
builtin/rebase.c +3
-1if (is_directory (buf.buf)) { -die ("`rebase -p` is no longer supported" ); +die ("`rebase --preserve-merges` (-p) is no longer supported.\n" +"You still have a `.git/rebase-merge/rewritten` directory, \n" +"indicating a `rebase preserve-merge` is still in progress.\n" ); }else {strbuf_reset (&buf);strbuf_addf (&buf,"%s/interactive" ,merge_dir ());As much of an improvement this is, I'd be no closer to knowing what I should do at this point.
Quoted 10 lines
Should I "rm -rf" that directory, downgrade my version of git if I'd like to recover my work (as the message alludes to).
In either case I'd think that this is getting a bit past the length where we'd have just a
die()v.s. splitting it into a die()/advise() pair. I.e. to have theadvise()carry some bullet-point list about X/Y/Z solutions, with thedie()being a brief ~"we did because xyz dir is still here".Hi Ævar,
Exactly. This is a slightly special, but real, case. The previous message was essentially totally opaque to users. An "If I were you I wouldn't start from here" response is somewhat true, so we simply tell the user how they got to receive the fatal message. They can then take any of the options they choose.
Quoted 3 lines
Ultimately the user downgraded and managed to use "rebase --continue", as advised by Git, without the response "fatal:" to complete their old preserve-merges rebase.
Right. I'm pointing out that in this proposed version of the
die()message we stop just short of actually telling the user how to proceed.
Quoted 6 lines
Quoted 3 lines
I.e. just that they have a X directory, not that they should either remove X and lose their work, or downgrade git, proceed, and then upgrade git.
In a sense, that is it. They are in a difficult place, but with at least a little information to seek further information and start making their choices. Before, they (users in difficulty) were rather uninformed.
Yes, it's definitely an improvement. I'm just wondering if we can tell them a bit more so that they're not needing to search "what do do about .git/rebase-merge/rewritten" on Google/stackoverflow/whatever.
I.e. can we just specifically say that they either need to downgrade, continue, upgrade, or alternatively rm -rf it, depending on wheher they'd like to not lose their work, or if that's OK?
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
They'll hit a similar fault in short order because when they next
pullthey'll be slipped into trying the preserve-merge rebase again - that's the 2/2 patch, making sure they know why.Well, this is "rebase". You can have been running rebases in a repository without ever having any interactions with remotes.
True. That is a possibility. But we have also removed the preserve option for interaction with remotes as well.
*Nod*, presumably you mean its removal from "builtin/pull.c" etc.
Quoted 5 lines
Quoted 5 lines
And even if you had interactions with remotes you might be doing so via "git fetch" followed by "git rebase", and might not ever invoke "git pull".
Quoted 7 lines
Quoted 3 lines
And even if you did a "git pull" later shouldn't the error you got here be sufficiently stand-alone as to tell you what to do, without needing a later "pull"?
Why are we delaying telling the user that they would have problems there as well? It shouldn't be a game about how many ways we can trip up the user.
It's a pity the problem has split into the different ways into disaster.
I'm just saying that the reason the message is a bit confusing is because we *are* delaying it. I.e. we could give a more specific error message in builtin/pull.c, but the "preserve merges" scaffolding was already removed from it.
Which is why we're doing it in builtin/rebase.c, and suggesting "tweak your pull settings", to a user who may or may not have arrived there after a "pull" (and may not be using remotes at all).
I think overall that's OK in this case, i.e. this fix shouldn't be held up by putting that scaffolding in place in builtin/pull.c again to slightly improve the error.
But perhaps in the 2/2 error just say instead: "if you arrived here via 'git pull' ....". I.e. no extra code, just a slight tweak to the message, and any user seeing it by invoking "rebase" directly won't be further confused about why the error is saying something about "pull" (which they didn't run).
PATCH 2/2 rebase: `preserve` is also a pull option, tell dying users
In reply to Johannes Schindelin via GitGitGadgetFrom: Philip Oakley <[email protected]>
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.
--- builtin/rebase.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
builtin/rebase.c +4 -1
@@ -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." ));if (action !=ACTION_NONE && total_argc !=2 ) { usage_with_options(builtin_rebase_usage,
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Quoted 20 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
The
--preserve-mergesoption 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-mergesdie message to direct users to this option and it's locations.Signed-off-by Philip Oakley <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1211
Quoted 8 lines
builtin/rebase.c +4
-1@@ -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: preserveI.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?
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Quoted 44 lines · Ævar Arnfjörð Bjarmason
Quoted 20 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
The
--preserve-mergesoption 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-mergesdie message to direct users to this option and it's locations.Signed-off-by Philip Oakley <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1211
Quoted 8 lines
builtin/rebase.c +4
-1@@ -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: preserveI.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.
Quoted 5 lines
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.
Philip
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Hi Ævar,
Quoted 62 lines · Philip Oakley
Quoted 44 lines · Ævar Arnfjörð Bjarmason
Quoted 20 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
The
--preserve-mergesoption 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-mergesdie message to direct users to this option and it's locations.Signed-off-by Philip Oakley <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1211
Quoted 8 lines
builtin/rebase.c +4
-1@@ -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: preserveI.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.Quoted 5 lines
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.
Quoted 9 lines
If we are to remove a capability the
give up and diemessages 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
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Quoted 74 lines · Philip Oakley
Hi Ævar,
Quoted 62 lines · Philip Oakley
Quoted 44 lines · Ævar Arnfjörð Bjarmason
Quoted 20 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
The
--preserve-mergesoption 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-mergesdie message to direct users to this option and it's locations.Signed-off-by Philip Oakley <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1211
Quoted 8 lines
builtin/rebase.c +4
-1@@ -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: preserveI.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.Quoted 5 lines
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.Quoted 9 lines
If we are to remove a capability the
give up and diemessages 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.
Quoted 5 lines
I don't think that came out well. For this message, it's about reminding users that they may *also* be using the
preserveoption 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".
I see. What I was confused with (and still find confusing) is that the error here says "[and the pull.rebase=preserve config] may also invoke this option".
But it doesn't do that at all, it *used to*, but now "git pull" will promptly fail on it as well.
So this is really just trying to tell us something like this, right?
Hey, you invoked --preserve-merges, bad mojo! Also: if you invoke 'git pull' and used the related config you may or may not have used, that'll die too. So maybe check that out?
:)
Anyway, to the user the "invoke this option" isn't strictly accurate, but they probably don't care/know the difference, so I don't mind per-se, I was just wondering if I was missing something.
I do wonder if this part is needed at all. I.e. at this point this will only benefit users who are *manually* invoking "git rebase --preserve-merges", everyone else (including that "VS" example you mentioned) will go through "git pull" and see the other error.
IOW maybe just saying "this option is an error now" when we intercept the option is enough, and ditto for "this config is an error now" when we intercept the config, and not assume that a user using the option must also be using the config is simpler/sufficient.
Just my 0.02. For my part whahtever you decide to do here is fine by me, I was just wondering if I'd misundestood the whole flow around this legacy option and how we invoke the small parts of die() boilerplate left of it.
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Sorry for the delay. I've got some on-going family matters to attend to which have limited my availablilty.
Quoted 80 lines · Ævar Arnfjörð Bjarmason
Quoted 78 lines · Philip Oakley
Hi Ævar,
Quoted 62 lines · Philip Oakley
Quoted 44 lines · Ævar Arnfjörð Bjarmason
Quoted 20 lines · Philip Oakley via GitGitGadget
From: Philip Oakley <[email protected]>
The
--preserve-mergesoption 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-mergesdie message to direct users to this option and it's locations.Signed-off-by Philip Oakley <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1211
Quoted 8 lines
builtin/rebase.c +4
-1@@ -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: preserveI.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.Quoted 5 lines
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.Quoted 9 lines
If we are to remove a capability the
give up and diemessages 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
preserveoption within their pull options, and not just within their rebase command's options.
Quoted 7 lines
Quoted 1 line
A case of "don't use it here, and don't use it there either, please check".
I see. What I was confused with (and still find confusing) is that the error here says "[and the pull.rebase=preserve config] may also invoke this option".
But it doesn't do that at all, it *used to*, but now "git pull" will promptly fail on it as well.
Ok, That's probably my local idiom that confusing the now of the error and the past when the config would have been set up. I'll see if I can clarify that better.
Quoted 17 lines
So this is really just trying to tell us something like this, right?
Hey, you invoked --preserve-merges, bad mojo! Also: if you invoke 'git pull' and used the related config you may or may not have used, that'll die too. So maybe check that out?
:)
Anyway, to the user the "invoke this option" isn't strictly accurate, but they probably don't care/know the difference, so I don't mind per-se, I was just wondering if I was missing something.
I do wonder if this part is needed at all. I.e. at this point this will only benefit users who are *manually* invoking "git rebase --preserve-merges", everyone else (including that "VS" example you mentioned) will go through "git pull" and see the other error.
Quoted 4 lines
IOW maybe just saying "this option is an error now" when we intercept the option is enough, and ditto for "this config is an error now" when we intercept the config, and not assume that a user using the option must also be using the config is simpler/sufficient.
Error reporting is a tricky subject, especially when we add the human error and misunderstanding aspects into the mix.
Quoted 5 lines
Just my 0.02. For my part whahtever you decide to do here is fine by me, I was just wondering if I'd misundestood the whole flow around this legacy option and how we invoke the small parts of
die()boilerplate left of it.
Yep, it is tricky talking about disappeared options, the error messages, and the multiple ways that show themselves. In the originating edge case it wasn't helped by a Git advice message that contained the failing command line (git rebase --continue) suggestion. In general I was trying to cover all the possible sources in all the places the 'error' may occur.
I'll have another look at the ways these edge cases could appear, and try an improve the commit message explanations where the diff doesn't show sufficient context. It'll be at least next week.
Philip
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Hi Philip,
Quoted 3 lines · Philip Oakley
I'll have another look at the ways these edge cases could appear, and try an improve the commit message explanations where the diff doesn't show sufficient context. It'll be at least next week.
Can I punt this patch series back to you? (I sent it upstream on your behalf because I had assumed that you'd want me to, sorry for misunderstanding your intentions.)
Ciao,
Dscho
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Hi Dscho,
Quoted 11 lines · Johannes Schindelin
Hi Philip,
Quoted 3 lines · Philip Oakley
I'll have another look at the ways these edge cases could appear, and try an improve the commit message explanations where the diff doesn't show sufficient context. It'll be at least next week.
Can I punt this patch series back to you? (I sent it upstream on your behalf because I had assumed that you'd want me to, sorry for misunderstanding your intentions.)
I'm happy to continue to work on this series, and am grateful for your support in pushing it through GGG. How is it best to transfer the 'ownership' at GGG?
I've still got some family issues so it'll be later in the week, or even next week before I can update the series.
Thanks.
Philip
Re: [PATCH 2/2] rebase: `preserve` is also a pull option, tell dying users
Hi Philip,
Quoted 8 lines · Philip Oakley
Quoted 7 lines · Johannes Schindelin
Hi Philip,
Quoted 3 lines · Philip Oakley
I'll have another look at the ways these edge cases could appear, and try an improve the commit message explanations where the diff doesn't show sufficient context. It'll be at least next week.
Quoted 7 lines
Quoted 4 lines
Can I punt this patch series back to you? (I sent it upstream on your behalf because I had assumed that you'd want me to, sorry for misunderstanding your intentions.)
I'm happy to continue to work on this series, and am grateful for your support in pushing it through GGG. How is it best to transfer the 'ownership' at GGG?
Sadly, I don't know of any way how I could transfer ownership to you, but maybe you can just open a new one and reference the first thread in the cover letter?
Quoted 2 lines
I've still got some family issues so it'll be later in the week, or even next week before I can update the series.
Sorry to hear that you have issues. These are tough times, and I feel for you.
Ciao,
Dscho
Hi Philip
Quoted 9 lines · Johannes Schindelin via GitGitGadget
This small update to the
die()preserve-merges messages is a response to the reported edge case in the Git-for-Windows googlegroups thread
[https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM] where even git rebase --continue would die.It is most relevant for Windows because Visual Studio still offers the option to run git pull --preserve, therefore Git for Windows already applied these patches. The improvements are not specific to Windows, though, and should therefore also get into core Git, albeit at a more leisurely pace.
I think the new messages are an improvement, I was wondering how difficult it would be to allow the user to run rebase --abort so they can at least easily start again with --rebase-merges.
Best Wishes
Phillip
Quoted 15 lines
This is a companion patch series to
https://github.com/git-for-windows/git/pull/3708Philip Oakley (2):
rebase: help user when dying with preserve-merges`
rebase:preserveis also a pull option, tell dying usersbuiltin/rebase.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)base-commit:
e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1155%2Fdscho%2Fdie_preserve-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1155/dscho/die_preserve-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1155
Hi Phillip,
Quoted 17 lines · Phillip Wood
Hi Philip
Quoted 7 lines · Johannes Schindelin via GitGitGadget
This small update to the
die()preserve-merges messages is a response to the
reported edge case in the Git-for-Windows googlegroups thread
[https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM] where even git
rebase --continue would die.Quoted 6 lines
It is most relevant for Windows because Visual Studio still offers the option to run git pull --preserve, therefore Git for Windows already applied
these patches. The improvements are not specific to Windows, though, and should therefore also get into core Git, albeit at a more leisurely pace.
Quoted 3 lines
I think the new messages are an improvement, I was wondering how difficult it would be to allow the user to run rebase --abort so they can at least easily start again with --rebase-merges.
In this case, the user (another Phillip), couldn't run rebase --continue without getting a fatal: report. A code inspection showed that was one of the first tests so I don't believe they could run --quit or abort either!
We eventually nailed it down to being an update of Git, after getting into a bad conflict resolution, so the update refused to do any rebase commands! Later they tried downgrading and finishing the rebase that way in the usual 'hack & hope' way. Luckily they had a backup from the time of the update, which was able to confirm the presence of the indicative directory (which is an implementation detail).
If the rebase abort was moved earlier in the code, then it might work, but we'd still need to keep the clean-up code for a non-existent option, which is less than ideal .
Philip
Quoted 23 lines
Best Wishes
Phillip
Quoted 17 lines
This is a companion patch series to
https://github.com/git-for-windows/git/pull/3708Philip Oakley (2):
rebase: help user when dying with preserve-merges`
rebase:preserveis also a pull option, tell dying users
builtin/rebase.c| 9 +++++++--1 file changed, 7 insertions(+), 2 deletions(-)base-commit:
e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464ePublished-As:
https://github.com/gitgitgadget/git/releases/tag/pr-1155%2Fdscho%2Fdie_preserve-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-1155/dscho/die_preserve-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1155