Die preserve ggg

From: Philip Oakley <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/rebase.c +0 -0

index 7ab50cda2ad..6ce7e98a6f1 100644

From: Philip Oakley <[email protected]>

Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.

Make the rebase --abort option available to allow users to remove traces of any preserve-merges rebase, even if they had upgraded during a rebase.

One trigger was an unexpectedly difficult to resolve conflict, as reported on the git-users group.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)

Tell the user the options to resolve the problem manually.

Signed-off-by Philip Oakley <[email protected]>

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

builtin/rebase.c +0 -0

index 6ce7e98a6f1..aada25a8870 100644
@@ -1182,8 +1182,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else if (is_directory(merge_dir())) {
 		strbuf_reset(&buf);

From: Philip Oakley <[email protected]>

The --preserve-merges option was removed by v2.35.0. However users may not be aware that it is also a Pull option, and it is still offered by major IDE vendors such as Visual Studio.

Extend the --preserve-merges die message to also direct users to the use of the preserve option in the pull config.

Signed-off-by Philip Oakley <[email protected]>

---
 builtin/rebase.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index aada25a8870..6fc0aaebbb8 100644

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.

Quoted 17 lines · Philip Oakley via GitGitGadget

From: Philip Oakley <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/rebase.c +0 -0

index 7ab50cda2ad..6ce7e98a6f1 100644

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 8 lines

builtin/rebase.c +1 -1

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

Quoted 30 lines · Philip Oakley via GitGitGadget

From: Philip Oakley <[email protected]>

Git will die if a "rebase --preserve-merges" is in progress. Users cannot --quit, --abort or --continue the rebase.

Make the rebase --abort option available to allow users to remove traces of any preserve-merges rebase, even if they had upgraded during a rebase.

One trigger was an unexpectedly difficult to resolve conflict, as reported on the git-users group.
(https://groups.google.com/g/git-for-windows/c/3jMWbBlXXHM)

Tell the user the options to resolve the problem manually.

Signed-off-by Philip Oakley <[email protected]>

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

builtin/rebase.c +0 -2

index 6ce7e98a6f1..aada25a8870 100644
@@ -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");

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1191

Quoted 7 lines

builtin/rebase.c +4 -0

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

Quoted 21 lines · Philip Oakley via GitGitGadget

From: Philip Oakley <[email protected]>

The --preserve-merges option was removed by v2.35.0. However users may not be aware that it is also a Pull option, and it is still offered by major IDE vendors such as Visual Studio.

Extend the --preserve-merges die message to also direct users to the use of the preserve option in the pull config.

Signed-off-by Philip Oakley <[email protected]>

---
 builtin/rebase.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index aada25a8870..6fc0aaebbb8 100644
@@ -1205,7 +1205,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			     builtin_rebase_usage, 0);
 

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1212

Quoted 7 lines

builtin/rebase.c +2 -1

 	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?

Quoted 6 lines · 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.

Quoted 4 lines

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.

Quoted 23 lines

From: Philip Oakley <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/rebase.c +1 -1

index 7ab50cda2ad..6ce7e98a6f1 100644
@@ -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 "

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 3 lines

builtin/rebase.c +0 -0

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

Quoted 6 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 6 lines

builtin/rebase.c +1 -1

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

Quoted 8 lines

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 5 lines

builtin/rebase.c +0 -2

 	} 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");

Quoted 8 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1191

Quoted 7 lines

builtin/rebase.c +4 -0

+		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 ;-)

Quoted 4 lines

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.

From: Philip Oakley <[email protected]>

The --preserve-merges option was removed by v2.35.0. However

users may not be aware that it is also a Pull option, and it is still offered by major IDE vendors such as Visual Studio.

Extend the --preserve-merges die message to also direct users to the use of the preserve option in the pull config.

Signed-off-by Philip Oakley <[email protected]>

---
 builtin/rebase.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index aada25a8870..6fc0aaebbb8 100644

Quoted 10 lines · René Scharfe

Quoted 6 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 6 lines

builtin/rebase.c +1 -1

 		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:

Quoted 8 lines

Quoted 6 lines · René Scharfe

Quoted 6 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 6 lines

builtin/rebase.c +1 -1

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

Quoted 3 lines · Philip Oakley via GitGitGadget

From: Philip Oakley <[email protected]>

The --preserve-merges option was removed by v2.35.0. However

Are you sure about that?

52f1e821 (pull: remove support for --rebase=preserve, 2021-09-07) that is in v2.34.0 and above dropped pull.rebase=preserve from the Documentation/config/pull.txt (and others). My local collection of various Git versions agrees with me. "git help config" from 2.34.0 does not list preserve as a valid choice, but 2.33.0 does.

Quoted 18 lines

users may not be aware that it is also a Pull option, and it is still offered by major IDE vendors such as Visual Studio.

Extend the --preserve-merges die message to also direct users to the use of the preserve option in the pull config.

Signed-off-by Philip Oakley <[email protected]>

---
  builtin/rebase.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index aada25a8870..6fc0aaebbb8 100644
@@ -1205,7 +1205,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
  			     builtin_rebase_usage, 0);
 

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1209

Quoted 4 lines

builtin/rebase.c +2 -1

  	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?

Am 26.05.22 um 11:40 schrieb Ævar Arnfjörð Bjarmason:

Quoted 23 lines · Philip Oakley via GitGitGadget

Quoted 22 lines

From: Philip Oakley <[email protected]>

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 <[email protected]>

---
  builtin/rebase.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

builtin/rebase.c +0 -1

index 7ab50cda2ad..6ce7e98a6f1 100644
@@ -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 "

Quoted 8 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 4 lines

builtin/rebase.c +1 -0

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

Quoted 8 lines

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é

Quoted 8 lines · René Scharfe

Quoted 6 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 6 lines

OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,

builtin/rebase.c +1 -1

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

So there's no point in changing this string, nor to have translators focus on it, it'll never be used.

Am 26.05.22 um 22:33 schrieb Junio C Hamano:

Quoted 17 lines

Quoted 9 lines · René Scharfe

Quoted 6 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 6 lines

OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected,

builtin/rebase.c +1 -1

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

Quoted 5 lines

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

Am 26.05.22 um 11:40 schrieb Ævar Arnfjörð Bjarmason:

Quoted 25 lines · Philip Oakley via GitGitGadget

Quoted 23 lines

From: Philip Oakley <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/rebase.c +1 -1

index 7ab50cda2ad..6ce7e98a6f1 100644
@@ -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 "

Quoted 8 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1116

Quoted 3 lines

builtin/rebase.c +0 -0

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

Quoted 3 lines

So there's no point in changing this string, nor to have translators focus on it, it'll never be used.

Quoted 5 lines

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?

Quoted 3 lines · Philip Oakley

Quoted 3 lines

Quoted 3 lines

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

Quoted 14 lines

Quoted 14 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1191

Quoted 13 lines

  	} else if (is_directory(merge_dir())) {
  		strbuf_reset(&buf);
  		strbuf_addf(&buf, "%s/rewritten", merge_dir());

builtin/rebase.c +4 -2

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

Quoted 2 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 4 lines · Junio C Hamano

Quoted 4 lines

Quoted 4 lines

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.

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Philip Oakley

Quoted 3 lines

Quoted 3 lines

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

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

From: Philip Oakley <[email protected]>

The --preserve-merges option was removed by v2.34.0. However users may not be aware that it is also a Pull configuration option, which is still offered by major IDE vendors such as Visual Studio.

Extend the --preserve-merges die message to also direct users to the possible use of the preserve option in the pull.rebase config. This is an additional 'belt and braces' information statement.

Signed-off-by Philip Oakley <[email protected]>

---
 builtin/rebase.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index 17cc776b4b1..5f8921551e1 100644

Quoted 22 lines · Philip Oakley via GitGitGadget

From: Philip Oakley <[email protected]>

The --preserve-merges option was removed by v2.34.0. However users may not be aware that it is also a Pull configuration option, which is still offered by major IDE vendors such as Visual Studio.

Extend the --preserve-merges die message to also direct users to the possible use of the preserve option in the pull.rebase config. This is an additional 'belt and braces' information statement.

Signed-off-by Philip Oakley <[email protected]>

---
 builtin/rebase.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/rebase.c +0 -0

index 17cc776b4b1..5f8921551e1 100644
@@ -1205,7 +1205,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			     builtin_rebase_usage, 0);
 

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1210

Quoted 5 lines

builtin/rebase.c +3 -1

 	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.

Quoted 10 lines

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,

Quoted 7 lines · Philip Oakley

Quoted 7 lines

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.

Quoted 4 lines

Quoted 3 lines

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

Quoted 5 lines

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.