From 42fa0cbfe02bfb5f3e11d6d04f0205e1650f2e39 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:12:33 -0400 Subject: [PATCH 1/8] credential: handle invalid arguments earlier The git-credential command only takes one argument: the operation to perform. If we don't have one, we complain immediately. But if we have one that we don't recognize, we don't notice until after we've read the credential from stdin. This is likely to confuse a user invoking "git credential -h", as the program will hang waiting for their input before showing anything. Let's detect this case early. Likewise, we never noticed when there are extra arguments beyond the one we're expecting. Let's catch this with the same conditional. Note that we don't need to handle "--help" similarly, because the git wrapper does this before even calling cmd_credential(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/credential.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/credential.c b/builtin/credential.c index 0412fa00f0..879acfbcda 100644 --- a/builtin/credential.c +++ b/builtin/credential.c @@ -10,9 +10,9 @@ int cmd_credential(int argc, const char **argv, const char *prefix) const char *op; struct credential c = CREDENTIAL_INIT; - op = argv[1]; - if (!op) + if (argc != 2 || !strcmp(argv[1], "-h")) usage(usage_msg); + op = argv[1]; if (credential_read(&c, stdin) < 0) die("unable to read credential from stdin"); From 619b6c1710f726866f62e34b01cc57c03b390299 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:13:43 -0400 Subject: [PATCH 2/8] upload-archive: handle "-h" option early Normally upload-archive forks off upload-archive--writer to do the real work, and relays any errors back over the sideband channel. This is a good thing when the command is properly invoked remotely via ssh or git-daemon. But it's confusing to curious users who try "git upload-archive -h". Let's catch this invocation early and give a real usage message, rather than spewing "-h does not appear to be a git repository" amidst packet-lines. The chance of a false positive due to a real client asking for the repo "-h" is quite small. Likewise, we'll catch "-h" in upload-archive--writer. People shouldn't be invoking it manually, but it doesn't hurt to give a sane message if they do. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/upload-archive.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c index cde06977b7..84532ae9a9 100644 --- a/builtin/upload-archive.c +++ b/builtin/upload-archive.c @@ -22,7 +22,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix) struct argv_array sent_argv = ARGV_ARRAY_INIT; const char *arg_cmd = "argument "; - if (argc != 2) + if (argc != 2 || !strcmp(argv[1], "-h")) usage(upload_archive_usage); if (!enter_repo(argv[1], 0)) @@ -76,6 +76,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) { struct child_process writer = { argv }; + if (argc == 2 && !strcmp(argv[1], "-h")) + usage(upload_archive_usage); + /* * Set up sideband subprocess. * From bb246590a1b648ac23f6a22d1d9e119129ba2f03 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:15:09 -0400 Subject: [PATCH 3/8] remote-{ext,fd}: print usage message on invalid arguments We just say "Expected two arguments" when we get a different number of arguments, but we can be slightly friendlier. People shouldn't generally be running remote helpers themselves, but curious users might say "git remote-ext -h". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/remote-ext.c | 5 ++++- builtin/remote-fd.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c index 11b48bfb41..bfb21ba7d2 100644 --- a/builtin/remote-ext.c +++ b/builtin/remote-ext.c @@ -3,6 +3,9 @@ #include "run-command.h" #include "pkt-line.h" +static const char usage_msg[] = + "git remote-ext "; + /* * URL syntax: * 'command [arg1 [arg2 [...]]]' Invoke command with given arguments. @@ -193,7 +196,7 @@ static int command_loop(const char *child) int cmd_remote_ext(int argc, const char **argv, const char *prefix) { if (argc != 3) - die("Expected two arguments"); + usage(usage_msg); return command_loop(argv[2]); } diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c index 08d7121b6d..91dfe07e06 100644 --- a/builtin/remote-fd.c +++ b/builtin/remote-fd.c @@ -1,6 +1,9 @@ #include "builtin.h" #include "transport.h" +static const char usage_msg[] = + "git remote-fd "; + /* * URL syntax: * 'fd::[/]' Read/write socket pair @@ -57,7 +60,7 @@ int cmd_remote_fd(int argc, const char **argv, const char *prefix) char *end; if (argc != 3) - die("Expected two arguments"); + usage(usage_msg); input_fd = (int)strtoul(argv[2], &end, 10); From f0994fa85dfcbd9854686d7b6de3b05b7952f41c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:16:50 -0400 Subject: [PATCH 4/8] submodule--helper: show usage for "-h" Normal users shouldn't ever call submodule--helper, but it doesn't hurt to give them a normal usage message if they try "-h". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 566a5b6a6f..a78b003c25 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1222,9 +1222,8 @@ static struct cmd_struct commands[] = { int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { int i; - if (argc < 2) - die(_("submodule--helper subcommand must be " - "called with a subcommand")); + if (argc < 2 || !strcmp(argv[1], "-h")) + usage("git submodule--helper "); for (i = 0; i < ARRAY_SIZE(commands); i++) { if (!strcmp(argv[1], commands[i].cmd)) { From 5a88f97cff0f269f8b2069b17c9da05ee66c98bb Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 1 Jun 2017 13:38:16 +0900 Subject: [PATCH 5/8] diff- and log- family: handle "git cmd -h" early "git $builtin -h" bypasses the usual repository setup and calls the cmd_$builtin() function, expecting it to show the help text. Unfortunately the commands in the log- and the diff- family want to call into the revisions machinery, which by definition needs to have a repository already discovered. Strictly speaking, they may not need a repository only for parsing "-h", but it is a good discipline to future-proof codepath to ensure that setup_revisions() is called after we know that a repository is there. Handle the "git $builtin -h" special case very early in these commands to work around potential issues. Signed-off-by: Junio C Hamano --- builtin/diff-files.c | 3 +++ builtin/diff-index.c | 3 +++ builtin/diff-tree.c | 3 +++ builtin/rev-list.c | 3 +++ 4 files changed, 12 insertions(+) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 15c61fd8d1..6be1df684a 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -20,6 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) int result; unsigned options = 0; + if (argc == 2 && !strcmp(argv[1], "-h")) + usage(diff_files_usage); + init_revisions(&rev, prefix); gitmodules_config(); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 1af373d002..02dd35ba45 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -17,6 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + if (argc == 2 && !strcmp(argv[1], "-h")) + usage(diff_cache_usage); + init_revisions(&rev, prefix); gitmodules_config(); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 326f88b657..773cc254b5 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -105,6 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) struct setup_revision_opt s_r_opt; int read_stdin = 0; + if (argc == 2 && !strcmp(argv[1], "-h")) + usage(diff_tree_usage); + init_revisions(opt, prefix); gitmodules_config(); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ diff --git a/builtin/rev-list.c b/builtin/rev-list.c index bcf77f0b8a..2951e0efd7 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -277,6 +277,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) int use_bitmap_index = 0; const char *show_progress = NULL; + if (argc == 2 && !strcmp(argv[1], "-h")) + usage(rev_list_usage); + git_config(git_default_config, NULL); init_revisions(&revs, prefix); revs.abbrev = DEFAULT_ABBREV; From b48cbfc5e6112952bc3be4dea0208bc5e1f331eb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:17:42 -0400 Subject: [PATCH 6/8] version: convert to parse-options The "git version" command didn't traditionally accept any options, and in fact ignores any you give it. When we added simple option parsing for "--build-options" in 6b9c38e14, we didn't improve this; we just loop over the arguments and pick out the one we recognize. Instead, let's move to a real parsing loop, complain about nonsense options, and recognize conventions like "-h". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- help.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/help.c b/help.c index bc6cd19cf3..1064363cd5 100644 --- a/help.c +++ b/help.c @@ -8,6 +8,7 @@ #include "column.h" #include "version.h" #include "refs.h" +#include "parse-options.h" void add_cmdname(struct cmdnames *cmds, const char *name, int len) { @@ -424,16 +425,30 @@ const char *help_unknown_cmd(const char *cmd) int cmd_version(int argc, const char **argv, const char *prefix) { + int build_options = 0; + const char * const usage[] = { + N_("git version []"), + NULL + }; + struct option options[] = { + OPT_BOOL(0, "build-options", &build_options, + "also print build options"), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, usage, 0); + /* * The format of this string should be kept stable for compatibility * with external projects that rely on the output of "git version". + * + * Always show the version, even if other options are given. */ printf("git version %s\n", git_version_string); - while (*++argv) { - if (!strcmp(*argv, "--build-options")) { - printf("sizeof-long: %d\n", (int)sizeof(long)); - /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ - } + + if (build_options) { + printf("sizeof-long: %d\n", (int)sizeof(long)); + /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ } return 0; } From 8893fd95b66cbd6566136a289dd05fcf4e547281 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:18:43 -0400 Subject: [PATCH 7/8] git: add hidden --list-builtins option It can be useful in the test suite to be able to iterate over the list of builtins. We could do this with some Makefile magic. But since the authoritative list is in the commands array inside git.c, and since this could also be handy for debugging, let's add a hidden command-line option to dump that list. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/git.c b/git.c index 8ff44f081d..1b8b7f51a6 100644 --- a/git.c +++ b/git.c @@ -26,6 +26,8 @@ static const char *env_names[] = { static char *orig_env[4]; static int save_restore_env_balance; +static void list_builtins(void); + static void save_env_before_alias(void) { int i; @@ -232,6 +234,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) } (*argv)++; (*argc)--; + } else if (!strcmp(cmd, "--list-builtins")) { + list_builtins(); + exit(0); } else { fprintf(stderr, "Unknown option: %s\n", cmd); usage(git_usage_string); @@ -529,6 +534,13 @@ int is_builtin(const char *s) return !!get_builtin(s); } +static void list_builtins(void) +{ + int i; + for (i = 0; i < ARRAY_SIZE(commands); i++) + printf("%s\n", commands[i].cmd); +} + #ifdef STRIP_EXTENSION static void strip_extension(const char **argv) { From d691551192ac845747694258ccae9ffeeb6bdd58 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 30 May 2017 01:19:30 -0400 Subject: [PATCH 8/8] t0012: test "-h" with builtins Since commit 99caeed05 (Let 'git -h' show usage without a git dir, 2009-11-09), the git wrapper handles "-h" specially, skipping any repository setup but still calling the builtin's cmd_foo() function. This means that every cmd_foo() must be ready to handle this case, but we don't have any systematic tests. This led to "git am -h" being broken for some time without anybody noticing. This patch just tests that "git foo -h" works for every builtin, where we see a 129 exit code (the normal code for our usage() helper), and that the word "usage" appears in the output. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t0012-help.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 8faba2e8bc..487b92a5de 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" " test_i18ncmp expect actual " +test_expect_success 'generate builtin list' ' + git --list-builtins >builtins +' + +while read builtin +do + test_expect_success "$builtin can handle -h" ' + test_expect_code 129 git $builtin -h >output 2>&1 && + test_i18ngrep usage output + ' +done