| Message ID | xmqqmslptw3u.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usage_msg_opt() and _optf() must die | expand |
On Tue, Aug 6, 2024 at 1:11 PM Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Patrick Steinhardt <ps@pks.im> writes: > >> I always have to wonder how helpful it really is to print the usage > >> information in such a context. I feel that it is too distracting because > >> in many cases, we end up printing dozens of lines of options that drown > >> out the single line of information that the user actually cares for, > >> namely why the command has failed. Thank you, Patrick, for voicing this concern; I nearly did so myself upon reading this series. I always find it very user-hostile when the program dumps the entire usage text, thus forcing the user to sift through a bunch of noise, when it should instead just be printing a simple explanation of the problem to help the user correct the invocation. (It's even more frustrating when a program dumps the usage text but doesn't even bother explaining the problem[*], as if the user will somehow be able to intuit what went wrong.) [*]: https://lore.kernel.org/git/CAPig+cSK+wLPUDuGf1d41J_F5jQS+J=a=7kHQLV07-ZKZW9GsA@mail.gmail.com/ > Yes. I do not think I found it useful to give the single-line > message, blank line, followed by the full usage text even a single > time myself. > > 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.)
On 24/08/06 10:11AM, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Patrick Steinhardt <ps@pks.im> writes: > > > >> I always have to wonder how helpful it really is to print the usage > >> information in such a context. I feel that it is too distracting because > >> in many cases, we end up printing dozens of lines of options that drown > >> out the single line of information that the user actually cares for, > >> namely why the command has failed. > > Yes. I do not think I found it useful to give the single-line > message, blank line, followed by the full usage text even a single > time myself. I tend to also agree. The printed usage information is rather noisy and makes it more challenging to find the information that is actually relevant. I would be in favor of supressing the usage information altogether. If we did want to provide some sort of breadcrumb for users, maybe we could print a one-liner explaining how to fetch detailed usage information for a command if desired? Overall it would still be a lot less noisy. -Justin
On Tue, 6 Aug 2024 at 19:11, Junio C Hamano <gitster@pobox.com> wrote: > 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); > } Yes, I'm very much in favor of this. I know I've been greeted by a wall of usage text more than once and it does feel kind of intimidating. I just tested this using a silly git clone a b c d e f and the user experience is so much better after this patch. It's simply "fatal: Too many arguments.", which is kind of neat, all things considered. While one can always polish each error message individually, hiding it behind a big usage dump feels wrong. Martin
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.
On Tue, Aug 06, 2024 at 01:21:44PM -0700, Junio C Hamano wrote: > 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
diff --git c/parse-options.c w/parse-options.c index 30b9e68f8a..f27c425557 100644 --- c/parse-options.c +++ w/parse-options.c @@ -1277,11 +1277,10 @@ void NORETURN usage_with_options(const char * const *usagestr, } 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); } void NORETURN usage_msg_optf(const char * const fmt,