Re: [RFC] usage_msg_opt() and _optf() must die

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 7, 2024, 05:01 UTC
Message-ID: <ZrL_xI0bNga-RqCv@tanuki>
In-Reply-To: <xmqqikwds8pz.fsf@gitster.g>

On Tue, Aug 06, 2024 at 01:21:44PM -0700, Junio C Hamano wrote:

Show 27 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> I am very much tempted to suggest us do this.
> >>
> >>  void NORETURN usage_msg_opt(const char *msg,
> >> -                  const char * const *usagestr,
> >> -                  const struct option *options)
> >> +                  const char * const *usagestr UNUSED,
> >> +                  const struct option *options UNUSED)
> >>  {
> >> -       die_message("%s\n", msg); /* The extra \n is intentional */
> >> -       usage_with_options(usagestr, options);
> >> +       die("%s", msg);
> >>  }
> >
> > As a minimal "fix" to eliminate the user-hostile behavior, I would be
> > very much in favor of this change.
> >
> > (Retiring `usage_msg_opt` altogether would be even better, but is much
> > more invasive.)
> 
> The above is following the usual "we make changes but be nice to in
> flight topics by keeping the API function still available, but the
> function now behaves better" pattern.  In other words, elimination
> of the API function is a breaking change and can go slower.  What
> needs more urgent to get to that goal would be to adjust the tests
> and documentation pages to the fallout from the above single liner.

I think it is fine to do the above as an intermediate step towards dropping `usage_msg_opt()` altogether during this cycle. But what I'd like to see is that we already convert all existing callers to stop calling the function such that we can rest assured that we really can drop the function once the Git v2.48 release cycle starts. Somewhat like we have handled the deprecation of `struct ref_store`-less functions in "refs.h".

Otherwise I fear that it's going to stay around indefinitely in a misleading way.

I'm less sold on swapping the "usage:" prefix out for "error:". I think that the "usage:" prefix actually gives a helpful signal to the user, namely that it was the user that passed unexpected arguments. This is in contrast to "error:", where the command went with what they gave it but ultimately ended up running into an error condition.

Patrick