| Message ID | patch-06.10-25fadf3ffc1-20221017T115544Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: make it a built-in, remove git-submodule.sh | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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
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 @@ -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 + 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 '
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". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- git-submodule.sh | 15 ++++++++------- t/t7400-submodule-basic.sh | 15 +++++++++++++-- 2 files changed, 21 insertions(+), 9 deletions(-)