| Message ID | patch-2.5-6f386fc32c8-20210718T074936Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | commit-graph: usage fixes | expand |
On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote: > If we don't handle the -h option here like most parse_options() users > we'll fall through and it'll do the right thing for us. > > I think this code added in 4ce58ee38d (commit-graph: create > git-commit-graph builtin, 2018-04-02) was always redundant, > parse_options() did this at the time, and the commit-graph code never > used PARSE_OPT_NO_INTERNAL_HELP. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/commit-graph.c | 4 ---- > t/t5318-commit-graph.sh | 5 +++++ > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index c3fa4fde3e4..baead04a03b 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -319,10 +319,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) > OPT_END(), > }; > > - if (argc == 2 && !strcmp(argv[1], "-h")) > - usage_with_options(builtin_commit_graph_usage, > - builtin_commit_graph_options); > - > git_config(git_default_config, NULL); > argc = parse_options(argc, argv, prefix, > builtin_commit_graph_options, > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > index af88f805aa2..5fccce95724 100755 > --- a/t/t5318-commit-graph.sh > +++ b/t/t5318-commit-graph.sh > @@ -5,6 +5,11 @@ test_description='commit graph' > > GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0 > > +test_expect_success 'usage' ' > + test_expect_code 129 git commit-graph -h 2>err && > + ! grep error: err New test is partially redundant to the test in the loop at the bottom of 't0012-help.sh'. > +' > + > test_expect_success 'setup full repo' ' > mkdir full && > cd "$TRASH_DIRECTORY/full" && >
On Sun, Jul 18, 2021 at 02:55:09PM +0200, Andrei Rybak wrote: > On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote: > > I think this code added in 4ce58ee38d (commit-graph: create > > git-commit-graph builtin, 2018-04-02) was always redundant, > > parse_options() did this at the time, and the commit-graph code never > > used PARSE_OPT_NO_INTERNAL_HELP. Yep, that matches my findings. > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > > index af88f805aa2..5fccce95724 100755 > > --- a/t/t5318-commit-graph.sh > > +++ b/t/t5318-commit-graph.sh > > @@ -5,6 +5,11 @@ test_description='commit graph' > > GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0 > > +test_expect_success 'usage' ' > > + test_expect_code 129 git commit-graph -h 2>err && > > + ! grep error: err > > New test is partially redundant to the test in the loop at the bottom of > 't0012-help.sh'. Agreed. The change looks good to me, but we should drop this redundant test (although inspecting `git grep -w 'git .* -h' -- t` shows that there are a handful of other ones that could probably be dropped, too). I don't really feel strongly enough to worry about cleaning up the existing ones, but we should strive to avoid adding new ones. Thanks, Taylor
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index c3fa4fde3e4..baead04a03b 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -319,10 +319,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix) OPT_END(), }; - if (argc == 2 && !strcmp(argv[1], "-h")) - usage_with_options(builtin_commit_graph_usage, - builtin_commit_graph_options); - git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, builtin_commit_graph_options, diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index af88f805aa2..5fccce95724 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -5,6 +5,11 @@ test_description='commit graph' GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0 +test_expect_success 'usage' ' + test_expect_code 129 git commit-graph -h 2>err && + ! grep error: err +' + test_expect_success 'setup full repo' ' mkdir full && cd "$TRASH_DIRECTORY/full" &&
If we don't handle the -h option here like most parse_options() users we'll fall through and it'll do the right thing for us. I think this code added in 4ce58ee38d (commit-graph: create git-commit-graph builtin, 2018-04-02) was always redundant, parse_options() did this at the time, and the commit-graph code never used PARSE_OPT_NO_INTERNAL_HELP. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/commit-graph.c | 4 ---- t/t5318-commit-graph.sh | 5 +++++ 2 files changed, 5 insertions(+), 4 deletions(-)