Die preserve ggg #1242
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1110,8 +1110,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) | |
| PARSE_OPT_NOARG | PARSE_OPT_NONEG, | ||
|
There was a problem hiding this comment. On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
> From: Philip Oakley <[email protected]>
>
> 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 <[email protected]>
> ---
> 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...
There was a problem hiding this comment. On the Git mailing list, Philip Oakley wrote (reply to this): On 26/05/2022 10:43, Ævar Arnfjörð Bjarmason wrote:
> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: Philip Oakley <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 ?
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Philip Oakley <[email protected]> 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.There was a problem hiding this comment. On the Git mailing list, Philip Oakley wrote (reply to this): On 26/05/2022 21:42, Junio C Hamano wrote:
> Philip Oakley <[email protected]> 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
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Philip Oakley <[email protected]> writes:
> On 26/05/2022 21:42, Junio C Hamano wrote:
>> Philip Oakley <[email protected]> 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.There was a problem hiding this comment. On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
> From: 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 also direct users to
> the use of the `preserve` option in the `pull` config.
>
> Signed-off-by: Philip Oakley <[email protected]>
> ---
> builtin/rebase.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index aada25a8870..6fc0aaebbb8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1205,7 +1205,8 @@ 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"
> + "Your `pull` configuration, may also invoke this option."));
>
> if (action != ACTION_NONE && total_argc != 2) {
> usage_with_options(builtin_rebase_usage,
Ditto 2/3 about maybe die_message() + advise(). In this case that has
the slight advantace of allowing us to keep the existing translated
string as-is.
But also, is *our* pull configuration causing us to end up here? I
vaguely recall that being discussed (probably in answer to a question of
mine) in the earlier round, or is this the IDE picking it up & invoking
us like this?
There was a problem hiding this comment. On the Git mailing list, Philip Oakley wrote (reply to this): hi,
On 26/05/2022 10:50, Ævar Arnfjörð Bjarmason wrote:
> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: 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 also direct users to
>> the use of the `preserve` option in the `pull` config.
>>
>> Signed-off-by: Philip Oakley <[email protected]>
>> ---
>> builtin/rebase.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index aada25a8870..6fc0aaebbb8 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1205,7 +1205,8 @@ 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"
>> + "Your `pull` configuration, may also invoke this option."));
>>
>> if (action != ACTION_NONE && total_argc != 2) {
>> usage_with_options(builtin_rebase_usage,
> Ditto 2/3 about maybe die_message() + advise().
I'm not that enamoured about hiding die message details behind an advice
option. In this case it not meant to be a regular reminder type thing,
rather a one-off fix-it-forever sort of `advice'. At least that my
reasoning.
> In this case that has
> the slight advantace of allowing us to keep the existing translated
> string as-is.
>
> But also, is *our* pull configuration causing us to end up here?
Yes, but. The extra message is about fixing all places that the user may
have setup a config for using preserve-merges, not just here. The fact
that IDEs offer a menu for adding that setting makes it easy for users
to get into this.
I'd agree that pull already has detection for this, but I was looking to
avoid the 'fool me once, fool me twice' scenarios.
It could be dropped if thought over zealous.
> I
> vaguely recall that being discussed (probably in answer to a question of
> mine) in the earlier round, or is this the IDE picking it up & invoking
> us like this?
>
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Philip Oakley via GitGitGadget" <[email protected]> writes:
> From: Philip Oakley <[email protected]>
>
> The `--preserve-merges` option was removed by v2.35.0. However
Are you sure about that?
52f1e821 (pull: remove support for `--rebase=preserve`, 2021-09-07)
that is in v2.34.0 and above dropped pull.rebase=preserve from the
Documentation/config/pull.txt (and others). My local collection
of various Git versions agrees with me. "git help config" from
2.34.0 does not list preserve as a valid choice, but 2.33.0 does.
> 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 also direct users to
> the use of the `preserve` option in the `pull` config.
>
> Signed-off-by: Philip Oakley <[email protected]>
> ---
> builtin/rebase.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index aada25a8870..6fc0aaebbb8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1205,7 +1205,8 @@ 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"
> + "Your `pull` configuration, may also invoke this option."));
What is a `pull` configuration? Our configuration variable names
all have at least one dot in it. I think it is better to be
explicit to clarify what exactly we are suggesting to fix.
"Your `pull.rebase` configuration may be set to 'preserve', which is
no longer supported; use 'merges' instead", or somesuch?
Thanks.There was a problem hiding this comment. On the Git mailing list, Philip Oakley wrote (reply to this): On 26/05/2022 21:55, Junio C Hamano wrote:
> "Philip Oakley via GitGitGadget" <[email protected]> writes:
>
>> From: Philip Oakley <[email protected]>
>>
>> The `--preserve-merges` option was removed by v2.35.0. However
> Are you sure about that?
Not any more. I think that was because of the version the user reported...
It's clearly wrong, as the down grade is to 2.33.0
> 52f1e821 (pull: remove support for `--rebase=preserve`, 2021-09-07)
> that is in v2.34.0 and above dropped pull.rebase=preserve from the
> Documentation/config/pull.txt (and others). My local collection
> of various Git versions agrees with me. "git help config" from
> 2.34.0 does not list preserve as a valid choice, but 2.33.0 does.
>
>> 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 also direct users to
>> the use of the `preserve` option in the `pull` config.
>>
>> Signed-off-by: Philip Oakley <[email protected]>
>> ---
>> builtin/rebase.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index aada25a8870..6fc0aaebbb8 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1205,7 +1205,8 @@ 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"
>> + "Your `pull` configuration, may also invoke this option."));
> What is a `pull` configuration?
I was thinking of 'those that relate to the pull command';-)
> Our configuration variable names
> all have at least one dot in it. I think it is better to be
> explicit to clarify what exactly we are suggesting to fix.
>
> "Your `pull.rebase` configuration may be set to 'preserve', which is
> no longer supported; use 'merges' instead", or somesuch?
That's a lot better. I'll borrow that..
P.There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Philip Oakley via GitGitGadget" <[email protected]> writes:
> From: Philip Oakley <[email protected]>
>
> The `--preserve-merges` option was removed by v2.34.0. However
> users may not be aware that it is also a Pull configuration option,
> which is still offered by major IDE vendors such as Visual Studio.
>
> Extend the `--preserve-merges` die message to also direct users to
> the possible use of the `preserve` option in the `pull.rebase` config.
> This is an additional 'belt and braces' information statement.
>
> Signed-off-by: Philip Oakley <[email protected]>
> ---
> builtin/rebase.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 17cc776b4b1..5f8921551e1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1205,7 +1205,9 @@ 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"
> + "Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
> + "which is no longer supported; use 'merges' instead"));
"be set" -> "be set".
I am not sure how this helps anybody, though.
When pull.rebase is parsed, rebase.c::rebase_parse_value() is called
from builtin/pull.c::parse_config_rebase() and would trigger an
error, whether it comes from the pull.rebase or the branch.*.rebase
configuration variable. An error() message already said that
'preserve' was removed and 'merges' would be a replacement when it
happened.
If the user has *not* reached this die() due to a configuration
variable, then there is not much point giving this new message,
either.There was a problem hiding this comment. On the Git mailing list, Philip Oakley wrote (reply to this): Sorry for delay, I had other family priorities to attend to.
On 06/06/2022 18:57, Junio C Hamano wrote:
> "Philip Oakley via GitGitGadget" <[email protected]> writes:
>
>> From: Philip Oakley <[email protected]>
>>
>> The `--preserve-merges` option was removed by v2.34.0. However
>> users may not be aware that it is also a Pull configuration option,
>> which is still offered by major IDE vendors such as Visual Studio.
>>
>> Extend the `--preserve-merges` die message to also direct users to
>> the possible use of the `preserve` option in the `pull.rebase` config.
>> This is an additional 'belt and braces' information statement.
>>
>> Signed-off-by: Philip Oakley <[email protected]>
>> ---
>> builtin/rebase.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 17cc776b4b1..5f8921551e1 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1205,7 +1205,9 @@ 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"
>> + "Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
>> + "which is no longer supported; use 'merges' instead"));
> "be set" -> "be set".
Noted. I see that the series is now in `next` [Thank you], so not worth
the churn of a patch, unless folks start noticing..
>
> I am not sure how this helps anybody, though.
It's the Catch 22 problem for deleted capabilities, which we rarely see
because we normally have backward compatibility.
>
> When pull.rebase is parsed, rebase.c::rebase_parse_value() is called
> from builtin/pull.c::parse_config_rebase() and would trigger an
> error, whether it comes from the pull.rebase or the branch.*.rebase
> configuration variable. An error() message already said that
> 'preserve' was removed and 'merges' would be a replacement when it
> happened.
>
> If the user has *not* reached this die() due to a configuration
> variable, then there is not much point giving this new message,
> either.
From my perspective, users should then be purging _all_ their `preserve`
configurations once they hit such errors. As the v2.34.0 change
propagates through the Git ecosystem, hopefully it'll be a sufficient
prompt for those who haven't realised that the option can be 'hidden' in
their configuration options.
Time will tell.
Thanks
PhilipThere was a problem hiding this comment. On the Git mailing list, Philip Oakley wrote (reply to this): small clarification,
On 11/06/2022 15:03, Philip Oakley wrote:
>> When pull.rebase is parsed, rebase.c::rebase_parse_value() is called
>> from builtin/pull.c::parse_config_rebase() and would trigger an
>> error, whether it comes from the pull.rebase or the branch.*.rebase
>> configuration variable. An error() message already said that
>> 'preserve' was removed and 'merges' would be a replacement when it
>> happened.
>>
>> If the user has *not* reached this die() due to a configuration
>> variable, then there is not much point giving this new message,
>> either.
> From my perspective, users should then
That is, when users hit any of the `preserve-merges` error message, ...
> be purging _all_ their `preserve`
> configurations once they hit such errors. As the v2.34.0 change
> propagates through the Git ecosystem, hopefully it'll be a sufficient
> prompt for those who haven't realised that the option can be 'hidden' in
> their configuration options.
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): Philip Oakley <[email protected]> writes:
> small clarification,
>
> On 11/06/2022 15:03, Philip Oakley wrote:
>>> When pull.rebase is parsed, rebase.c::rebase_parse_value() is called
>>> from builtin/pull.c::parse_config_rebase() and would trigger an
>>> error, whether it comes from the pull.rebase or the branch.*.rebase
>>> configuration variable. An error() message already said that
>>> 'preserve' was removed and 'merges' would be a replacement when it
>>> happened.
>>>
>>> If the user has *not* reached this die() due to a configuration
>>> variable, then there is not much point giving this new message,
>>> either.
>> From my perspective, users should then
>
> That is, when users hit any of the `preserve-merges` error message, ...
Yes, but configuration parsing happens way earlier than the actual
use of the option (which is decided after configuration and then
command line is read), so the users would probably have hit the
error message and corrected their configuration before they can even
see this error message, no?
I guess I am repeating myself, so there may be some case where a
stale variable can still be in the user's configuration file and the
user can hit this error message without seeing the other error
message about the stale configuration variable that I am not seeing?
>> be purging _all_ their `preserve`
>> configurations once they hit such errors. As the v2.34.0 change
>> propagates through the Git ecosystem, hopefully it'll be a sufficient
>> prompt for those who haven't realised that the option can be 'hidden' in
>> their configuration options. |
||
| parse_opt_interactive), | ||
| OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected, | ||
| N_("(REMOVED) was: try to recreate merges " | ||
| "instead of ignoring them"), | ||
| 1, PARSE_OPT_HIDDEN), | ||
| OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate), | ||
| OPT_CALLBACK_F(0, "empty", &options, "{drop,keep,ask}", | ||
|
|
@@ -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 (!(action == ACTION_ABORT) && is_directory(buf.buf)) { | ||
| 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.")); | ||
| } else { | ||
| strbuf_reset(&buf); | ||
| strbuf_addf(&buf, "%s/interactive", merge_dir()); | ||
|
|
@@ -1203,7 +1205,9 @@ 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\n" | ||
| "Note: Your `pull.rebase` configuration may also be set to 'preserve',\n" | ||
| "which is no longer supported; use 'merges' instead")); | ||
|
|
||
| if (action != ACTION_NONE && total_argc != 2) { | ||
| usage_with_options(builtin_rebase_usage, | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Philip Oakley wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, René Scharfe wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, René Scharfe wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Philip Oakley wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Philip Oakley wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Philip Oakley wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):