| Message ID | 20250116012524.1557441-6-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Send help text from "git cmd -h" to stdout | expand |
On Wed, Jan 15, 2025 at 05:25:22PM -0800, Junio C Hamano wrote: > Using the show_usage_and_exit_if_asked() helper we introduced > earlier, fix callers of usage() that want to show the help text when > explicitly asked by the end-user. The help text now goes to the > standard output stream for them. > > The callers in this step are oddballs in that their invocations of > usage() are *not* guarded by > > if (argc == 2 && !strcmp(argv[1], "-h") > usage(...); > > They are (unnecessarily) being clever and do things like > > if (argc != 2 || !strcmp(argv[1], "-h") > usage(...); > > to say "I know I take only one argument, so argc != 2 is always an > error regardless of what is in argv[]. Ah, by the way, even if argc > is 2, "-h" is a request for usage text, so we do the same". Some > just do not treat "-h" any specially, and let it take the same error > code paths as a parameter error. As the author of at least one of these, I feel judged. :) But yes, untangling this is obviously good (especially if we change the exit code later!). And pulling these into their own commit made reviewing much easier. > diff --git a/builtin/var.c b/builtin/var.c > index 1449656cc9..6a09c1c39a 100644 > --- a/builtin/var.c > +++ b/builtin/var.c > @@ -221,6 +221,7 @@ int cmd_var(int argc, > const struct git_var *git_var; > char *val; > > + show_usage_and_exit_if_asked(argc, argv, var_usage); > if (argc != 2) > usage(var_usage); Hmm, what's going on in this one? It does not check "-h" at all. Ah, I see. It simply hits the get_git_var() call, sees there is no var called "-h", and exits with an error. So checking for "-h" up front is correct. It is an oddball, though not quite the same as the others (the oddest ball?). -Peff
Jeff King <peff@peff.net> writes: >> .... Some >> just do not treat "-h" any specially, and let it take the same error >> code paths as a parameter error. > ... >> + show_usage_and_exit_if_asked(argc, argv, var_usage); >> if (argc != 2) >> usage(var_usage); > > Hmm, what's going on in this one? It does not check "-h" at all. Yes, this is the one that let the usual error code paths to treat it as a mere parameter error.
diff --git a/builtin/credential.c b/builtin/credential.c index 14c8c6608b..8a8cffecc8 100644 --- a/builtin/credential.c +++ b/builtin/credential.c @@ -18,7 +18,8 @@ int cmd_credential(int argc, git_config(git_default_config, NULL); - if (argc != 2 || !strcmp(argv[1], "-h")) + show_usage_and_exit_if_asked(argc, argv, usage_msg); + if (argc != 2) usage(usage_msg); op = argv[1]; diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c index 6da2825753..307351af55 100644 --- a/builtin/unpack-file.c +++ b/builtin/unpack-file.c @@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid) return path; } +static const char usage_msg[] = +"git unpack-file <blob>"; + int cmd_unpack_file(int argc, const char **argv, const char *prefix UNUSED, @@ -33,8 +36,9 @@ int cmd_unpack_file(int argc, { struct object_id oid; - if (argc != 2 || !strcmp(argv[1], "-h")) - usage("git unpack-file <blob>"); + show_usage_and_exit_if_asked(argc, argv, usage_msg); + if (argc != 2) + usage(usage_msg); if (repo_get_oid(the_repository, argv[1], &oid)) die("Not a valid object name %s", argv[1]); diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c index 9e9343f121..3b282d41e6 100644 --- a/builtin/upload-archive.c +++ b/builtin/upload-archive.c @@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc, const char *arg_cmd = "argument "; int ret; - if (argc != 2 || !strcmp(argv[1], "-h")) + show_usage_and_exit_if_asked(argc, argv, upload_archive_usage); + if (argc != 2) usage(upload_archive_usage); if (!enter_repo(argv[1], 0)) diff --git a/builtin/var.c b/builtin/var.c index 1449656cc9..6a09c1c39a 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -221,6 +221,7 @@ int cmd_var(int argc, const struct git_var *git_var; char *val; + show_usage_and_exit_if_asked(argc, argv, var_usage); if (argc != 2) usage(var_usage);
Using the show_usage_and_exit_if_asked() helper we introduced earlier, fix callers of usage() that want to show the help text when explicitly asked by the end-user. The help text now goes to the standard output stream for them. The callers in this step are oddballs in that their invocations of usage() are *not* guarded by if (argc == 2 && !strcmp(argv[1], "-h") usage(...); They are (unnecessarily) being clever and do things like if (argc != 2 || !strcmp(argv[1], "-h") usage(...); to say "I know I take only one argument, so argc != 2 is always an error regardless of what is in argv[]. Ah, by the way, even if argc is 2, "-h" is a request for usage text, so we do the same". Some just do not treat "-h" any specially, and let it take the same error code paths as a parameter error. Now we cannot do the same, so these callers are rewrittin to do the show_usage_and_exit_if_asked() first and then handle the usage error the way they used to. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/credential.c | 3 ++- builtin/unpack-file.c | 8 ++++++-- builtin/upload-archive.c | 3 ++- builtin/var.c | 1 + 4 files changed, 11 insertions(+), 4 deletions(-)