patch, 3 partsDie preserve ggg

> From: Philip Oakley <philipoakley@iee.email>
>
> Since feebd2d256 (rebase: hide --preserve-merges option, 2019-10-18)
> this option is now removed as stated in the subsequent release notes.
>
> Fix the option tip.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.email>
> ---
>  builtin/rebase.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 7ab50cda2ad..6ce7e98a6f1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1110,7 +1110,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
>  			parse_opt_interactive),
>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
> -			      N_("(DEPRECATED) try to recreate merges instead of "
> +			      N_("(REMOVED) try to recreate merges instead of "
>  				 "ignoring them"),
>  			      1, PARSE_OPT_HIDDEN),
>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),

> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> Since feebd2d256 (rebase: hide --preserve-merges option, 2019-10-18)
>> this option is now removed as stated in the subsequent release notes.
>>
>> Fix the option tip.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>> ---
>>  builtin/rebase.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 7ab50cda2ad..6ce7e98a6f1 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1110,7 +1110,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
>>  			parse_opt_interactive),
>>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>> +			      N_("(REMOVED) try to recreate merges instead of "
>>  				 "ignoring them"),
>>  			      1, PARSE_OPT_HIDDEN),
>>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
> I have some local patches for this more generally, but for
> PARSE_OPT_HIDDEN options we never do anything with the "argh" field,
> i.e. it's only used for showing the "git <cmd> -h" output, and if it's
> hidden it won't be there.
>
> So there's no point in changing this string, nor to have translators
> focus on it, it'll never be used.

>
> This series shouldn't fix the general issue (which parse-options.c
> should really be BUG()-ing about, after fixing the existing
> occurances. But For this one we could just set this to have a string of
> "" or something, only the string you're changing in 3/3 will be seen by
> anyone.

>
> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> Since feebd2d256 (rebase: hide --preserve-merges option, 2019-10-18)
>> this option is now removed as stated in the subsequent release notes.
>>
>> Fix the option tip.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>> ---
>>  builtin/rebase.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 7ab50cda2ad..6ce7e98a6f1 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1110,7 +1110,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
>>  			parse_opt_interactive),
>>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>> +			      N_("(REMOVED) try to recreate merges instead of "
>>  				 "ignoring them"),
>>  			      1, PARSE_OPT_HIDDEN),
>>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
>
> I have some local patches for this more generally, but for
> PARSE_OPT_HIDDEN options we never do anything with the "argh" field,
> i.e. it's only used for showing the "git <cmd> -h" output, and if it's
> hidden it won't be there.

> So there's no point in changing this string, nor to have translators
> focus on it, it'll never be used.
>
> This series shouldn't fix the general issue (which parse-options.c
> should really be BUG()-ing about, after fixing the existing
> occurances. But For this one we could just set this to have a string of
> "" or something, only the string you're changing in 3/3 will be seen by
> anyone.

>>>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>  				 "ignoring them"),
>>>  			      1, PARSE_OPT_HIDDEN),
>>>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
>
> Hidden options are shown if you use --help-all instead of -h.
>
> OPT_SET_INT_F always sets the struct option member "argh" to NULL.  The
> string changed above is the "help" member, not "argh".

>> So there's no point in changing this string, nor to have translators
>> focus on it, it'll never be used.
>>
>> This series shouldn't fix the general issue (which parse-options.c
>> should really be BUG()-ing about, after fixing the existing
>> occurances. But For this one we could just set this to have a string of
>> "" or something, only the string you're changing in 3/3 will be seen by
>> anyone.
>
> What is the general issue?

> René Scharfe <l.s.r@web.de> writes:
>
>>>>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>>  				 "ignoring them"),
>>>>  			      1, PARSE_OPT_HIDDEN),
>>>>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),

>> Anyway, the new help text explaining what the option once did is a bit
>> confusing.  It would be better to focus on what it's doing now (nothing)
>> and/or why we still have it (for backward compatibility), I think.
>
> Do you mean that we should say "this option used to do such and such
> but it is now a no-op" after "(REMOVED)" label, instead of the above
> "this option does such and such"?  I think "(REMOVED)" is a strong
> enough hint that lets us get away without saying "used to" and "but
> it is now a no-op", so I can accept both.
>
> Or do you mean we should say "(REMOVED) for backward compatibility,
> does nothing but errors out"?  I would be less in faviour, then.
> Those who are curious enough to ask --help-all would find it more
> helpful if we said what it used to do.  Otherwise they wouldn't be
> asking --help-all in the first place, no?

> Am 26.05.22 um 22:33 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>>>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>>>  				 "ignoring them"),
>>>>>  			      1, PARSE_OPT_HIDDEN),
>>>>>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
> ...
> Perhaps a better option help text would be something like "no longer
> supported, consider using --rebase-merges instead"?

> Am 26.05.22 um 22:33 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>>>   		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>>>   				 "ignoring them"),
>>>>>   			      1, PARSE_OPT_HIDDEN),
>>>>>   		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
>>> Anyway, the new help text explaining what the option once did is a bit
>>> confusing.  It would be better to focus on what it's doing now (nothing)
>>> and/or why we still have it (for backward compatibility), I think.
>> Do you mean that we should say "this option used to do such and such
>> but it is now a no-op" after "(REMOVED)" label, instead of the above
>> "this option does such and such"?  I think "(REMOVED)" is a strong
>> enough hint that lets us get away without saying "used to" and "but
>> it is now a no-op", so I can accept both.
>>
>> Or do you mean we should say "(REMOVED) for backward compatibility,
>> does nothing but errors out"?  I would be less in faviour, then.
>> Those who are curious enough to ask --help-all would find it more
>> helpful if we said what it used to do.  Otherwise they wouldn't be
>> asking --help-all in the first place, no?
> When I see an option labeled "REMOVED" then I get confused because a
> thing that says it no longer exists is obviously lying

> René Scharfe <l.s.r@web.de> writes:
>
>>>>   		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>>   				 "ignoring them"),
>>>>   			      1, PARSE_OPT_HIDDEN),
>>>>   		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
>> Hidden options are shown if you use --help-all instead of -h.
>>
>> OPT_SET_INT_F always sets the struct option member "argh" to NULL.  The
>> string changed above is the "help" member, not "argh".
> Good points.  I do think it is OK to say REMOVED in case --help-all
> asks us to show everything, even though I wonder if we can leave it
> there until we remove the "support" of noticing the user asking for
> a now-removed feature.

> On 26/05/2022 21:33, Junio C Hamano wrote:
>>>> So there's no point in changing this string, nor to have translators
>>>> focus on it, it'll never be used.
>>>>
>>>>
> The translation change would need to be a separate patch, no? That
> would make it easy to drop if not wanted.

> Am 26.05.22 um 11:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>>
>>> From: Philip Oakley <philipoakley@iee.email>
>>>
>>> Since feebd2d256 (rebase: hide --preserve-merges option, 2019-10-18)
>>> this option is now removed as stated in the subsequent release notes.
>>>
>>> Fix the option tip.
>>>
>>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>>> ---
>>>   builtin/rebase.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>>> index 7ab50cda2ad..6ce7e98a6f1 100644
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -1110,7 +1110,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>>   			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
>>>   			parse_opt_interactive),
>>>   		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>   				 "ignoring them"),
>>>   			      1, PARSE_OPT_HIDDEN),
>>>   		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
>> I have some local patches for this more generally, but for
>> PARSE_OPT_HIDDEN options we never do anything with the "argh" field,
>> i.e. it's only used for showing the "git <cmd> -h" output, and if it's
>> hidden it won't be there.
> Hidden options are shown if you use --help-all instead of -h.
>
> OPT_SET_INT_F always sets the struct option member "argh" to NULL.  The
> string changed above is the "help" member, not "argh".

>> So there's no point in changing this string, nor to have translators
>> focus on it, it'll never be used.
>>
>> This series shouldn't fix the general issue (which parse-options.c
>> should really be BUG()-ing about, after fixing the existing
>> occurances. But For this one we could just set this to have a string of
>> "" or something, only the string you're changing in 3/3 will be seen by
>> anyone.
> What is the general issue?
>
> Anyway, the new help text explaining what the option once did is a bit
> confusing.  It would be better to focus on what it's doing now (nothing)
> and/or why we still have it (for backward compatibility), I think.
>
> René

> Am 26.05.22 um 11:40 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>>
>>> From: Philip Oakley <philipoakley@iee.email>
>>>
>>> Since feebd2d256 (rebase: hide --preserve-merges option, 2019-10-18)
>>> this option is now removed as stated in the subsequent release notes.
>>>
>>> Fix the option tip.
>>>
>>> Signed-off-by: Philip Oakley <philipoakley@iee.email>
>>> ---
>>>  builtin/rebase.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>>> index 7ab50cda2ad..6ce7e98a6f1 100644
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -1110,7 +1110,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>>  			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
>>>  			parse_opt_interactive),
>>>  		OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,
>>> -			      N_("(DEPRECATED) try to recreate merges instead of "
>>> +			      N_("(REMOVED) try to recreate merges instead of "
>>>  				 "ignoring them"),
>>>  			      1, PARSE_OPT_HIDDEN),
>>>  		OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
>>
>> I have some local patches for this more generally, but for
>> PARSE_OPT_HIDDEN options we never do anything with the "argh" field,
>> i.e. it's only used for showing the "git <cmd> -h" output, and if it's
>> hidden it won't be there.
>
> Hidden options are shown if you use --help-all instead of -h.
>
> OPT_SET_INT_F always sets the struct option member "argh" to NULL.  The
> string changed above is the "help" member, not "argh".
>
>> So there's no point in changing this string, nor to have translators
>> focus on it, it'll never be used.
>>
>> This series shouldn't fix the general issue (which parse-options.c
>> should really be BUG()-ing about, after fixing the existing
>> occurances. But For this one we could just set this to have a string of
>> "" or something, only the string you're changing in 3/3 will be seen by
>> anyone.
>
> What is the general issue?

> From: Philip Oakley <philipoakley@iee.email>
>
> 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 <philipoakley@iee.email>
> ---
>  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());

> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> 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 <philipoakley@iee.email>
>> ---
>>  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.

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

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

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

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

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

> From: Philip Oakley <philipoakley@iee.email>
>
> 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 <philipoakley@iee.email>
> ---
>  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,

> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> 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 <philipoakley@iee.email>
>> ---
>>  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?
>

> 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 <philipoakley@iee.email>
> ---
>  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."));

> "Philip Oakley via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> 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 <philipoakley@iee.email>
>> ---
>>   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?

> This short series is a follow up to GitGitGadget "Update the die()
> preserve-merges messages to help some users (PR #1155)" [1].
>
> The first patch is a tidy up of the --preserve option to highlight that it
> is now Deleted, rather than Deprecated.
>
> In response to Avar's comments that the former error message merely
> 'tantilised without telling' the user what to do, it became obvious that the
> underling problem was that the user was unable to git rebase --abort which
> was also fatal, when a preserve-rebase was in progress.

> On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote:
>
>> This short series is a follow up to GitGitGadget "Update the die()
>> preserve-merges messages to help some users (PR #1155)" [1].
>>
>> The first patch is a tidy up of the --preserve option to highlight that it
>> is now Deleted, rather than Deprecated.
>>
>> In response to Avar's comments that the former error message merely
>> 'tantilised without telling' the user what to do, it became obvious that the
>> underling problem was that the user was unable to git rebase --abort which
>> was also fatal, when a preserve-rebase was in progress.
> Thanks a lot for following up on this, this all looks OK to me. I had
> some minor comments about maybe tweaking this & that, but as far as I'm
> concerned this could go in as-is, depending on whether you think it
> needs a re-roll in response to my comments + others.

> From: Philip Oakley <philipoakley@iee.email>
>
> 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 <philipoakley@iee.email>
> ---
>  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"));

> "Philip Oakley via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Philip Oakley <philipoakley@iee.email>
>>
>> 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 <philipoakley@iee.email>
>> ---
>>  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".

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

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

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