Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > Since the preceding commit all sub-commands of "git submodule" have > been dispatched to "git submodule--helper" directly, we therefore > don't need to emit "usage()" if we see "--cached" without the > sub-command being "status" or "summary", we can trust that > parse_options() will spot that and barf on it. > > This does change one obscure aspect of undocumented behavior, for > "status" and "summary" we supported these undocumented forms: > > git submodule --cached (status | summary) > > As noted in a preceding commit to git-submodule.sh which removed the > "--branch" special-case, this comes down to emergent behavior seen in > 5c08dbbdf1a (git-submodule: fix subcommand parser, > 2008-01-15). I.e. we wanted to support was for subcommand-less invocations like: > > git submodule --cached > > To be synonymous with invocations that explicitly named the "status" > sub-command: > > git submodule status --cached > > But we did not intend to mix the two, and allow "--cached" to be an > option to the top-level "submodule" command when the "status" or > "summary" sub-commands were explicitly provided. > > Let's remove this undocumented edge case, which makes a subsequent > removal of git-submodule.sh easier to reason about. The test case > added here is duplicated from the existing for-loop, except for the > different and desired handling of "git submodule --cached status". Makes sense, I completely agree. > diff --git a/git-submodule.sh b/git-submodule.sh > index ac2f95c1285..4f8f62ce981 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -43,7 +43,14 @@ do > quiet=1 > ;; > --cached) > - cached=1 > + if test -z "$command" > + then > + cached=1 && > + shift && > + break > + else > + usage > + fi > ;; > --) > break Do we need the 'if test -z "$command"'? This is in a 'while test $# != 0 && test -z "$command"' loop, so it seems unnecessary. I've tried removing it and it seems to work just fine. > @@ -69,12 +76,6 @@ then > fi > fi > > -# "--cached" is accepted only by "status" and "summary" > -if test -n "$cached" && test "$command" != status && test "$command" != summary > -then > - usage > -fi > - > case "$command" in > absorbgitdirs) > git submodule--helper "$command" --prefix "$wt_prefix" "$@" > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index b50db3f1031..d8f7d6ee29a 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -31,7 +31,7 @@ test_expect_success 'submodule usage: status --' ' > test_expect_code 1 git submodule --end-of-options > ' > > -for opt in '--quiet' '--cached' > +for opt in '--quiet' > do > test_expect_success "submodule usage: status $opt" ' > git submodule $opt && > @@ -40,6 +40,17 @@ do > ' > done > > +for opt in '--cached' > +do > + test_expect_success "submodule usage: status $opt" ' > + git submodule $opt && > + git submodule status $opt && > + test_expect_code 1 git submodule $opt status >out 2>err && > + grep "^usage: git submodule" err && > + test_must_be_empty out > + ' > +done > + Now that there's only a single opt in each of these tests, do we still want to keep the for loop? > test_expect_success 'submodule deinit works on empty repository' ' > git submodule deinit --all > ' > @@ -576,7 +587,7 @@ test_expect_success 'status should be "modified" after submodule commit' ' > ' > > test_expect_success 'the --cached sha1 should be rev1' ' > - git submodule --cached status >list && > + git submodule status --cached >list && > grep "^+$rev1" list > ' > > -- > 2.38.0.1091.gf9d18265e59
