[PATCH 0/3] Die preserve ggg

[PATCH 0/3] Die preserve ggg

[Philip Oakley via GitGitGadget]

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.

Thus the main update is to allow the rebase --abort command, even when a
--preserve is in progress, to proceed. The --abort code was unchanged by the
removal of the preserve option, as the resetting and clean up of internal
state is common to the other rebase options.

The user facing fatal message now simply advises to abort, or downgrade to a
version that has preserve-merges to complete the rebase.

The final patch highlights that some IDEs still allow the setting of the
preserve-merges option as a pull config setup.

Philip Oakly

[1] GitLore ref pull.1155.git.1645526016.gitgitgadget@gmail.com
https://lore.kernel.org/git/pull.1155.git.1645526016.gitgitgadget@gmail.com/

Philip Oakley (3):
  rebase.c: state preserve-merges has been removed
  rebase: help users when dying with `preserve-merges`
  rebase: note `preserve` merges may be a pull config option

 builtin/rebase.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)


base-commit: c4f0e309ae745751d08727f24e8ff55e56355755
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1242%2FPhilipOakley%2Fdie_preserve_ggg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1242/PhilipOakley/die_preserve_ggg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1242
-- 
gitgitgadget

[PATCH 1/3] rebase.c: state preserve-merges has been removed

[Philip Oakley via GitGitGadget]

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

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

[Philip Oakley via GitGitGadget]

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());
-- 
gitgitgadget

[PATCH 3/3] rebase: note `preserve` merges may be a pull config option

[Philip Oakley via GitGitGadget]

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

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

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

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.

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

[Ævar Arnfjörð Bjarmason]

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.

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

Re: [PATCH 3/3] rebase: note `preserve` merges may be a pull config option

[Ævar Arnfjörð Bjarmason]

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?

Re: [PATCH 0/3] Die preserve ggg

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Philip Oakley]

On 26/05/2022 10:40, Ævar Arnfjörð Bjarmason wrote:
> 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.

I still think it's useful for those that read the code, as it reminds
folks about what it is/was, and why it's hidden, hence the usefulness of
the change, from my perspective. I'm flexible either way, but didn't
feel the 'DEPRECATED' was correct information.
>
> 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.

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

[Philip Oakley]

On 26/05/2022 10:43, Ævar Arnfjörð Bjarmason wrote:
> 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?
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 ?

Re: [PATCH 3/3] rebase: note `preserve` merges may be a pull config option

[Philip Oakley]

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

Re: [PATCH 0/3] Die preserve ggg

[Philip Oakley]

On 26/05/2022 10:54, Ævar Arnfjörð Bjarmason wrote:
> 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.
Thanks, I am a bit busy with family issues, so I'd rather use as-is,
unless other feel that the tweaks will be worth it.
I'm effectively off-line for the next 5-6 days anyway.
--
Philip

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[René Scharfe]

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é

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[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),
>
> 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.

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

I am afraid to ask, after having learned to be worried about those
large rearchitecting projects Ævar talks about X-<.

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

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

[Junio C Hamano]

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

Re: [PATCH 3/3] rebase: note `preserve` merges may be a pull config option

[Junio C Hamano]

"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?

Thanks.

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[René Scharfe]

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 -- a removed
option would simply not be listed.  Here the feature is gone and its
option remains, but only reports an educational message now.

Perhaps a better option help text would be something like "no longer
supported, consider using --rebase-merges instead"?

René

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> 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"?

Yeah, that would read very well.

Thanks.

Re: [PATCH 3/3] rebase: note `preserve` merges may be a pull config option

[Philip Oakley]

On 26/05/2022 21:55, Junio C Hamano wrote:
> "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?
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 <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?
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.

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Philip Oakley]

Hi René,

On 26/05/2022 14:02, René Scharfe wrote:
> 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".
I should probably also add the verb "was" to indicate its historic use ;-)
>> 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é
P.

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Philip Oakley]

On 26/05/2022 21:33, Junio C Hamano wrote:
> 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.

I'll add "was .." to clarify its historic use.
I expect that it'll be there for many years to catch late upgrading 
users, as well as those that help others stuck in this trap using a 
modern portable Git (esp. Windows).
>>> 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.
P.

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Ævar Arnfjörð Bjarmason]

On Thu, May 26 2022, René Scharfe wrote:

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

You're right. I'd missed that case with --help-all, and remembered
briefly testing where we used the string before in some WIP code.

Looking at it again I think I tried NULL-ing a few and running the tests
with SANITIZE=address or something, which in this case it looks like
we'd 100% pass with. I.e. we have zero test coverage for that subsequent
NULL dereference.

Sorry about the noise. I'll add some tests for this case in some
parse-options.c sanity checking tests/patches I'm planning to submit at
some point.

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Philip Oakley]

Hi René

On 26/05/2022 22:27, René Scharfe wrote:
> 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

That's a misunderstanding between the response to the command line 
option, and the described operation of the former sub-command/option.
> -- a removed
> option would simply not be listed.  Here the feature is gone and its
> option remains, but only reports an educational message now.

The needed user response is more that educational. In this case (for the 
Series) they are in a Catch-22 situation, stuck in a no-man's land 
between a preserve merges that has been started, and a Git that won't 
proceed. Currently (prior to the series) Git will even refuse to abort..
>
> Perhaps a better option help text would be something like "no longer
> supported, consider using --rebase-merges instead"?
We'll still need to say _what_ is no longer supported, to ensure the 
user has context. I'd agree with the suggestion aspect (Junio had 
commented similarly).

I suspect this problem could be a long, slow burner. We so rarely remove 
capabilities like this, so it's tricky second guessing how users will 
react, or when they discover the problem.

P.

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

[Philip Oakley]

On 26/05/2022 21:42, Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.email> writes:
>
>>>> Make the `rebase --abort` option available to allow users to remove
>>>> traces of any preserve-merges rebase, even if they had upgraded
>>>> during a rebase.
> This patch does not make it "available", though.

Yes it does. Sorry if the terminology or explanation was poor (here we 
are looking at the commit message, not the user facing message?).

Currently, if the user has an in-progress rebase with preserve-merges, 
and now using the latest Git, they will reach the fatal die(), even if 
they try any of the git status suggestions of --abort, --continue, etc.  
Essentially, it's a 'you shouldn't be here', lets stop right now, go 
straight to jail condition. We do want to permit the `rebase --abort` 
command option.

I can swap around the && condition so that it's clearer that we check 
the user isn't requesting an --abort before checking the internal 
directory and then dying.
> 	Suggest using `--abort` to get out of the situation after a
> 	failed preserve-rebase and remove traces of ...
>
> perhaps?
>
> I do think the suggestion is worth doing if a user ever gets into
> the situation, but how likely does it happen?  A user has to start
> "rebase -p" with older Git,

.. hit a conflict, seeks help. Helper bring a personal portable Git with 
latest version - Oops.

Or Helper, says "Oh, your version is old, upgrade, and that'll fix it", 
again Oops.

> wait until Git gets updated to a future
> version of Git that includes this change, and then say "rebase -p
> --continue"?
You don't need the -p there ;-)

For this change, the "git rebase --continue" will still die() with the 
fatal: message. We do not have a way to continue. However..

After this change, the "git rebase --abort" will properly clear and 
clean the repo/status so that the user can then choose what to do.

>
>>>>   	} else if (is_directory(merge_dir())) {
>>>>   		strbuf_reset(&buf);
>>>>   		strbuf_addf(&buf, "%s/rewritten", merge_dir());
>>>> -		if (is_directory(buf.buf)) {
>>>> -			die("`rebase -p` is no longer supported");
>>>> +		if (is_directory(buf.buf) && !(action == ACTION_ABORT)) {
>>>> +			die("`rebase --preserve-merges` (-p) is no longer supported.\n"
>>>> +			"Use `git rebase --abort` to terminate current rebase.\n"
>>>> +			"Or downgrade to v2.33, or earlier, to complete the rebase.\n");
>>>>   		} else {
>>>>   			strbuf_reset(&buf);
>>>>   			strbuf_addf(&buf, "%s/interactive", merge_dir());
>>> Existing issue: No _(), shouldn't we add it?
>> This `strbuf_addf` is forming a path for internal use. It just happens
>> to look like legible English ;-)
> I do not think Ævar meant "%s/interactive"; the enhanced message
> above that you inherited from the original "no longer supported"
> that was not marked for translation.
Ok.
>
>>> I wonder if we should use die_message() + advise() in these cases,
>>> i.e. stick to why we died in die_message() and have the advise() make
>>> suggestions, as e4921d877ab (tracking branches: add advice to ambiguous
>>> refspec error, 2022-04-01) does.
>> Ah, maybe it's my message.. that needs translating.
> Yup.
Ok, I'd add a separate patch for that.

> This whole '-p' business will go away in a few releases down, so a
> longer message give to the existing die() should be sufficient and
> there is no need for the choice between "yes, I am still weaning
> myself off of rebase -p and want to keep seeing the advice" and
> "thanks, I saw the message often enough, you no longer need to tell
> me how to get out", I would think.
I think it will take a long while for all the users, tools providers and 
distros to get beyond 2.33, so while each user may be weaned quickly, 
the generic problem is likely to continue to linger.


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

Philip

Re: [PATCH 1/3] rebase.c: state preserve-merges has been removed

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

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

I think you are responding to what Ævar said, but the string this
patch is modifying is already inside N_(), and modifying a string
that is already marked for translation in any way (other than
removing the N_() or _() around it) incurs the cost to translate the
updated string already, with or without any separate patch.

If we are adding a new die() call that uses a new message, we should
mark the message for translation from the beginning.  The messages
produced by die/warning/error are meant to be read by human users,
so unless there is some very strong reason not to, they should be
marked for translation.

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

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

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

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

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

Thanks.

[PATCH v2 0/4] Die preserve ggg

[Philip Oakley via GitGitGadget]

This [2] short series is a follow up to GitGitGadget "Update the die()
preserve-merges messages to help some users (PR #1155)" [1].

Since v1: Additional patch to translate the user facing die message. Bring
the --abort preclusion to the start of the if && condition for clarity.
Clarify that the pull.rebase config is complementary to this particular
'die'. Updates to the commit messages.

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

Thus the main update is to allow the rebase --abort command, even when a
--preserve is in progress, to proceed. The --abort code was unchanged by the
removal of the preserve option, as the resetting and clean up of internal
state is common to the other rebase options.

The user facing fatal message now simply advises to abort, or downgrade to a
version that has preserve-merges to complete the rebase.

The final patch highlights that some IDEs still allow the setting of the
preserve-merges option as a pull config setup.

Philip Oakly

[1] GitLore ref pull.1155.git.1645526016.gitgitgadget@gmail.com
https://lore.kernel.org/git/pull.1155.git.1645526016.gitgitgadget@gmail.com/

[2]
https://lore.kernel.org/git/pull.1242.git.1653556865.gitgitgadget@gmail.com/t/#u

Philip Oakley (4):
  rebase.c: state preserve-merges has been removed
  rebase: help users when dying with `preserve-merges`
  rebase: note `preserve` merges may be a pull config option
  rebase: translate a die(preserve-merges) message

 builtin/rebase.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)


base-commit: c4f0e309ae745751d08727f24e8ff55e56355755
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1242%2FPhilipOakley%2Fdie_preserve_ggg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1242/PhilipOakley/die_preserve_ggg-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1242

Range-diff vs v1:

 1:  0a4c81d8caf ! 1:  d60ec67cb06 rebase.c: state preserve-merges has been removed
     @@ Commit message
          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.
     +    Fix and reflow the option tip.
      
          Signed-off-by: Philip Oakley <philipoakley@iee.email>
      
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       			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"),
     +-				 "ignoring them"),
     ++			      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}",
 2:  d0fb5410594 ! 2:  47f27187529 rebase: help users when dying with `preserve-merges`
     @@ Metadata
       ## Commit message ##
          rebase: help users when dying with `preserve-merges`
      
     -    Git will die if a "rebase --preserve-merges" is in progress.
     -    Users cannot --quit, --abort or --continue the rebase.
     +    Git would die if a "rebase --preserve-merges" was in progress.
     +    Users could neither --quit, --abort, nor --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
     +    One trigger case 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.
     +    Other potential use-cases include git-experts using the portable
     +    'Git on a stick' to help users with an older git version.
      
          Signed-off-by: Philip Oakley <philipoakley@iee.email>
      
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       		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)) {
     ++		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.\n");
     ++			"Or downgrade to v2.33, or earlier, to complete the rebase.");
       		} else {
       			strbuf_reset(&buf);
       			strbuf_addf(&buf, "%s/interactive", merge_dir());
 3:  ece3eecdc4d ! 3:  fe000f06207 rebase: note `preserve` merges may be a pull config option
     @@ Metadata
       ## Commit message ##
          rebase: note `preserve` merges may be a pull config option
      
     -    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.
     +    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 use of the `preserve` option in the `pull` config.
     +    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: int cmd_rebase(int argc, const char **argv, const char *prefix
       	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."));
     ++			"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,
 -:  ----------- > 4:  ae02c6d5a6e rebase: translate a die(preserve-merges) message

-- 
gitgitgadget

[PATCH v2 1/4] rebase.c: state preserve-merges has been removed

[Philip Oakley via GitGitGadget]

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 and reflow the option tip.

Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7ab50cda2ad..bad95d98adf 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1110,8 +1110,8 @@ 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 "
-				 "ignoring them"),
+			      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}",
-- 
gitgitgadget

[PATCH v2 2/4] rebase: help users when dying with `preserve-merges`

[Philip Oakley via GitGitGadget]

From: Philip Oakley <philipoakley@iee.email>

Git would die if a "rebase --preserve-merges" was in progress.
Users could neither --quit, --abort, nor --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 case was an unexpectedly difficult to resolve conflict, as
reported on the `git-users` group.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)

Other potential use-cases include git-experts using the portable
'Git on a stick' to help users with an older git version.

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 bad95d98adf..17cc776b4b1 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 (!(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());
-- 
gitgitgadget

[PATCH v2 3/4] rebase: note `preserve` merges may be a pull config option

[Philip Oakley via GitGitGadget]

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"));
 
 	if (action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
-- 
gitgitgadget

[PATCH v2 4/4] rebase: translate a die(preserve-merges) message

[Philip Oakley via GitGitGadget]

From: Philip Oakley <philipoakley@iee.email>

This is a user facing message for a situation seen in the wild.

Translate it.

Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5f8921551e1..640b6046a5a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1183,9 +1183,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		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"
+			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.");
+			"Or downgrade to v2.33, or earlier, to complete the rebase."));
 		} else {
 			strbuf_reset(&buf);
 			strbuf_addf(&buf, "%s/interactive", merge_dir());
-- 
gitgitgadget

Re: [PATCH v2 3/4] rebase: note `preserve` merges may be a pull config option

[Junio C Hamano]

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

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.

Re: [PATCH v2 3/4] rebase: note `preserve` merges may be a pull config option

[Philip Oakley]

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

Philip

Re: [PATCH v2 3/4] rebase: note `preserve` merges may be a pull config option

[Philip Oakley]

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.

Re: [PATCH v2 3/4] rebase: note `preserve` merges may be a pull config option

[Junio C Hamano]

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