{"thread":{"id":"31627","subject":"submodule: if $command was not matched, don't parse other args","startedAt":"2012-09-22T11:27:59Z","lastAt":"2012-09-24T18:49:04Z","messageCount":8,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jens Lehmann","Marc Branchaud"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"199736","messageId":"CALkWK0npySdS7FDt=6VKdtoNS2gqQH5WaTQ4H6TEmXdP9fuF=g@mail.gmail.com","threadId":"31627","inReplyTo":null,"subject":"submodule: if $command was not matched, don't parse other args","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-22T11:27:59Z","receivedAt":"2012-09-22T11:27:59Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"When we try to execute 'git submodule' with an invalid subcommand, we\nget an error like the following:\n\n $ git submodule show\n error: pathspec 'show' did not match any file(s) known to git.\n Did you forget to 'git add'?\n\nThe cause of the problem: since $command is not matched, it is set to\n\"status\", and \"show\" is treated as an argument to \"status\". Change\nthis so that usage information is printed when an invalid subcommand\nis tried.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n This breaks test 41 in t7400-submodule-bash -- does the test cover a\n real-world usecase?\n\n git-submodule.sh | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a7e933e..dfec45d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1108,7 +1108,15 @@ do\n done\n\n # No command word defaults to \"status\"\n-test -n \"$command\" || command=status\n+if test -z \"$command\"\n+then\n+ if test $# = 0\n+ then\n+\tcommand=status\n+ else\n+\tusage\n+ fi\n+fi\n\n # \"-b branch\" is accepted only by \"add\"\n if test -n \"$branch\" && test \"$command\" != add\n-- \n1.7.12.GIT\n"},{"id":"199747","messageId":"7v8vc13ilc.fsf@alter.siamese.dyndns.org","threadId":"31627","inReplyTo":"CALkWK0npySdS7FDt=6VKdtoNS2gqQH5WaTQ4H6TEmXdP9fuF=g@mail.gmail.com","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-22T20:31:27Z","receivedAt":"2012-09-22T20:31:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> When we try to execute 'git submodule' with an invalid subcommand, we\n> get an error like the following:\n>\n> $ git submodule show\n> error: pathspec 'show' did not match any file(s) known to git.\n> Did you forget to 'git add'?\n>\n> The cause of the problem: since $command is not matched, it is set to\n> \"status\", and \"show\" is treated as an argument to \"status\". Change\n> this so that usage information is printed when an invalid subcommand\n> is tried.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> This breaks test 41 in t7400-submodule-bash -- does the test cover a\n> real-world usecase?\n\nYou know how to ask \"shortlog --since=18.months --no-merges\" to find\npeople to list on \"Cc:\" line to ask that question, no?\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index a7e933e..dfec45d 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -1108,7 +1108,15 @@ do\n> done\n>\n> # No command word defaults to \"status\"\n> -test -n \"$command\" || command=status\n> +if test -z \"$command\"\n> +then\n> + if test $# = 0\n> + then\n> +\tcommand=status\n> + else\n> +\tusage\n> + fi\n> +fi\n\nI personally feel \"no command means this default\" is a mistake for\n\"git submodule\", even if there is no pathspec or other arguments,\nbut I am not a heavy user of submodules, so others should discuss\nthis.\n"},{"id":"199781","messageId":"505F489B.1000309@web.de","threadId":"31627","inReplyTo":"7v8vc13ilc.fsf@alter.siamese.dyndns.org","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-09-23T17:36:27Z","receivedAt":"2012-09-23T17:36:27Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 22.09.2012 22:31, schrieb Junio C Hamano:\n> Ramkumar Ramachandra writes:\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index a7e933e..dfec45d 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -1108,7 +1108,15 @@ do\n>> done\n>>\n>> # No command word defaults to \"status\"\n>> -test -n \"$command\" || command=status\n>> +if test -z \"$command\"\n>> +then\n>> + if test $# = 0\n>> + then\n>> +\tcommand=status\n>> + else\n>> +\tusage\n>> + fi\n>> +fi\n> \n> I personally feel \"no command means this default\" is a mistake for\n> \"git submodule\", even if there is no pathspec or other arguments,\n> but I am not a heavy user of submodules, so others should discuss\n> this.\n\nThe commit message of 97a5d8cce9 (git-submodule: re-enable 'status'\nas the default subcommand) back from 2007 indicates that Lars did\nback then think that \"status\" is a sane default. I agree with Junio\nthat this is not optimal, but I'd rather tend to not change that\nbehavior which has been there from day one for backward compatibility\nreasons. But if many others see that as an improvement too I won't\nobject against changing it the way Ramkumar proposes (but he'd have\nto change the documentation too ;-).\n\nSince diff and status learned to display submodule status information\n(except for a submodule being uninitialized) I almost never use this\noption myself, so I'd be interested to hear what submodule users who\ndo use \"git submodule [status]\" frequently think.\n"},{"id":"199816","messageId":"50607748.6000204@xiplink.com","threadId":"31627","inReplyTo":"505F489B.1000309@web.de","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Marc Branchaud","fromEmail":"mbranchaud@xiplink.com","sentAt":"2012-09-24T15:07:52Z","receivedAt":"2012-09-24T15:07:52Z","isPatch":false,"sender":{"key":"mbranchaud@xiplink.com","avatar":null},"body":"On 12-09-23 01:36 PM, Jens Lehmann wrote:\n> Am 22.09.2012 22:31, schrieb Junio C Hamano:\n>> Ramkumar Ramachandra writes:\n>>> diff --git a/git-submodule.sh b/git-submodule.sh\n>>> index a7e933e..dfec45d 100755\n>>> --- a/git-submodule.sh\n>>> +++ b/git-submodule.sh\n>>> @@ -1108,7 +1108,15 @@ do\n>>> done\n>>>\n>>> # No command word defaults to \"status\"\n>>> -test -n \"$command\" || command=status\n>>> +if test -z \"$command\"\n>>> +then\n>>> + if test $# = 0\n>>> + then\n>>> +\tcommand=status\n>>> + else\n>>> +\tusage\n>>> + fi\n>>> +fi\n>>\n>> I personally feel \"no command means this default\" is a mistake for\n>> \"git submodule\", even if there is no pathspec or other arguments,\n>> but I am not a heavy user of submodules, so others should discuss\n>> this.\n> \n> The commit message of 97a5d8cce9 (git-submodule: re-enable 'status'\n> as the default subcommand) back from 2007 indicates that Lars did\n> back then think that \"status\" is a sane default. I agree with Junio\n> that this is not optimal, but I'd rather tend to not change that\n> behavior which has been there from day one for backward compatibility\n> reasons. But if many others see that as an improvement too I won't\n> object against changing it the way Ramkumar proposes (but he'd have\n> to change the documentation too ;-).\n> \n> Since diff and status learned to display submodule status information\n> (except for a submodule being uninitialized) I almost never use this\n> option myself, so I'd be interested to hear what submodule users who\n> do use \"git submodule [status]\" frequently think.\n\nI also almost never use \"git submodule [status]\", and I also agree that\ngit-submodule shouldn't have a default sub-command.\n\n(Honestly, submodule's status sub-command has always felt more like plumbing\nto me than something a user would work with directly. Maybe it's just the\nfull-length SHA's that put me off...)\n\n\t\tM.\n"},{"id":"199824","messageId":"7v7grj1jkr.fsf@alter.siamese.dyndns.org","threadId":"31627","inReplyTo":"50607748.6000204@xiplink.com","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-24T16:17:40Z","receivedAt":"2012-09-24T16:17:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marc Branchaud writes:\n\n> On 12-09-23 01:36 PM, Jens Lehmann wrote:\n>> Am 22.09.2012 22:31, schrieb Junio C Hamano:\n>>> Ramkumar Ramachandra writes:\n>>>> diff --git a/git-submodule.sh b/git-submodule.sh\n>>>> index a7e933e..dfec45d 100755\n>>>> --- a/git-submodule.sh\n>>>> +++ b/git-submodule.sh\n>>>> @@ -1108,7 +1108,15 @@ do\n>>>> done\n>>>>\n>>>> # No command word defaults to \"status\"\n>>>> -test -n \"$command\" || command=status\n>>>> +if test -z \"$command\"\n>>>> +then\n>>>> + if test $# = 0\n>>>> + then\n>>>> +\tcommand=status\n>>>> + else\n>>>> +\tusage\n>>>> + fi\n>>>> +fi\n>>>\n>>> I personally feel \"no command means this default\" is a mistake for\n>>> \"git submodule\", even if there is no pathspec or other arguments,\n>>> but I am not a heavy user of submodules, so others should discuss\n>>> this.\n>> \n>> ... but I'd rather tend to not change that\n>> behavior which has been there from day one for backward compatibility\n>> reasons. But if many others see that as an improvement too I won't\n>> object against changing it the way Ramkumar proposes (but he'd have\n>> to change the documentation too ;-).\n>> \n>> Since diff and status learned to display submodule status information\n>> (except for a submodule being uninitialized) I almost never use this\n>> option myself, so I'd be interested to hear what submodule users who\n>> do use \"git submodule [status]\" frequently think.\n>\n> I also almost never use \"git submodule [status]\", and I also agree that\n> git-submodule shouldn't have a default sub-command.\n\nOK, I do not think Ramkumar's patch hurts anybody, but dropping the\n\"nothing on the command line defaults to 'status' action\" could. So\nlet's queue the patch as-is at least for now and leave the default\ndiscussion to a separarte thread if needed.\n\nThanks.\n"},{"id":"199837","messageId":"CALkWK0mpDp652Hmgx2-KCw+SdFmFKHMLAOya=vRy-fsV_YH4MQ@mail.gmail.com","threadId":"31627","inReplyTo":"7v7grj1jkr.fsf@alter.siamese.dyndns.org","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-24T18:31:17Z","receivedAt":"2012-09-24T18:31:17Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> OK, I do not think Ramkumar's patch hurts anybody, but dropping the\n> \"nothing on the command line defaults to 'status' action\" could. So\n> let's queue the patch as-is at least for now and leave the default\n> discussion to a separarte thread if needed.\n\nPlease don't do that, because it breaks test 41 in\nt7400-submodule-bash. I'll add a hunk to remove the test and send a\npatch tomorrow.\n\nThanks.\n\nRam\n"},{"id":"199840","messageId":"7vbogvz2d9.fsf@alter.siamese.dyndns.org","threadId":"31627","inReplyTo":"CALkWK0mpDp652Hmgx2-KCw+SdFmFKHMLAOya=vRy-fsV_YH4MQ@mail.gmail.com","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-24T18:45:22Z","receivedAt":"2012-09-24T18:45:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> OK, I do not think Ramkumar's patch hurts anybody, but dropping the\n>> \"nothing on the command line defaults to 'status' action\" could. So\n>> let's queue the patch as-is at least for now and leave the default\n>> discussion to a separarte thread if needed.\n>\n> Please don't do that, because it breaks test 41 in\n> t7400-submodule-bash. I'll add a hunk to remove the test and send a\n> patch tomorrow.\n\nI personally see no need waiting for something trivial like this.\nIsn't it sufficient to squash the following in? Is anything else\nneeded?\n\n Documentation/git-submodule.txt | 1 -\n t/t7400-submodule-basic.sh | 4 ++--\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git i/Documentation/git-submodule.txt w/Documentation/git-submodule.txt\nindex 2de7bf0..b4683bb 100644\n--- i/Documentation/git-submodule.txt\n+++ w/Documentation/git-submodule.txt\n@@ -112,7 +112,6 @@ status::\n \tinitialized, `+` if the currently checked out submodule commit\n \tdoes not match the SHA-1 found in the index of the containing\n \trepository and `U` if the submodule has merge conflicts.\n-\tThis command is the default command for 'git submodule'.\n +\n If `--recursive` is specified, this command will recurse into nested\n submodules, and show their status as well.\ndiff --git i/t/t7400-submodule-basic.sh w/t/t7400-submodule-basic.sh\nindex 0278f48..442dc44 100755\n--- i/t/t7400-submodule-basic.sh\n+++ w/t/t7400-submodule-basic.sh\n@@ -438,8 +438,8 @@ test_expect_success 'moving to a commit without submodule does not leave empty d\n \tgit checkout second\n '\n \n-test_expect_success 'submodule warns' '\n-\ttest_failure_with_unknown_submodule\n+test_expect_success 'submodule fails' '\n+\ttest_must_fail git submodule no-such-subcommand\n '\n \n test_expect_success 'add submodules without specifying an explicit path' '\n"},{"id":"199842","messageId":"CALkWK0nzvam=VEpsBviTw3EECsz3UYiE5XR_7s1FakyGd-ZcJg@mail.gmail.com","threadId":"31627","inReplyTo":"7vbogvz2d9.fsf@alter.siamese.dyndns.org","subject":"Re: submodule: if $command was not matched, don't parse other args","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-24T18:49:04Z","receivedAt":"2012-09-24T18:49:04Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra writes:\n>\n>> Junio C Hamano wrote:\n>>> OK, I do not think Ramkumar's patch hurts anybody, but dropping the\n>>> \"nothing on the command line defaults to 'status' action\" could. So\n>>> let's queue the patch as-is at least for now and leave the default\n>>> discussion to a separarte thread if needed.\n>>\n>> Please don't do that, because it breaks test 41 in\n>> t7400-submodule-bash. I'll add a hunk to remove the test and send a\n>> patch tomorrow.\n>\n> I personally see no need waiting for something trivial like this.\n> Isn't it sufficient to squash the following in? Is anything else\n> needed?\n\nI think this is sufficient. Thanks.\n\nRam\n"}]}