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

From: Junio C Hamano <gitster@pobox.com>
Date: May 26, 2022, 20:33 UTC
Message-ID: <xmqq5ylsxccw.fsf@gitster.g>
In-Reply-To: <19baf95d-67d4-d7ed-72a6-96d098171d3a@web.de>

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

Show 11 quoted lines

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

Show 10 quoted 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?

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?