| Message ID | 0a4c81d8cafdc048fa89c24fcfa4e2715a17d176.1653556865.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Die preserve ggg | expand |
On Thu, May 26 2022, Philip Oakley via GitGitGadget wrote: > From: Philip Oakley <philipoakley@iee.email> > > Since feebd2d256 (rebase: hide --preserve-merges option, 2019-10-18) > this option is now removed as stated in the subsequent release notes. > > Fix the option tip. > > Signed-off-by: Philip Oakley <philipoakley@iee.email> > --- > builtin/rebase.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 7ab50cda2ad..6ce7e98a6f1 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1110,7 +1110,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > PARSE_OPT_NOARG | PARSE_OPT_NONEG, > parse_opt_interactive), > OPT_SET_INT_F('p', "preserve-merges", &preserve_merges_selected, > - N_("(DEPRECATED) try to recreate merges instead of " > + N_("(REMOVED) try to recreate merges instead of " > "ignoring them"), > 1, PARSE_OPT_HIDDEN), > OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate), I have some local patches for this more generally, but for PARSE_OPT_HIDDEN options we never do anything with the "argh" field, i.e. it's only used for showing the "git <cmd> -h" output, and if it's hidden it won't be there. So there's no point in changing this string, nor to have translators focus on it, it'll never be used. This series shouldn't fix the general issue (which parse-options.c should really be BUG()-ing about, after fixing the existing occurances. But For this one we could just set this to have a string of "" or something, only the string you're changing in 3/3 will be seen by anyone.
On 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.
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é
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?
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é
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.
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.
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.
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.
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.
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.
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),