setup_git_env called without repository
Hi,
After upgrading to Git 2.13.0, I'm seeing the following error message when running git am -h.
$ git am -h fatal: BUG: setup_git_env called without repository
And with Git built from the next branch:
$ git am -h BUG: environment.c:172: setup_git_env called without repository Abort trap: 6
Signature · 2 lines
Best regards, Zero King
Quoted 11 lines · Zero King
After upgrading to Git 2.13.0, I'm seeing the following error message when running
git am -h.$ git am -h fatal: BUG: setup_git_env called without repositoryAnd with Git built from the next branch:
$ git am -h BUG: environment.c:172: setup_git_env called without repository Abort trap: 6
Jeff, bisects to your b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"", 2016-10-20).
Quoted 7 lines · Ævar Arnfjörð Bjarmason
Quoted 6 lines · Zero King
After upgrading to Git 2.13.0, I'm seeing the following error message when running
git am -h.$ git am -h fatal: BUG: setup_git_env called without repository
Quoted 8 lines
Quoted 5 lines
And with Git built from the next branch:
$ git am -h BUG: environment.c:172: setup_git_env called without repository Abort trap: 6Jeff, bisects to your
b1ef400eec("setup_git_env: avoid blind fall-back to ".git"", 2016-10-20).
Well, yes. But that commit is just making it easier to notice existing violations of the setup_git_env() rules. The interesting thing is where the violation comes from. :)
In this case, the "am" builtin uses git_pathdup() and git_config() to set up some defaults before calling parse_options(), which is where we handle "-h". Normally that's fine, because it uses the RUN_SETUP flag to tell the git wrapper to barf if we're not in a repository.
But when the wrapper sees that there is a single command-line argument and that it's "-h", it skips all of the setup and runs the builtin's cmd_am() function anyway, under the assumption that the builtin won't do anything meaningful before noticing the "-h" and dying. This goes back to Jonathan's 99caeed05 (Let 'git <command> -h' show usage without a git dir, 2009-11-09).
I have mixed feelings on that commit. It's unquestionably more friendly to make "git foo -h" work outside of a repository, rather than printing "Not a git repository". But it does break the assumptions of the cmd_* functions.
In this case it's fairly harmless. We'd fill in bogus values for the am_state (a bogus git-dir, but also we'd quietly ignore any repo-level config when we _are_ in a repo), but those aren't actually used before we hit the "-h" handling. Conceivably a cmd_* function could show defaults as part of "-h" output, but I don't know of any cases where that is true.
I'd be much more worried about cases where the cmd function doesn't handle "-h" at all, and just proceeds with a broken setup. That said, it's been this way for almost 8 years. And I see that several commands already have workarounds for dying early in this case (try grepping for '"-h"'). So probably we should just follow suit, like:
builtin/am.c +3 -0
@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_END() }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (usage, options); +git_config (git_am_config,NULL );am_state_init (&state);
I've used the same incantation there as in other commands; possibly this should be abstracted to a simple:
check_help_option(argc, argv, usage, options);
that each builtin could call.
What would be _really_ nice is if the usage/options pointers for each builtin were made available to git.c, and then it could just call usage_with_options() itself when it sees "-h". I think that would end up with more boilerplate than it saves, though, as we'd have to add the pointer to the definition of each builtin struct in git.c. Possibly it could be helped with some macro trickery and naming conventions, but I don't know if it's worth it.
The other thing to add is a test to make sure this doesn't pop up again in another builtin. We should be able to do:
for i in $builtins
do
test_expect_success "$i -h works" " test_expect_code 129 $i -h "
done
We'd need a list of builtins; probably a "git help --list-builtins" would be helpful there. I ran something similar by hand and "git am" is the only command that has this problem (for now, anyway). Interestingly, "git credential" doesn't parse the "-h" at all, and so it just hangs waiting for input (and would misbehave by ignoring repo config in this case!). It should be fixed, too.
I'll try to put together patches in the next day or so. Comments welcome in the meantime.
-Peff
cover consistent "-h" handling in builtins
Quoted 2 lines · Jeff King
I'll try to put together patches in the next day or so. Comments welcome in the meantime.
So here they are. For those just joining us, the immediate problem is that "git am -h" is broken (whether you're in a repo or not). That's fixed by the first patch, which can go to "maint" separately (although the rest are pretty unadventurous).
The rest of it cleans up "-h" handling for a variety of builtin commands, and then adds a test to make sure they all behave sanely (which unsurprisingly is how I found all the problems fixed by the earlier patches).
[1/8]: am: handle "-h" argument earlier
[2/8]: credential: handle invalid arguments earlier [3/8]: upload-archive: handle "-h" option early [4/8]: remote-{ext,fd}: print usage message on invalid arguments [5/8]: submodule--helper: show usage for "-h"
[6/8]: version: convert to parse-options
[7/8]: git: add hidden --list-builtins option
[8/8]: t0012: test "-h" with builtins
builtin/am.c | 3 +++ builtin/credential.c | 4 ++-- builtin/remote-ext.c | 5 ++++- builtin/remote-fd.c | 5 ++++- builtin/submodule--helper.c | 5 ++--- builtin/upload-archive.c | 5 ++++- git.c | 12 ++++++++++++ help.c | 25 ++++++++++++++++++++----- t/t0012-help.sh | 12 ++++++++++++ 9 files changed, 63 insertions(+), 13 deletions(-)
-Peff
PATCH 1/8 am: handle "-h" argument earlier
If the user provides "-h" on the command line, then our parse_options() invocation will show a usage message and quit. But if "-h" is the only argument, the git wrapper behaves specially: it ignores our RUN_SETUP flag and calls cmd_am() without having done repository setup at all. This is due to 99caeed05 (Let 'git <command> -h' show usage without a git dir, 2009-11-09).
Before cmd_am() calls parse_options(), though, it runs a few other setup functions. One of these is am_state_init(), which uses git_pathdup() to set up the default rebase-apply path. But calling git_pathdup() when we haven't done repository setup will fall back to using ".git". That's mostly harmless (since we won't use the value anyway), but is forbidden since b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"", 2016-10-20), and we now BUG().
We can't easily move that setup to after the parse_options() call; the point is to set up defaults that are overwritten by the option parsing. Instead, we'll detect the "-h" case early and show the usage then. This matches the behavior of other builtins which have a similar setup-ordering issue (e.g., git-branch).
--- builtin/am.c | 3 +++ 1 file changed, 3 insertions(+)
builtin/am.c +3 -0
@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_END() }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (usage, options); +git_config (git_am_config,NULL );am_state_init (&state);
Signature · 1 line
2.13.0.613.g11c956365
Re: [PATCH 1/8] am: handle "-h" argument earlier
Quoted 6 lines · Jeff King
We can't easily move that setup to after the
parse_options()call; the point is to set up defaults that are overwritten by the option parsing. Instead, we'll detect the "-h" case early and show the usage then. This matches the behavior of other builtins which have a similar setup-ordering issue (e.g., git-branch).
Thanks. And this structure of the series is very much appreciated.
Quoted 19 lines
Signed-off-by Jeff King <[email protected]>--- builtin/am.c | 3 +++ 1 file changed, 3 insertions(+)builtin/am.c +3
-0@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_END() }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (usage, options); +git_config (git_am_config,NULL );am_state_init (&state);
PATCH 2/8 credential: handle invalid arguments earlier
In reply to Jeff KingThe 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().
--- builtin/credential.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/credential.c +2 -2
@@ -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" );
Signature · 1 line
2.13.0.613.g11c956365
PATCH 3/8 upload-archive: handle "-h" option early
In reply to Jeff KingNormally 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.
--- builtin/upload-archive.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
builtin/upload-archive.c +4 -1
@@ -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. *
Signature · 1 line
2.13.0.613.g11c956365
PATCH 4/8 remote-{ext,fd}: print usage message on invalid arguments
In reply to Jeff KingWe 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".
---
According to remote-curl.c, we should actually handle the 1-argument case, too. I didn't dig into that because it's orthogonal to this series, and it's not clear that anybody cares.
builtin/remote-ext.c | 5 ++++- builtin/remote-fd.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-)
builtin/remote-ext.c +4 -1
@@ -3,6 +3,9 @@#include "run-command.h" #include "pkt-line.h" +static const char usage_msg[] = +"git remote-ext <remote> <url>" ; +/* * 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 ]); }
builtin/remote-fd.c +4 -1
@@ -1,6 +1,9 @@#include "builtin.h" #include "transport.h" +static const char usage_msg[] = +"git remote-fd <remote> <url>" ; +/* * URL syntax: * 'fd::<inoutfd>[/<anything>]' 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 );
Signature · 1 line
2.13.0.613.g11c956365
PATCH 5/8 submodule--helper: show usage for "-h"
In reply to Jeff KingNormal users shouldn't ever call submodule--helper, but it doesn't hurt to give them a normal usage message if they try "-h".
---
The usage message isn't that helpful _either_, and I admit my ulterior motive is just to make the test at the end of this series pass. :)
I was tempted to actually dump the names from the commands array to stdout, but this command really is an internal helper. It's probably not worth spending time on such niceties.
builtin/submodule--helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +2 -3
@@ -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 <command>" );for (i =0 ; i <ARRAY_SIZE (commands); i++) {if (!strcmp (argv[1 ], commands[i].cmd)) {
Signature · 1 line
2.13.0.613.g11c956365
PATCH 6/8 version: convert to parse-options
In reply to Jeff KingThe "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".
---
I assume nobody was running "git version --foobar" and expecting it to work. I guess we could also complain about "git version foobar" (no dashes), but this patch doesn't. Mainly I wanted the auto-generated options list.
help.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
help.c +20 -5
@@ -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 [<options>]" ), +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 ; }
Signature · 1 line
2.13.0.613.g11c956365
PATCH 1/1 version: move --build-options to a test helper
Move the undocumented --build-options argument to a test helper. It's purely used for testing git itself, so it belongs in a test helper instead of something that's part of the public plumbing.
---
Quoted 9 lines · Jeff King
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".
Quoted 6 lines
Signed-off-by Jeff King <[email protected]>---
I assume nobody was running "git version --foobar" and expecting it to work. I guess we could also complain about "git version foobar" (no dashes), but this patch doesn't. Mainly I wanted the auto-generated options list.
Looks good to me. I started hacking this up the other day, but then thought "wait a minute, isn't this just a test helper?" and wrote this which I've rebased on top of your change.
I may be missing something here but isn't this a much straightforward way to accomplish this, or is this used by some external program outside of git.git that's going to rely on --build-options output?
Makefile | 1 + help.c | 7 ------- t/helper/.gitignore | 1 + t/helper/test-long-is-64bit.c | 6 ++++++ t/test-lib.sh | 9 +-------- 5 files changed, 9 insertions(+), 15 deletions(-) create mode 100644 t/helper/test-long-is-64bit.c
Makefile +1 -0
@@ -623,6 +623,7 @@ TEST_PROGRAMS_NEED_X += test-hashmap TEST_PROGRAMS_NEED_X += test-index-version TEST_PROGRAMS_NEED_X += test-lazy-init-name-hash TEST_PROGRAMS_NEED_X += test-line-buffer +TEST_PROGRAMS_NEED_X += test-long-is-64bit TEST_PROGRAMS_NEED_X += test-match-trees TEST_PROGRAMS_NEED_X += test-mergesort TEST_PROGRAMS_NEED_X += test-mktemp
help.c +0 -7
@@ -384,14 +384,11 @@ 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 [<options>]" ),NULL };struct option options[] = { -OPT_BOOL (0 ,"build-options" , &build_options, -"also print build options" ),OPT_END () };
@@ -405,10 +402,6 @@ int cmd_version(int argc, const char **argv, const char *prefix)*/ printf ("git version %s\n" , git_version_string); -if (build_options) { -printf ("sizeof-long: %d\n" , (int )sizeof (long )); -/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ - }return 0 ; }
t/helper/.gitignore +1 -0
@@ -13,6 +13,7 @@ /test-index-version /test-lazy-init-name-hash /test-line-buffer +/test-long-is-64bit /test-match-trees /test-mergesort /test-mktemp
t/helper/test-long-is-64bit.c new file +6 -0
@@ -0,0 +1,6 @@ +#include "git-compat-util.h" + +int cmd_main (int argc,const char **argv) +{ +return (8 <= (int )sizeof (long )) ?0 :1 ; +}
t/test-lib.sh +1 -8
@@ -1165,14 +1165,7 @@ run_with_limited_cmdline () { test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true' -build_option () { - git version --build-options | - sed -ne "s/^$1: //p" -} - -test_lazy_prereq LONG_IS_64BIT ' - test 8 -le "$(build_option sizeof-long)" -' +test_lazy_prereq LONG_IS_64BIT 'test-long-is-64bit' test_lazy_prereq TIME_IS_64BIT 'test-date is64bit' test_lazy_prereq TIME_T_IS_64BIT 'test-date time_t-is64bit'
Signature · 1 line
2.13.0.303.g4ebf302169
Re: [PATCH 6.5?/8] version: move --build-options to a test helper
Quoted 17 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Jeff King
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".
Quoted 6 lines
Signed-off-by Jeff King <[email protected]>---
I assume nobody was running "git version --foobar" and expecting it to work. I guess we could also complain about "git version foobar" (no dashes), but this patch doesn't. Mainly I wanted the auto-generated options list.
Quoted 7 lines
Looks good to me. I started hacking this up the other day, but then thought "wait a minute, isn't this just a test helper?" and wrote this which I've rebased on top of your change.
I may be missing something here but isn't this a much straightforward way to accomplish this, or is this used by some external program outside of git.git that's going to rely on --build-options output?
My intent in putting it into the actual git binary was that it could also be useful for collecting build-time knobs from users (who may be using a binary package). Like:
http://public-inbox.org/git/[email protected]/
We haven't filled in that NEEDSWORK yet, but I'd rather see us go in that direction than remove the option entirely.
-Peff
Re: [PATCH 6.5?/8] version: move --build-options to a test helper
Hi,
Quoted 27 lines · Jeff King
Quoted 17 lines · Ævar Arnfjörð Bjarmason
Quoted 9 lines · Jeff King
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".
Quoted 6 lines
Signed-off-by Jeff King <[email protected]>---
I assume nobody was running "git version --foobar" and expecting it to work. I guess we could also complain about "git version foobar" (no dashes), but this patch doesn't. Mainly I wanted the auto-generated options list.Quoted 7 lines
Looks good to me. I started hacking this up the other day, but then thought "wait a minute, isn't this just a test helper?" and wrote this which I've rebased on top of your change.
I may be missing something here but isn't this a much straightforward way to accomplish this, or is this used by some external program outside of git.git that's going to rely on --build-options output?
Quoted 8 lines
My intent in putting it into the actual git binary was that it could also be useful for collecting build-time knobs from users (who may be using a binary package). Like:
http://public-inbox.org/git/[email protected]/
We haven't filled in that NEEDSWORK yet, but I'd rather see us go in that direction than remove the option entirely.
FWIW it also helped Git for Windows.
The two additional bits we added to the build options (the commit from which Git was built and the architecture) did not hurt one bit, either.
In other words, it would make my life a lot harder if --build-options were moved to a test helper that does not ship with the end product.
Ciao,
Dscho
Re: [PATCH 6.5?/8] version: move --build-options to a test helper
Quoted 9 lines · Johannes Schindelin
Quoted 8 lines
My intent in putting it into the actual git binary was that it could also be useful for collecting build-time knobs from users (who may be using a binary package). Like:
http://public-inbox.org/git/[email protected]/
We haven't filled in that NEEDSWORK yet, but I'd rather see us go in that direction than remove the option entirely.
Quoted 7 lines
FWIW it also helped Git for Windows.
The two additional bits we added to the build options (the commit from which Git was built and the architecture) did not hurt one bit, either.
In other words, it would make my life a lot harder if --build-options were moved to a test helper that does not ship with the end product.
Cool, I'm glad it has helped already. If you have further bits added to the output, is it worth sending that patch upstream?
-Peff
Re: [PATCH 6.5?/8] version: move --build-options to a test helper
Hi Peff,
Quoted 13 lines · Jeff King
Quoted 11 lines · Johannes Schindelin
Quoted 8 lines
My intent in putting it into the actual git binary was that it could also be useful for collecting build-time knobs from users (who may be using a binary package). Like:
http://public-inbox.org/git/[email protected]/
We haven't filled in that NEEDSWORK yet, but I'd rather see us go in that direction than remove the option entirely.
FWIW it also helped Git for Windows.
Quoted 8 lines
Quoted 5 lines
The two additional bits we added to the build options (the commit from which Git was built and the architecture) did not hurt one bit, either.
In other words, it would make my life a lot harder if --build-options were moved to a test helper that does not ship with the end product.
Cool, I'm glad it has helped already. If you have further bits added to the output, is it worth sending that patch upstream?
Yes, of course.
The day only has 24h though and I am still stuck with other things I try to contribute that seem to be requiring a lot more effort (mostly trying to make my case that there are certain coding paradigms I find too sloppy to put my name on) from my side to get accepted than I'd like.
So yeah, as soon as the queue drains a bit more, I have tons more patches ready to go upstream.
Ciao,
Dscho
Re: [PATCH 6.5?/8] version: move --build-options to a test helper
Quoted 28 lines · Johannes Schindelin
Hi Peff,
Quoted 13 lines · Jeff King
Quoted 11 lines · Johannes Schindelin
Quoted 8 lines
My intent in putting it into the actual git binary was that it could also be useful for collecting build-time knobs from users (who may be using a binary package). Like:
http://public-inbox.org/git/[email protected]/
We haven't filled in that NEEDSWORK yet, but I'd rather see us go in that direction than remove the option entirely.
FWIW it also helped Git for Windows.
Quoted 8 lines
Quoted 5 lines
The two additional bits we added to the build options (the commit from which Git was built and the architecture) did not hurt one bit, either.
In other words, it would make my life a lot harder if --build-options were moved to a test helper that does not ship with the end product.
Cool, I'm glad it has helped already. If you have further bits added to the output, is it worth sending that patch upstream?
Yes, of course.
Quoted 7 lines
The day only has 24h though and I am still stuck with other things I try to contribute that seem to be requiring a lot more effort (mostly trying to make my case that there are certain coding paradigms I find too sloppy to put my name on) from my side to get accepted than I'd like.
So yeah, as soon as the queue drains a bit more, I have tons more patches ready to go upstream.
Thanks both. It makes sense to discard this patch.
I wasn't sure whether anyone really cared about this, and thought a patch was a better starting point of discussion.
Re: [PATCH 6.5?/8] version: move --build-options to a test helper
Quoted 4 lines · Ævar Arnfjörð Bjarmason
Thanks both. It makes sense to discard this patch.
I wasn't sure whether anyone really cared about this, and thought a patch was a better starting point of discussion.
I will never complain about somebody starting a discussion with a patch as long as they don't mind if it gets shot down. :)
Thanks for raising the point. I do think it was worth thinking about.
-Peff
PATCH 7/8 git: add hidden --list-builtins option
In reply to Jeff KingIt 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.
---
The forward declaration here is necessary because of the function ordering. We could push handle_options() much lower in the file, but I didn't think it was worth the churn.
git.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
git.c +12 -0
@@ -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) {
Signature · 1 line
2.13.0.613.g11c956365
PATCH 8/8 t0012: test "-h" with builtins
In reply to Jeff KingSince commit 99caeed05 (Let 'git <command> -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.
--- t/t0012-help.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
t/t0012-help.sh +12 -0
@@ -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 <builtins + test_done
Signature · 1 line
2.13.0.613.g11c956365
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 20 lines · Jeff King
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 <[email protected]>--- t/t0012-help.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)t/t0012-help.sh +2
-0@@ -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
t/t0012-help.sh test_expect_success "--help does not work for guides" " · line 63
Quoted 10 lines
t/t0012-help.sh +10
-0+' + +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 <builtins +
These still seem to need further tweaks?
diff-files
diff-index
diff-tree
rev-list
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 15 lines · Junio C Hamano
t/t0012-help.sh test_expect_success "--help does not work for guides" " · line 63
Quoted 12 lines
t/t0012-help.sh +12
-0+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 <builtins +These still seem to need further tweaks?
Quoted 4 lines
diff-files
diff-index
diff-tree
rev-list
How so? They pass the test for me, and the output for a manual run looks sane.
-Peff
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 17 lines · Jeff King
Quoted 15 lines · Junio C Hamano
t/t0012-help.sh test_expect_success "--help does not work for guides" " · line 63
Quoted 12 lines
t/t0012-help.sh +12
-0+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 <builtins +These still seem to need further tweaks?
Quoted 7 lines
Quoted 4 lines
diff-files
diff-index
diff-tree
rev-listHow so? They pass the test for me, and the output for a manual run looks sane.
Am I missing some patches (I have these 8) outside the series, perhaps?
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 24 lines · Junio C Hamano
Quoted 22 lines · Jeff King
Quoted 19 lines · Junio C Hamano
t/t0012-help.sh test_expect_success "--help does not work for guides" " · line 63
Quoted 12 lines
t/t0012-help.sh +12
-0+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 <builtins +These still seem to need further tweaks?
diff-files
diff-index
diff-tree
rev-list
Quoted 5 lines
Quoted 2 lines
How so? They pass the test for me, and the output for a manual run looks sane.
Am I missing some patches (I have these 8) outside the series, perhaps?
Nope, I have those patches directly on your e83352ef23, and it passes. I wonder if there's something funny between our environments. What does the failure look like for you?
-Peff
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 3 lines · Jeff King
Nope, I have those patches directly on your
e83352ef23, and it passes. I wonder if there's something funny between our environments. What does the failure look like for you?
Travis seems to be seeing the same failure. Curiously, the topic by itself passes for me; iow, pu fails, pu^2 doesn't fail.
git.git/pu$ ./git rev-list -h
BUG: environment.c:173: setup_git_env called without repository Aborted (core dumped)
Hmph...
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Jeff King
Nope, I have those patches directly on your
e83352ef23, and it passes. I wonder if there's something funny between our environments. What does the failure look like for you?
Quoted 8 lines
Travis seems to be seeing the same failure. Curiously, the topic by itself passes for me; iow, pu fails, pu^2 doesn't fail.
git.git/pu$ ./git rev-list -h
BUG: environment.c:173:setup_git_envcalled without repository Aborted (core dumped)Hmph...
Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky. To see the problem, you need both my new test _and_ b1ef400ee (setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter is only in v2.13, so topics forked from v2.12 need that commit applied.
Anyway, the problem is sk/dash-is-previous, specifically fc5684b47 (revision.c: args starting with "-" might be a revision, 2017-02-25). It looks like the revision parser used to just bail on "-h", because revision.c would say "I don't recognize this" and then cmd_rev_list() would similarly say "I don't recognize this" and call usage(). But now we actually try to read it as a ref, which obviously requires being inside a repository.
Normally that's OK, but because of the "-h doesn't set up the repo" thing from 99caeed05, we may not have setup the repo, and so looking up refs is forbidden. The fix is probably to have revision.c explicitly recognize "-h" and bail on it as an unknown option (it can't handle the flag itself because only the caller knows the full usage()).
I do wonder, though, if there's any other downside to trying to look up other options as revisions (at least it wastes time doing nonsense revision lookups on options known only to cmd_rev_list()). I'm not sure why that commit passes everything starting with a dash as a possible revision, rather than just "-".
I.e.:
revision.c +8 -1
@@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s }if (opts <0 )exit (128 ); - maybe_opt =1 ; +if (arg[1 ]) { +/* arg is an unknown option */ + argv[left++] = arg; +continue ; + }else { +/* special token "-" */ + maybe_opt =1 ; + } }
I don't see anything in the commit message, but I didn't dig in the mailing list.
-Peff
Re: [PATCH 8/8] t0012: test "-h" with builtins
[+cc Siddharth, so quoting copiously]
Quoted 57 lines · Jeff King
Quoted 8 lines
Travis seems to be seeing the same failure. Curiously, the topic by itself passes for me; iow, pu fails, pu^2 doesn't fail.
git.git/pu$ ./git rev-list -h
BUG: environment.c:173:setup_git_envcalled without repository Aborted (core dumped)Hmph...
Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky. To see the problem, you need both my new test _and_
b1ef400ee(setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter is only in v2.13, so topics forked from v2.12 need that commit applied.Anyway, the problem is sk/dash-is-previous, specifically
fc5684b47(revision.c: args starting with "-" might be a revision, 2017-02-25). It looks like the revision parser used to just bail on "-h", becauserevision.cwould say "I don't recognize this" and thencmd_rev_list()would similarly say "I don't recognize this" and callusage(). But now we actually try to read it as a ref, which obviously requires being inside a repository.Normally that's OK, but because of the "-h doesn't set up the repo" thing from
99caeed05, we may not have setup the repo, and so looking up refs is forbidden. The fix is probably to haverevision.cexplicitly recognize "-h" and bail on it as an unknown option (it can't handle the flag itself because only the caller knows the fullusage()).I do wonder, though, if there's any other downside to trying to look up other options as revisions (at least it wastes time doing nonsense revision lookups on options known only to
cmd_rev_list()). I'm not sure why that commit passes everything starting with a dash as a possible revision, rather than just "-".I.e.:
revision.c +8
-1@@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s }if (opts <0 )exit (128 ); - maybe_opt =1 ; +if (arg[1 ]) { +/* arg is an unknown option */ + argv[left++] = arg; +continue ; + }else { +/* special token "-" */ + maybe_opt =1 ; + } }
Quoted 2 lines
I don't see anything in the commit message, but I didn't dig in the mailing list.
I think this line of reasoning comes from
http://public-inbox.org/git/[email protected]/
And the idea is that ranges like "-.." should work. TBH, I'm not sure how I feel about that, for exactly the reason that came up here: it makes it hard to syntactically differentiate the "-" shorthand from actual options. We do have @{-1} already for this purpose. I don't mind "-" as a shortcut for things like "git checkout -" or "git show -", but it feels like most of the benefit is lost when you're combining it with other operators.
-Peff
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 7 lines · Jeff King
And the idea is that ranges like "-.." should work. TBH, I'm not sure how I feel about that, for exactly the reason that came up here: it makes it hard to syntactically differentiate the "-" shorthand from actual options. We do have @{-1} already for this purpose. I don't mind "-" as a shortcut for things like "git checkout -" or "git show -", but it feels like most of the benefit is lost when you're combining it with other operators.
All correct and that is why I haven't seriously considered merging the topic further down (yet). Things like -.. makes readers of the commands go "Huh?", and "git tbdiff ..- -.." does not even work ;-)
Re: [PATCH 8/8] t0012: test "-h" with builtins
In reply to Jeff KingQuoted 7 lines · Jeff King
Anyway, the problem is sk/dash-is-previous, specifically
fc5684b47(revision.c: args starting with "-" might be a revision, 2017-02-25). It looks like the revision parser used to just bail on "-h", becauserevision.cwould say "I don't recognize this" and thencmd_rev_list()would similarly say "I don't recognize this" and callusage(). But now we actually try to read it as a ref, which obviously requires being inside a repository.
Heh, I found another ;-)
95e98cd9 ("revision.c: use refs_for_each*() instead of for_each_*_submodule()", 2017-04-19), which is in the middle of Duy's nd/prune-in-worktree series, does this:
#0 die (err=0x6128f8 "BUG: setup_git_env called without repository") at usage.c:114
#1 0x00000000004f9467 in setup_git_env () at environment.c:172
#2 0x00000000004f966c in get_git_dir () at environment.c:214
#3 0x000000000055113b in get_main_ref_store () at refs.c:1544
#4 0x0000000000570ee0 in handle_revision_pseudo_opt (submodule=0x0,
revs=0x7fffffffd6a0, argc=1, argv=0x7fffffffe180, flags=0x7fffffffc59c)
at revision.c:2110
#5 0x00000000005716f5 in setup_revisions (argc=2, argv=0x7fffffffe178,
revs=0x7fffffffd6a0, opt=0x0) at revision.c:2254
#6 0x000000000043074a in cmd_diff_files (argc=2, argv=0x7fffffffe178, prefix=0x0)
at builtin/diff-files.c:29
#7 0x0000000000405907 in run_builtin (p=0x87ba00 <commands+672>, argc=2,
argv=0x7fffffffe178) at git.c:376#8 0x0000000000405bb5 in handle_builtin (argc=2, argv=0x7fffffffe178) at git.c:584 #9 0x0000000000405e04 in cmd_main (argc=2, argv=0x7fffffffe178) at git.c:683 #10 0x00000000004a3364 in main (argc=2, argv=0x7fffffffe178) at common-main.c:43
when jk/consistent-h is merged into it and then "git diff-files -h" is run.
I guess anything that calls setup_revisions() from the "git cmd -h" bypass need to be prepared with that
check_help_option(argc, argv, usage, options);
thing. Which is a bit sad, but I tend to agree with you that restructuring to make usage[] of everybody available to git.c is probably too noisy for the benefit it would give us.
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Jeff King
Anyway, the problem is sk/dash-is-previous, specifically
fc5684b47(revision.c: args starting with "-" might be a revision, 2017-02-25). It looks like the revision parser used to just bail on "-h", becauserevision.cwould say "I don't recognize this" and thencmd_rev_list()would similarly say "I don't recognize this" and callusage(). But now we actually try to read it as a ref, which obviously requires being inside a repository.
Quoted 5 lines
Heh, I found another ;-)
95e98cd9("revision.c: use refs_for_each*() instead of for_each_*_submodule()", 2017-04-19), which is in the middle of Duy's nd/prune-in-worktree series, does this:
Hrm, yeah. The problem is that handle_revision_pseudo_opt() initializes the ref store at the top of the function, even if we don't see any arguments that require us to use it (and obviously in the "-h" case, we don't).
That's an implementation detail that we could fix, but I do think in general that we should probably just declare it forbidden to call setup_revisions() when the repo hasn't been discovered.
Quoted 8 lines
I guess anything that calls
setup_revisions()from the "git cmd -h" bypass need to be prepared with thatcheck_help_option(argc, argv, usage, options);thing. Which is a bit sad, but I tend to agree with you that restructuring to make usage[] of everybody available to
git.cis probably too noisy for the benefit it would give us.
The other options are:
- reverting the "-h" magic in git.c. It really is the source of most of this confusion, I think, because functions which assume RUN_SETUP are having that assumption broken. But at the same time I do think it makes "-h" a lot friendlier, and I'd prefer to keep it.
- reverting the BUG() in setup_git_env(); this has been flushing out a lot of bugs, and I think is worth keeping
I did look at writing something like check_help_option(). One of the annoyances is that we have two different usage formats: one that's a straight string for usage(), and one that's an array-of-strings for parse_options(). We could probably unify those.
It doesn't actually save that much code, though. The real value is that it abstracts the "did git.c decide to skip RUN_SETUP?" logic.
-Peff
Re: [PATCH 8/8] t0012: test "-h" with builtins
In reply to Junio C HamanoQuoted 9 lines · Junio C Hamano
Heh, I found another ;-)
95e98cd9("revision.c: use refs_for_each*() instead of for_each_*_submodule()", 2017-04-19), which is in the middle of Duy's nd/prune-in-worktree series, does this:
...
when jk/consistent-h is merged into it and then "git diff-files -h" is run.
Quoted 8 lines
I guess anything that calls
setup_revisions()from the "git cmd -h" bypass need to be prepared with thatcheck_help_option(argc, argv, usage, options);thing. Which is a bit sad, but I tend to agree with you that restructuring to make usage[] of everybody available to
git.cis probably too noisy for the benefit it would give us.
For now, I will mix this in when queuing the whole thing in 'pu', as I hate to push out something that does not even work for me to the general public.
-- >8 --
Subject: [PATCH] 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, before they can parse the options.
Handle the "git $builtin -h" special case very early in these commands to work around potential issues.
--- 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(+)
builtin/diff-files.c +3 -0
@@ -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 */
builtin/diff-index.c +3 -0
@@ -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 */
builtin/diff-tree.c +3 -0
@@ -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 */
builtin/rev-list.c +3 -0
@@ -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 ;
Signature · 1 line
2.13.0-513-g1c0955652f
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 7 lines · Junio C Hamano
For now, I will mix this in when queuing the whole thing in 'pu', as I hate to push out something that does not even work for me to the general public.
-- >8 --
Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
...
And then the check_help_option() thing may look like this.
I am not proud of the way it "unifies" the two styles of usage strings, obviously.
One benefit this patch has is that it makes it easier to highlight what it does *not* touch.
$ git grep -A2 -E -e 'a(rg)?c [!=]= 2 .*strcmp.*-h'
shows there are somewhat curious construct
if (argc != 2 || !strcmp(argv[1], "-h")) usage(...);
left in the code. Upon closer inspection, they all happen to be doing the right thing for their current set of options and arguments, but they are somewhat ugly.
builtin/am.c | 3 +-- builtin/branch.c | 3 +-- builtin/check-ref-format.c | 3 +-- builtin/checkout-index.c | 7 ++++--- builtin/commit.c | 6 ++---- builtin/diff-files.c | 3 +-- builtin/diff-index.c | 3 +-- builtin/diff-tree.c | 3 +-- builtin/gc.c | 3 +-- builtin/index-pack.c | 3 +-- builtin/ls-files.c | 3 +-- builtin/merge-ours.c | 3 +-- builtin/merge.c | 3 +-- builtin/pack-redundant.c | 3 +-- builtin/rev-list.c | 3 +-- builtin/update-index.c | 3 +-- builtin/upload-archive.c | 3 +-- fast-import.c | 3 +-- git-compat-util.h | 3 +++ usage.c | 11 +++++++++++ 20 files changed, 36 insertions(+), 39 deletions(-)
builtin/am.c +1 -2
@@ -2307,8 +2307,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_END() }; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (usage, options); +check_help_option (argc, argv, usage, options);git_config (git_am_config,NULL );
builtin/branch.c +1 -2
@@ -599,8 +599,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) filter.kind =FILTER_REFS_BRANCHES ; filter.abbrev =-1 ; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (builtin_branch_usage, options); +check_help_option (argc, argv, builtin_branch_usage, options);git_config (git_branch_config,NULL );
builtin/check-ref-format.c +1 -2
@@ -55,8 +55,7 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)int flags =0 ;const char *refname; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage (builtin_check_ref_format_usage); +check_help_option (argc, argv, builtin_check_ref_format_usage,NULL );if (argc ==3 && !strcmp (argv[1 ],"--branch" ))return check_ref_format_branch (argv[2 ]);
builtin/checkout-index.c +4 -3
@@ -179,9 +179,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) OPT_END() }; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (builtin_checkout_index_usage, - builtin_checkout_index_options); +check_help_option (argc, argv, + builtin_checkout_index_usage, + builtin_checkout_index_options); +git_config (git_default_config,NULL ); prefix_length = prefix ?strlen (prefix) :0 ;
builtin/commit.c +2 -4
@@ -1376,8 +1376,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)OPT_END (), }; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (builtin_status_usage, builtin_status_options); +check_help_option (argc, argv, builtin_status_usage, builtin_status_options);status_init_config (&s, git_status_config); argc = parse_options(argc, argv, prefix,
@@ -1661,8 +1660,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)struct ref_transaction *transaction;struct strbuf err =STRBUF_INIT ; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (builtin_commit_usage, builtin_commit_options); +check_help_option (argc, argv, builtin_commit_usage, builtin_commit_options);status_init_config (&s, git_commit_config); status_format =STATUS_FORMAT_NONE ;/* Ignore status.short */
builtin/diff-files.c +1 -2
@@ -20,8 +20,7 @@ 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); +check_help_option (argc, argv, diff_files_usage,NULL );git_config (git_diff_basic_config,NULL );/* no "diff" UI options */ init_revisions (&rev, prefix);
builtin/diff-index.c +1 -2
@@ -17,8 +17,7 @@ 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); +check_help_option (argc, argv, diff_cache_usage,NULL );git_config (git_diff_basic_config,NULL );/* no "diff" UI options */ init_revisions (&rev, prefix);
builtin/diff-tree.c +1 -2
@@ -105,8 +105,7 @@ 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); +check_help_option (argc, argv, diff_tree_usage,NULL );git_config (git_diff_basic_config,NULL );/* no "diff" UI options */ init_revisions (opt, prefix);
builtin/gc.c +1 -2
@@ -363,8 +363,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) OPT_END() }; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (builtin_gc_usage, builtin_gc_options); +check_help_option (argc, argv, builtin_gc_usage, builtin_gc_options);argv_array_pushl (&pack_refs_cmd,"pack-refs" ,"--all" ,"--prune" ,NULL );argv_array_pushl (&reflog,"reflog" ,"expire" ,"--all" ,NULL );
builtin/index-pack.c +1 -2
@@ -1640,8 +1640,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)unsigned foreign_nr =1 ;/* zero is a "good" value, assume bad */ int report_end_of_input =0 ; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage (index_pack_usage); +check_help_option (argc, argv, index_pack_usage,NULL ); check_replace_refs =0 ; fsck_options.walk = mark_link;
builtin/ls-files.c +1 -2
@@ -587,8 +587,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) OPT_END() }; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (ls_files_usage, builtin_ls_files_options); +check_help_option (argc, argv, ls_files_usage, builtin_ls_files_options);memset (&dir,0 ,sizeof (dir)); prefix = cmd_prefix;
builtin/merge-ours.c +1 -2
@@ -20,8 +20,7 @@ static const char *diff_index_args[] = {int cmd_merge_ours (int argc,const char **argv,const char *prefix) { -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage (builtin_merge_ours_usage); +check_help_option (argc, argv, builtin_merge_ours_usage,NULL );/* * We need to exit with 2 if the index does not match our HEAD tree,
builtin/merge.c +1 -2
@@ -1108,8 +1108,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)void *branch_to_free;int orig_argc = argc; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (builtin_merge_usage, builtin_merge_options); +check_help_option (argc, argv, builtin_merge_usage, builtin_merge_options);/* * Check if we are _not_ on a detached HEAD, i.e. if there is a
builtin/pack-redundant.c +1 -2
@@ -601,8 +601,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)unsigned char *sha1;char buf[42 ];/* 40 byte sha1 + \n + \0 */ -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage (pack_redundant_usage); +check_help_option (argc, argv, pack_redundant_usage,NULL );for (i =1 ; i < argc; i++) {const char *arg = argv[i];
builtin/rev-list.c +1 -2
@@ -277,8 +277,7 @@ 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); +check_help_option (argc, argv, rev_list_usage,NULL );git_config (git_default_config,NULL );init_revisions (&revs, prefix);
builtin/update-index.c +1 -2
@@ -1009,8 +1009,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) OPT_END() }; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage_with_options (update_index_usage, options); +check_help_option (argc, argv, update_index_usage, options);git_config (git_default_config,NULL );
builtin/upload-archive.c +1 -2
@@ -76,8 +76,7 @@ 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); +check_help_option (argc, argv, upload_archive_usage,NULL );/* * Set up sideband subprocess.
fast-import.c +1 -2
@@ -3447,8 +3447,7 @@ int cmd_main(int argc, const char **argv) {unsigned int i; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) -usage (fast_import_usage); +check_help_option (argc, argv, fast_import_usage,NULL );setup_git_directory ();reset_pack_idx_option (&pack_idx_opts);
git-compat-util.h +3 -0
@@ -418,6 +418,9 @@ extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2extern void warning (const char *err, ...) __attribute__((format (printf,1 ,2 )));extern void warning_errno (const char *err, ...) __attribute__((format (printf,1 ,2 ))); +struct option ; +extern void check_help_option (int ,const char **,const void *,struct option *); +#ifndef NO_OPENSSL #ifdef APPLE_COMMON_CRYPTO #include "compat/apple-common-crypto.h"
usage.c +11 -0
@@ -5,6 +5,7 @@*/ #include "git-compat-util.h" #include "cache.h" +#include "parse-options.h" void vreportf (const char *prefix,const char *err,va_list params) {
@@ -225,3 +226,13 @@ NORETURN void BUG(const char *fmt, ...) va_end(ap); }#endif + +void check_help_option (int argc,const char **argv,const void *help,struct option *opt) +{ +if (argc ==2 && !strcmp (argv[1 ],"-h" )) { +if (opt) +usage_with_options ((const char *const *)help, opt); +else +usage ((const char *)help); + } +}
Signature · 1 line
2.13.0-513-g1c0955652f
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Junio C Hamano
For now, I will mix this in when queuing the whole thing in 'pu', as I hate to push out something that does not even work for me to the general public.
Quoted 8 lines
Quoted 3 lines
-- >8 --
Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
...And then the
check_help_option()thing may look like this.I am not proud of the way it "unifies" the two styles of usage strings, obviously.
Heh, I came up with a similar hack. It is pretty nasty.
If we don't mind a one-time pain, I think we can just convert the existing usage() to something more like usage_with_options(). The patch below does that (and teaches the latter to handle a NULL options field).
The diff is ugly, and the conversion is mostly mechanical. But I think some sites can be improved. For example, look at the change in bundle.c, which now hands a set of strings rather than formatting the whole "or:" chain manually.
I think we could take this even further and break some of the really long entries (e.g., index-pack) into:
static const char *index_pack_usage[] = {
"git index-pack <pack-file>",
"git index-pack --stdin",
"",
"-v",
"-o <index-file>",
etc...
NULL};
With bonus points for actually describing each option (though at that point it may be as much work to just convert the thing to parse-options, which would also be fine with me).
That's tedious work which would need attention paid to each individual command. So I'd probably do a single mechanical patch like this one (that keeps the output identical), and then let people fix each one up on top.
Quoted 5 lines
One benefit this patch has is that it makes it easier to highlight what it does *not* touch.
$ git grep -A2 -E -e 'a(rg)?c [!=]= 2 .*strcmp.*-h'
Quoted 8 lines
shows there are somewhat curious construct
if (argc != 2 || !strcmp(argv[1], "-h")) usage(...);left in the code. Upon closer inspection, they all happen to be doing the right thing for their current set of options and arguments, but they are somewhat ugly.
Yeah, I noticed those too while looking for prior art in my "-h" series. I think they're all sane. And in fact some of them are necessary (and I think I even added one like what you quoted above) because the arguments are not otherwise parsed.
So for something that calls parse_options() later, just:
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(...);
is sufficient to catch the single funny case, and parse_options() catches the rest. But for some commands, this is all the parsing they do. We could split that in two, like:
/* early "-h" check */
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(...); /* now check for argument sanity */ if (argc != 2) usage(...);
That would make sense if we wanted to treat "-h" different from a normal "huh?" response, but we don't currently. I don't know that it's worth worrying about too much either way.
Anyway, here's the usage[] patch. I based it on the jk/consistent-h branch, since I added a few new calls there. I won't be surprised if I missed a NULL terminator somewhere, but it at least passes the big "-h" loop in t0012. :)
---
builtin.h +1 -1
@@ -8,7 +8,7 @@#define DEFAULT_MERGE_LOG_LEN 20 -extern const char git_usage_string[]; +extern const char *git_usage_string[];extern const char git_more_info_string[];#define PRUNE_PACKED_DRY_RUN 01
builtin/blame.c +2 -4
@@ -30,10 +30,8 @@#include "dir.h" #include "progress.h" -static char blame_usage[] =N_ ("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>" ); -static const char *blame_opt_usage[] = { - blame_usage, +N_ ("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>" ),"" ,N_ ("<rev-opts> are documented in git-rev-list(1)" ),NULL
@@ -2865,7 +2863,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)if (parse_range_arg (range_list.items[range_i].string, nth_line_cb, &sb, lno, anchor, &bottom, &top, sb.path)) -usage (blame_usage); +usage (blame_opt_usage);if (lno < top || ((lno || bottom) && lno < bottom)) die(Q_("file %s has only %lu line", "file %s has only %lu lines",
builtin/bundle.c +7 -5
@@ -9,11 +9,13 @@* bundle supporting "fetch", "pull", and "ls-remote". */ -static const char builtin_bundle_usage[] = -"git bundle create <file> <git-rev-list args>\n" -" or: git bundle verify <file>\n" -" or: git bundle list-heads <file> [<refname>...]\n" -" or: git bundle unbundle <file> [<refname>...]" ; +static const char *builtin_bundle_usage[] = { +"git bundle create <file> <git-rev-list args>" , +"git bundle verify <file>" , +"git bundle list-heads <file> [<refname>...]" , +"git bundle unbundle <file> [<refname>...]" , +NULL +};int cmd_bundle (int argc,const char **argv,const char *prefix) {
builtin/check-ref-format.c +5 -3
@@ -7,9 +7,11 @@#include "builtin.h" #include "strbuf.h" -static const char builtin_check_ref_format_usage[] = -"git check-ref-format [--normalize] [<options>] <refname>\n" -" or: git check-ref-format --branch <branchname-shorthand>" ; +static const char *builtin_check_ref_format_usage[] = { +"git check-ref-format [--normalize] [<options>] <refname>" , +"git check-ref-format --branch <branchname-shorthand>" , +NULL +};/* * Return a copy of refname but with leading slashes removed and runs
builtin/commit-tree.c +4 -1
@@ -10,7 +10,10 @@#include "utf8.h" #include "gpg-interface.h" -static const char commit_tree_usage[] ="git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>" ; +static const char *commit_tree_usage[] = { +"git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>" , +NULL +};static const char *sign_commit;
builtin/credential.c +4 -2
@@ -2,8 +2,10 @@#include "credential.h" #include "builtin.h" -static const char usage_msg[] = -"git credential [fill|approve|reject]" ; +static const char *usage_msg[] = { +"git credential [fill|approve|reject]" , +NULL +};int cmd_credential (int argc,const char **argv,const char *prefix) {
builtin/diff-files.c +4 -2
@@ -10,9 +10,11 @@#include "builtin.h" #include "submodule.h" -static const char diff_files_usage[] = +static const char *diff_files_usage[] = {"git diff-files [-q] [-0 | -1 | -2 | -3 | -c | --cc] [<common-diff-options>] [<path>...]" -COMMON_DIFF_OPTIONS_HELP ; +COMMON_DIFF_OPTIONS_HELP , +NULL +};int cmd_diff_files (int argc,const char **argv,const char *prefix) {
builtin/diff-index.c +4 -2
@@ -5,10 +5,12 @@#include "builtin.h" #include "submodule.h" -static const char diff_cache_usage[] = +static const char *diff_cache_usage[] = {"git diff-index [-m] [--cached] " "[<common-diff-options>] <tree-ish> [<path>...]" -COMMON_DIFF_OPTIONS_HELP ; +COMMON_DIFF_OPTIONS_HELP , +NULL +};int cmd_diff_index (int argc,const char **argv,const char *prefix) {
builtin/diff-tree.c +4 -2
@@ -79,12 +79,14 @@ static int diff_tree_stdin(char *line)return -1 ; } -static const char diff_tree_usage[] = +static const char *diff_tree_usage[] = {"git diff-tree [--stdin] [-m] [-c] [--cc] [-s] [-v] [--pretty] [-t] [-r] [--root] " "[<common-diff-options>] <tree-ish> [<tree-ish>] [<path>...]\n" " -r diff recursively\n" " --root include the initial commit as diff against /dev/null\n" -COMMON_DIFF_OPTIONS_HELP ; +COMMON_DIFF_OPTIONS_HELP , +NULL +};static void diff_tree_tweak_rev (struct rev_info *rev,struct setup_revision_opt *opt) {
builtin/diff.c +4 -2
@@ -26,8 +26,10 @@ struct blobinfo {unsigned mode; }; -static const char builtin_diff_usage[] = -"git diff [<options>] [<commit> [<commit>]] [--] [<path>...]" ; +static const char *builtin_diff_usage[] = { +"git diff [<options>] [<commit> [<commit>]] [--] [<path>...]" , +NULL +}; static void stuff_change(struct diff_options *opt, unsigned old_mode, unsigned new_mode,
builtin/fetch-pack.c +4 -2
@@ -5,10 +5,12 @@#include "connect.h" #include "sha1-array.h" -static const char fetch_pack_usage[] = +static const char *fetch_pack_usage[] = {"git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] " "[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] " -"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]" ; +"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]" , +NULL +};static void add_sought_entry (struct ref ***sought,int *nr,int *alloc,const char *name)
builtin/get-tar-commit-id.c +4 -2
@@ -7,8 +7,10 @@#include "builtin.h" #include "quote.h" -static const char builtin_get_tar_commit_id_usage[] = -"git get-tar-commit-id" ; +static const char *builtin_get_tar_commit_id_usage[] = { +"git get-tar-commit-id" , +NULL +};/* ustar header + extended global header content */ #define RECORDSIZE (512)
builtin/help.c +2 -2
@@ -459,7 +459,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)if (show_all) {git_config (git_help_config,NULL ); -printf (_ ("usage: %s%s" ),_ (git_usage_string),"\n\n" ); +printf (_ ("usage: %s%s" ),_ (git_usage_string[0 ]),"\n\n" );load_command_list ("git-" , &main_cmds, &other_cmds);list_commands (colopts, &main_cmds, &other_cmds); }
@@ -476,7 +476,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) }if (!argv[0 ]) { -printf (_ ("usage: %s%s" ),_ (git_usage_string),"\n\n" ); +printf (_ ("usage: %s%s" ),_ (git_usage_string[0 ]),"\n\n" );list_common_cmds_help ();printf ("\n%s\n" ,_ (git_more_info_string));return 0 ;
builtin/index-pack.c +4 -2
@@ -12,8 +12,10 @@#include "streaming.h" #include "thread-utils.h" -static const char index_pack_usage[] = -"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])" ; +static const char *index_pack_usage[] = { +"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])" , +NULL +};struct object_entry {struct pack_idx_entry idx;
builtin/init-db.c +1 -1
@@ -524,7 +524,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)die_errno (_ ("cannot chdir to %s" ), argv[0 ]); } }else if (0 < argc) { -usage (init_db_usage[0 ]); +usage (init_db_usage); }if (is_bare_repository_cfg ==1 ) {char *cwd =xgetcwd ();
builtin/mailinfo.c +4 -2
@@ -8,8 +8,10 @@#include "strbuf.h" #include "mailinfo.h" -static const char mailinfo_usage[] = -"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info" ; +static const char *mailinfo_usage[] = { +"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info" , +NULL +};int cmd_mailinfo (int argc,const char **argv,const char *prefix) {
builtin/mailsplit.c +4 -2
@@ -9,8 +9,10 @@#include "string-list.h" #include "strbuf.h" -static const char git_mailsplit_usage[] = -"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]" ; +static const char *git_mailsplit_usage[] = { +"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]" , +NULL +};static int is_from_line (const char *line,int len) {
builtin/merge-index.c +6 -1
@@ -65,6 +65,11 @@ static void merge_all(void) } } +static const char *usage_msg[] = { +"git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])" , +NULL +}; +int cmd_merge_index (int argc,const char **argv,const char *prefix) {int i, force_file =0 ;
@@ -75,7 +80,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)signal (SIGCHLD ,SIG_DFL );if (argc <3 ) -usage ("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])" ); +usage (usage_msg);read_cache ();
builtin/merge-ours.c +4 -2
@@ -10,8 +10,10 @@#include "git-compat-util.h" #include "builtin.h" -static const char builtin_merge_ours_usage[] = -"git merge-ours <base>... -- HEAD <remote>..." ; +static const char *builtin_merge_ours_usage[] = { +"git merge-ours <base>... -- HEAD <remote>..." , +NULL +};static const char *diff_index_args[] = {"diff-index" ,"--quiet" ,"--cached" ,"HEAD" ,"--" ,NULL
builtin/merge-tree.c +4 -1
@@ -5,7 +5,10 @@#include "exec_cmd.h" #include "merge-blobs.h" -static const char merge_tree_usage[] ="git merge-tree <base-tree> <branch1> <branch2>" ; +static const char *merge_tree_usage[] = { +"git merge-tree <base-tree> <branch1> <branch2>" , +NULL +};struct merge_list {struct merge_list *next;
builtin/mktag.c +6 -1
@@ -148,13 +148,18 @@ static int verify_tag(char *buffer, unsigned long size)return 0 ; } +static const char *usage_msg[] = { +"git mktag" , +NULL +}; +int cmd_mktag (int argc,const char **argv,const char *prefix) {struct strbuf buf =STRBUF_INIT ;unsigned char result_sha1[20 ];if (argc !=1 ) -usage ("git mktag" ); +usage (usage_msg);if (strbuf_read (&buf,0 ,4096 ) <0 ) {die_errno ("could not read from stdin" );
builtin/pack-redundant.c +4 -2
@@ -10,8 +10,10 @@#define BLKSIZE 512 -static const char pack_redundant_usage[] = -"git pack-redundant [--verbose] [--alt-odb] (--all | <filename.pack>...)" ; +static const char *pack_redundant_usage[] = { +"git pack-redundant [--verbose] [--alt-odb] (--all | <filename.pack>...)" , +NULL +};static int load_all_packs, verbose, alt_odb;
builtin/patch-id.c +4 -1
@@ -162,7 +162,10 @@ static void generate_id_list(int stable)strbuf_release (&line_buf); } -static const char patch_id_usage[] ="git patch-id [--stable | --unstable]" ; +static const char *patch_id_usage[] = { +"git patch-id [--stable | --unstable]" , +NULL +};static int git_patch_id_config (const char *var,const char *value,void *cb) {
builtin/reflog.c +16 -8
@@ -9,12 +9,18 @@#include "reachable.h" /* NEEDSWORK: switch to using parse_options */ -static const char reflog_expire_usage[] = -"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>..." ; -static const char reflog_delete_usage[] = -"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>..." ; -static const char reflog_exists_usage[] = -"git reflog exists <ref>" ; +static const char *reflog_expire_usage[] = { +"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>..." , +NULL +}; +static const char *reflog_delete_usage[] = { +"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>..." , +NULL +}; +static const char *reflog_exists_usage[] = { +"git reflog exists <ref>" , +NULL +};static timestamp_t default_reflog_expire;static timestamp_t default_reflog_expire_unreachable;
@@ -722,8 +728,10 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)* main "reflog" */ -static const char reflog_usage[] = -"git reflog [ show | expire | delete | exists ]" ; +static const char *reflog_usage[] = { +"git reflog [ show | expire | delete | exists ]" , +NULL +};int cmd_reflog (int argc,const char **argv,const char *prefix) {
builtin/remote-ext.c +4 -2
@@ -3,8 +3,10 @@#include "run-command.h" #include "pkt-line.h" -static const char usage_msg[] = -"git remote-ext <remote> <url>" ; +static const char *usage_msg[] = { +"git remote-ext <remote> <url>" , +NULL +};/* * URL syntax:
builtin/remote-fd.c +4 -2
@@ -1,8 +1,10 @@#include "builtin.h" #include "transport.h" -static const char usage_msg[] = -"git remote-fd <remote> <url>" ; +static const char *usage_msg[] = { +"git remote-fd <remote> <url>" , +NULL +};/* * URL syntax:
builtin/rev-list.c +4 -3
@@ -11,7 +11,7 @@#include "bisect.h" #include "progress.h" -static const char rev_list_usage[] = +static const char *rev_list_usage[] = {"git rev-list [OPTION] <commit-id>... [ -- paths... ]\n" " limiting output:\n" " --max-count=<n>\n"
@@ -47,8 +47,9 @@ static const char rev_list_usage[] =" special purpose:\n" " --bisect\n" " --bisect-vars\n" -" --bisect-all" -; +" --bisect-all" , +NULL +};static struct progress *progress;static unsigned progress_counter;
builtin/rev-parse.c +4 -2
@@ -530,12 +530,14 @@ static void die_no_single_rev(int quiet)die ("Needed a single revision" ); } -static const char builtin_rev_parse_usage[] = +static const char *builtin_rev_parse_usage[] = {N_ ("git rev-parse --parseopt [<options>] -- [<args>...]\n" " or: git rev-parse --sq-quote [<arg>...]\n" " or: git rev-parse [<options>] [<arg>...]\n" "\n" -"Run \"git rev-parse --parseopt -h\" for more information on the first usage." ); +"Run \"git rev-parse --parseopt -h\" for more information on the first usage." ), +NULL +};/* * Parse "opt" or "opt=<value>", setting value respectively to either
builtin/submodule--helper.c +10 -2
@@ -469,10 +469,14 @@ static int module_init(int argc, const char **argv, const char *prefix)static int module_name (int argc,const char **argv,const char *prefix) { +static const char *usage_msg[] = { +N_ ("git submodule--helper name <path>" ), +NULL + };const struct submodule *sub;if (argc !=2 ) -usage (_ ("git submodule--helper name <path>" )); +usage (usage_msg);gitmodules_config (); sub =submodule_from_path (null_sha1, argv[1 ]);
@@ -1220,9 +1224,13 @@ static struct cmd_struct commands[] = {int cmd_submodule__helper (int argc,const char **argv,const char *prefix) { +static const char *usage_msg[] = { +"git submodule--helper <command>" , +NULL + };int i;if (argc <2 || !strcmp (argv[1 ],"-h" )) -usage ("git submodule--helper <command>" ); +usage (usage_msg);for (i =0 ; i <ARRAY_SIZE (commands); i++) {if (!strcmp (argv[1 ], commands[i].cmd)) {
builtin/unpack-file.c +6 -1
@@ -20,12 +20,17 @@ static char *create_temp_file(unsigned char *sha1)return path; } +static const char *usage_msg[] = { +"git unpack-file <sha1>" , +NULL +}; +int cmd_unpack_file (int argc,const char **argv,const char *prefix) {unsigned char sha1[20 ];if (argc !=2 || !strcmp (argv[1 ],"-h" )) -usage ("git unpack-file <sha1>" ); +usage (usage_msg);if (get_sha1 (argv[1 ], sha1))die ("Not a valid object name %s" , argv[1 ]);
builtin/unpack-objects.c +4 -1
@@ -13,7 +13,10 @@#include "fsck.h" static int dry_run, quiet, recover, has_errors, strict; -static const char unpack_usage[] ="git unpack-objects [-n] [-q] [-r] [--strict]" ; +static const char *unpack_usage[] = { +"git unpack-objects [-n] [-q] [-r] [--strict]" , +NULL +};/* We always read in 4kB chunks. */ static unsigned char buffer[4096 ];
builtin/upload-archive.c +4 -2
@@ -9,8 +9,10 @@#include "run-command.h" #include "argv-array.h" -static const char upload_archive_usage[] = -"git upload-archive <repo>" ; +static const char *upload_archive_usage[] = { +"git upload-archive <repo>" , +NULL +};static const char deadchild[] ="git upload-archive: archiver died with error" ;
builtin/var.c +4 -1
@@ -5,7 +5,10 @@*/ #include "builtin.h" -static const char var_usage[] ="git var (-l | <variable>)" ; +static const char *var_usage[] = { +"git var (-l | <variable>)" , +NULL +};static const char *editor (int flag) {
daemon.c +4 -2
@@ -13,7 +13,7 @@ static int verbose;static int reuseaddr;static int informative_errors; -static const char daemon_usage[] = +static const char *daemon_usage[] = {"git daemon [--verbose] [--syslog] [--export-all]\n" " [--timeout=<n>] [--init-timeout=<n>] [--max-connections=<n>]\n" " [--strict-paths] [--base-path=<path>] [--base-path-relaxed]\n"
@@ -24,7 +24,9 @@ static const char daemon_usage[] =" [--access-hook=<path>]\n" " [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\n" " [--detach] [--user=<user> [--group=<group>]]\n" -" [<directory>...]" ; +" [<directory>...]" , +NULL +};/* List of acceptable pathname prefixes */ static const char **ok_paths;
fast-import.c +4 -2
@@ -3408,8 +3408,10 @@ static void git_pack_config(void)git_config (git_default_config,NULL ); } -static const char fast_import_usage[] = -"git fast-import [--date-format=<f>] [--max-pack-size=<n>] [--big-file-threshold=<n>] [--depth=<n>] [--active-branches=<n>] [--export-marks=<marks.file>]" ; +static const char *fast_import_usage[] = { +"git fast-import [--date-format=<f>] [--max-pack-size=<n>] [--big-file-threshold=<n>] [--depth=<n>] [--active-branches=<n>] [--export-marks=<marks.file>]" , +NULL +};static void parse_argv (void ) {
git-compat-util.h +1 -1
@@ -409,7 +409,7 @@ struct strbuf;/* General helper functions */ extern void vreportf (const char *prefix,const char *err,va_list params); -extern NORETURN void usage(const char *err); +extern NORETURN void usage(const char * const *err); extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))); extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2))); extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
git.c +5 -3
@@ -3,12 +3,14 @@#include "help.h" #include "run-command.h" -const char git_usage_string[] = +const char *git_usage_string[] = {"git [--version] [--help] [-C <path>] [-c name=value]\n" " [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n" " [-p | --paginate | --no-pager] [--no-replace-objects] [--bare]\n" " [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n" -" <command> [<args>]" ; +" <command> [<args>]" , +NULL +};const char git_more_info_string[] = N_("'git help -a' and 'git help -g' list available subcommands and some\n"
@@ -694,7 +696,7 @@ int cmd_main(int argc, const char **argv) }else {/* The user didn't specify a command; give them help */ commit_pager_choice (); -printf ("usage: %s\n\n" , git_usage_string); +printf ("usage: %s\n\n" , git_usage_string[0 ]);list_common_cmds_help ();printf ("\n%s\n" ,_ (git_more_info_string));exit (1 );
http-fetch.c +5 -2
@@ -3,8 +3,11 @@#include "http.h" #include "walker.h" -static const char http_fetch_usage[] ="git http-fetch " -"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url" ; +static const char *http_fetch_usage[] = { +"git http-fetch " +"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url" , +NULL +};int cmd_main (int argc,const char **argv) {
http-push.c +4 -2
@@ -18,8 +18,10 @@#include <expat.h> #endif -static const char http_push_usage[] = -"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n" ; +static const char *http_push_usage[] = { +"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]" , +NULL +};#ifndef XML_STATUS_OK enum XML_Status {
parse-options.c +3 -0
@@ -600,6 +600,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, usagestr++; } +if (!opts) +return PARSE_OPT_HELP ; +if (opts->type !=OPTION_GROUP )fputc ('\n' , outfile);
remote-testsvn.c +6 -1
@@ -284,6 +284,11 @@ static int do_command(struct strbuf *line)return 0 ; } +static const char *testsvn_usage[] = { +"git-remote-svn <remote-name> [<url>]" , +NULL +}; +int cmd_main (int argc,const char **argv) { struct strbuf buf = STRBUF_INIT, url_sb = STRBUF_INIT,
@@ -294,7 +299,7 @@ int cmd_main(int argc, const char **argv)setup_git_directory ();if (argc <2 || argc >3 ) { -usage ("git-remote-svn <remote-name> [<url>]" ); +usage (testsvn_usage);return 1 ; }
show-index.c +4 -2
@@ -1,8 +1,10 @@#include "cache.h" #include "pack.h" -static const char show_index_usage[] = -"git show-index" ; +static const char *show_index_usage[] = { +"git show-index" , +NULL +};int cmd_main (int argc,const char **argv) {
t/helper/test-date.c +9 -7
@@ -1,12 +1,14 @@#include "cache.h" -static const char *usage_msg ="\n" -" test-date relative [time_t]...\n" -" test-date show:<format> [time_t]...\n" -" test-date parse [date]...\n" -" test-date approxidate [date]...\n" -" test-date is64bit\n" -" test-date time_t-is64bit\n" ; +static const char *usage_msg[] = { +"test-date relative [time_t]..." , +"test-date show:<format> [time_t]..." , +"test-date parse [date]..." , +"test-date approxidate [date]..." , +"test-date is64bit" , +"test-date time_t-is64bit" , +NULL +};static void show_relative_dates (const char **argv,struct timeval *now) {
t/helper/test-line-buffer.c +1 -1
@@ -63,7 +63,7 @@ int cmd_main(int argc, const char **argv)else if (argc ==2 ) filename = argv[1 ];else -usage ("test-line-buffer [file | &fd] < script" ); +usagef ("test-line-buffer [file | &fd] < script" );if (buffer_init (&stdin_buf,NULL ))die_errno ("open error" );
t/helper/test-mktemp.c +6 -1
@@ -3,10 +3,15 @@*/ #include "git-compat-util.h" +static const char *usage_msg[] = { +"Expected 1 parameter defining the temporary file template" , +NULL +}; +int cmd_main (int argc,const char **argv) {if (argc !=2 ) -usage ("Expected 1 parameter defining the temporary file template" ); +usage (usage_msg);xmkstemp (xstrdup (argv[1 ]));
t/helper/test-regex.c +1 -1
@@ -47,7 +47,7 @@ int cmd_main(int argc, const char **argv)if (argc ==2 && !strcmp (argv[1 ],"--bug" ))return test_regex_bug ();else if (argc <3 ) -usage ("test-regex --bug\n" +usagef ("test-regex --bug\n" "test-regex <pattern> <string> [<options>]" ); argv++;
t/helper/test-svn-fe.c +4 -2
@@ -8,8 +8,10 @@#include "vcs-svn/sliding_window.h" #include "vcs-svn/line_buffer.h" -static const char test_svnfe_usage[] = -"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)" ; +static const char *test_svnfe_usage[] = { +"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)" , +NULL +};static int apply_delta (int argc,const char **argv) {
usage.c +3 -2
@@ -5,6 +5,7 @@*/ #include "git-compat-util.h" #include "cache.h" +#include "parse-options.h" void vreportf (const char *prefix,const char *err,va_list params) {
@@ -94,9 +95,9 @@ void NORETURN usagef(const char *err, ...)va_end (params); } -void NORETURN usage(const char *err) +void NORETURN usage(const char * const *err) { -usagef ("%s" , err); +usage_with_options (err,NULL ); } void NORETURN die(const char *err, ...)
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 13 lines · Jeff King
If we don't mind a one-time pain, I think we can just convert the existing
usage()to something more likeusage_with_options(). The patch below does that (and teaches the latter to handle a NULL options field).The diff is ugly, and the conversion is mostly mechanical. But I think some sites can be improved. For example, look at the change in
bundle.c, which now hands a set of strings rather than formatting the whole "or:" chain manually.
...
With bonus points for actually describing each option (though at that point it may be as much work to just convert the thing to parse-options, which would also be fine with me).
Quoted 4 lines
That's tedious work which would need attention paid to each individual command. So I'd probably do a single mechanical patch like this one (that keeps the output identical), and then let people fix each one up on top.
Yup. Unlike my other hack, this does look more useful to me. It was kind of surprising that the change to parse-options is just a single liner, but it's just "no input resulting in no output" ;-)
Quoted 45 lines
git.c +5
-3@@ -3,12 +3,14 @@#include "help.h" #include "run-command.h" -const char git_usage_string[] = +const char *git_usage_string[] = {"git [--version] [--help] [-C <path>] [-c name=value]\n" " [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n" " [-p | --paginate | --no-pager] [--no-replace-objects] [--bare]\n" " [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n" -" <command> [<args>]" ; +" <command> [<args>]" , +NULL +};const char git_more_info_string[] = N_("'git help -a' and 'git help -g' list available subcommands and some\n"@@ -694,7 +696,7 @@ int cmd_main(int argc, const char **argv) }else {/* The user didn't specify a command; give them help */ commit_pager_choice (); -printf ("usage: %s\n\n" , git_usage_string); +printf ("usage: %s\n\n" , git_usage_string[0 ]);list_common_cmds_help ();printf ("\n%s\n" ,_ (git_more_info_string));exit (1 );parse-options.c +3
-0@@ -600,6 +600,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, usagestr++; } +if (!opts) +return PARSE_OPT_HELP ; +if (opts->type !=OPTION_GROUP )fputc ('\n' , outfile);
Re: [PATCH 8/8] t0012: test "-h" with builtins
In reply to Junio C HamanoQuoted 6 lines · Junio C Hamano
For now, I will mix this in when queuing the whole thing in 'pu', as I hate to push out something that does not even work for me to the general public.
-- >8 --
Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
Yeah, I think this is a good step in the interim.
-Peff
Re: [PATCH 8/8] t0012: test "-h" with builtins
In reply to Jeff KingHi,
Quoted 7 lines · Jeff King
t/t0012-help.sh +1
-0@@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" " test_i18ncmp expect actual " +test_expect_success 'generate builtin list' '
t/t0012-help.sh test_expect_success "--help does not work for guides" " · line 62
Quoted 10 lines
t/t0012-help.sh +10
-0+ 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 <builtins
I love this. A few thoughts:
- I think the "generate builtin list" test should be marked as setup so people know it can't be skipped. I'll send a followup to do that.
- By the same token, if "generate builtin list" fails then these commands afterward would fail in an unpleasant way. Fortunately that's straightforward to fix, too.
- This checks both stdout and stderr to verify that the usage appears somewhere. I would have expected commands to be consistent --- should they always send usage to stdout, since the user requested it, or to stderr, since that's what we have done historically?
So I understand why this test is agnostic about that but I suspect it's pointing to some inconsistency that could be fixed independently.
- Do we plan to translate the "usage:" prefix some day? I could go both ways --- on one hand, having a predictable error:/usage:/fatal:
prefix may make life easier for e.g. GUI authors trying to show different kinds of error messages in different ways, but on the other hand, always using English for the prefix may be unfriendly to non English speakers.
In the past I had assumed it would never be translated but now I'm not so sure. What do you think?
- Should we have something like this for non-builtins, too?
In any case,
Re: [PATCH 8/8] t0012: test "-h" with builtins
Quoted 14 lines · Jonathan Nieder
t/t0012-help.sh test_expect_success "--help does not work for guides" " · line 62
Quoted 11 lines
t/t0012-help.sh +11
-0+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 <builtinsI love this. A few thoughts:
Quoted 6 lines
- I think the "generate builtin list" test should be marked as setup so people know it can't be skipped. I'll send a followup to do that.
- By the same token, if "generate builtin list" fails then these commands afterward would fail in an unpleasant way. Fortunately that's straightforward to fix, too.
Be my guest if you want, but I don't think either of those is actually worth spending any time on. It's a nice convenience when tests are independent, but ultimately if any single test fails, the results of all tests after it must be suspect. There are too many hidden dependencies to treat it any other way.
So I have no problem with skipping tests while debugging for a quicker turnaround, with the knowledge that what you're seeing _might_ be slightly invalidated. But I don't think it's worth dumping developer time into trying to make each block independent. That's what individual scripts are for.
(I also think it's particularly worthless for this script; the whole thing runs in ~500ms on my machine. It takes longer to type GIT_TEST_SKIP).
Quoted 7 lines
- This checks both stdout and stderr to verify that the usage appears somewhere. I would have expected commands to be consistent --- should they always send usage to stdout, since the user requested it, or to stderr, since that's what we have done historically?
So I understand why this test is agnostic about that but I suspect it's pointing to some inconsistency that could be fixed independently.
The difference is between parse-option's usage_with_options() and usage(). The former sends "-h" to stdout and errors to stderr, which I think is sensible. The latter only knows about stderr.
I think it would be reasonable to have follow the parse-options strategy consistently. It will definitely take some tweaking, though. There are lots of commands that respect "-h" only by hitting some default unhandled case. So you'll not only have to have a stdout analog for usage(), but you'll have to teach each command when to use each.
Quoted 9 lines
- Do we plan to translate the "usage:" prefix some day? I could go both ways --- on one hand, having a predictable error:/usage:/fatal:
prefix may make life easier for e.g. GUI authors trying to show different kinds of error messages in different ways, but on the other hand, always using English for the prefix may be unfriendly to non English speakers.In the past I had assumed it would never be translated but now I'm not so sure. What do you think?
I don't have an opinion either way. I know we don't translate it now, but it has been discussed. I figured it couldn't hurt to use i18ngrep to future-proof it.
Quoted 1 line
- Should we have something like this for non-builtins, too?
That would be nice, though you'll probably need some cooperation from the Makefile to get the list. I was specifically worried about catching the particular setup_git_directory() interaction with builtins here, so it seemed like a good stopping point.
But as a general rule, making sure that everything responds sensibly to "-h" seems like a good thing.
-Peff
Re: [PATCH 0/8] consistent "-h" handling in builtins
In reply to Jeff KingThe series was pretty straight-forward. Thanks for working on this.