threads / discuss / 31627

submodule: if $command was not matched, don't parse other args

Subject: submodule: if $command was not matched, don't parse other args

## tl;dr

8 messages between Sep 22, 2012 and Sep 24, 2012.

replies: 7people: 4as markdown or json

Ramkumar Ramachandra· Sep 22, 2012, 11:27 UTC · lore

When we try to execute 'git submodule' with an invalid subcommand, we get an error like the following:

    $ git submodule show
    error: pathspec 'show' did not match any file(s) known to git.
    Did you forget to 'git add'?

The cause of the problem: since $command is not matched, it is set to "status", and "show" is treated as an argument to "status". Change this so that usage information is printed when an invalid subcommand is tried.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 This breaks test 41 in t7400-submodule-bash -- does the test cover a
 real-world usecase?

 git-submodule.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index a7e933e..dfec45d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1108,7 +1108,15 @@ do
 done

 # No command word defaults to "status"
-test -n "$command" || command=status
+if test -z "$command"
+then
+    if test $# = 0
+    then
+	command=status
+    else
+	usage
+    fi
+fi

 # "-b branch" is accepted only by "add"
 if test -n "$branch" && test "$command" != add

-- 
1.7.12.GIT

Junio C Hamano· Sep 22, 2012, 20:31 UTC · re: Ramkumar Ramachandra · lore

Re: submodule: if $command was not matched, don't parse other args

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 16 quoted lines

> When we try to execute 'git submodule' with an invalid subcommand, we
> get an error like the following:
>
>     $ git submodule show
>     error: pathspec 'show' did not match any file(s) known to git.
>     Did you forget to 'git add'?
>
> The cause of the problem: since $command is not matched, it is set to
> "status", and "show" is treated as an argument to "status".  Change
> this so that usage information is printed when an invalid subcommand
> is tried.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  This breaks test 41 in t7400-submodule-bash -- does the test cover a
>  real-world usecase?

You know how to ask "shortlog --since=18.months --no-merges" to find people to list on "Cc:" line to ask that question, no?

Show 18 quoted lines

> diff --git a/git-submodule.sh b/git-submodule.sh
> index a7e933e..dfec45d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1108,7 +1108,15 @@ do
>  done
>
>  # No command word defaults to "status"
> -test -n "$command" || command=status
> +if test -z "$command"
> +then
> +    if test $# = 0
> +    then
> +	command=status
> +    else
> +	usage
> +    fi
> +fi

I personally feel "no command means this default" is a mistake for "git submodule", even if there is no pathspec or other arguments, but I am not a heavy user of submodules, so others should discuss this.

Jens Lehmann· Sep 23, 2012, 17:36 UTC · re: Junio C Hamano · lore

Re: submodule: if $command was not matched, don't parse other args

Am 22.09.2012 22:31, schrieb Junio C Hamano:

Show 24 quoted lines

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index a7e933e..dfec45d 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -1108,7 +1108,15 @@ do
>>  done
>>
>>  # No command word defaults to "status"
>> -test -n "$command" || command=status
>> +if test -z "$command"
>> +then
>> +    if test $# = 0
>> +    then
>> +	command=status
>> +    else
>> +	usage
>> +    fi
>> +fi
> 
> I personally feel "no command means this default" is a mistake for
> "git submodule", even if there is no pathspec or other arguments,
> but I am not a heavy user of submodules, so others should discuss
> this.

The commit message of 97a5d8cce9 (git-submodule: re-enable 'status' as the default subcommand) back from 2007 indicates that Lars did back then think that "status" is a sane default. I agree with Junio that this is not optimal, but I'd rather tend to not change that behavior which has been there from day one for backward compatibility reasons. But if many others see that as an improvement too I won't object against changing it the way Ramkumar proposes (but he'd have to change the documentation too ;-).

Since diff and status learned to display submodule status information (except for a submodule being uninitialized) I almost never use this option myself, so I'd be interested to hear what submodule users who do use "git submodule [status]" frequently think.

Marc Branchaud· Sep 24, 2012, 15:07 UTC · re: Jens Lehmann · lore

Re: submodule: if $command was not matched, don't parse other args

On 12-09-23 01:36 PM, Jens Lehmann wrote:

Show 39 quoted lines

> Am 22.09.2012 22:31, schrieb Junio C Hamano:
>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>> diff --git a/git-submodule.sh b/git-submodule.sh
>>> index a7e933e..dfec45d 100755
>>> --- a/git-submodule.sh
>>> +++ b/git-submodule.sh
>>> @@ -1108,7 +1108,15 @@ do
>>>  done
>>>
>>>  # No command word defaults to "status"
>>> -test -n "$command" || command=status
>>> +if test -z "$command"
>>> +then
>>> +    if test $# = 0
>>> +    then
>>> +	command=status
>>> +    else
>>> +	usage
>>> +    fi
>>> +fi
>>
>> I personally feel "no command means this default" is a mistake for
>> "git submodule", even if there is no pathspec or other arguments,
>> but I am not a heavy user of submodules, so others should discuss
>> this.
> 
> The commit message of 97a5d8cce9 (git-submodule: re-enable 'status'
> as the default subcommand) back from 2007 indicates that Lars did
> back then think that "status" is a sane default. I agree with Junio
> that this is not optimal, but I'd rather tend to not change that
> behavior which has been there from day one for backward compatibility
> reasons. But if many others see that as an improvement too I won't
> object against changing it the way Ramkumar proposes (but he'd have
> to change the documentation too ;-).
> 
> Since diff and status learned to display submodule status information
> (except for a submodule being uninitialized) I almost never use this
> option myself, so I'd be interested to hear what submodule users who
> do use "git submodule [status]" frequently think.

I also almost never use "git submodule [status]", and I also agree that git-submodule shouldn't have a default sub-command.

(Honestly, submodule's status sub-command has always felt more like plumbing to me than something a user would work with directly. Maybe it's just the full-length SHA's that put me off...)

M.

Junio C Hamano· Sep 24, 2012, 16:17 UTC · re: Marc Branchaud · lore

Re: submodule: if $command was not matched, don't parse other args

Marc Branchaud <mbranchaud@xiplink.com> writes:

Show 40 quoted lines

> On 12-09-23 01:36 PM, Jens Lehmann wrote:
>> Am 22.09.2012 22:31, schrieb Junio C Hamano:
>>> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>>>> diff --git a/git-submodule.sh b/git-submodule.sh
>>>> index a7e933e..dfec45d 100755
>>>> --- a/git-submodule.sh
>>>> +++ b/git-submodule.sh
>>>> @@ -1108,7 +1108,15 @@ do
>>>>  done
>>>>
>>>>  # No command word defaults to "status"
>>>> -test -n "$command" || command=status
>>>> +if test -z "$command"
>>>> +then
>>>> +    if test $# = 0
>>>> +    then
>>>> +	command=status
>>>> +    else
>>>> +	usage
>>>> +    fi
>>>> +fi
>>>
>>> I personally feel "no command means this default" is a mistake for
>>> "git submodule", even if there is no pathspec or other arguments,
>>> but I am not a heavy user of submodules, so others should discuss
>>> this.
>> 
>> ... but I'd rather tend to not change that
>> behavior which has been there from day one for backward compatibility
>> reasons. But if many others see that as an improvement too I won't
>> object against changing it the way Ramkumar proposes (but he'd have
>> to change the documentation too ;-).
>> 
>> Since diff and status learned to display submodule status information
>> (except for a submodule being uninitialized) I almost never use this
>> option myself, so I'd be interested to hear what submodule users who
>> do use "git submodule [status]" frequently think.
>
> I also almost never use "git submodule [status]", and I also agree that
> git-submodule shouldn't have a default sub-command.

OK, I do not think Ramkumar's patch hurts anybody, but dropping the "nothing on the command line defaults to 'status' action" could. So let's queue the patch as-is at least for now and leave the default discussion to a separarte thread if needed.

Thanks.

Ramkumar Ramachandra· Sep 24, 2012, 18:31 UTC · re: Junio C Hamano · lore

Re: submodule: if $command was not matched, don't parse other args

Hi Junio,

Junio C Hamano wrote:

> OK, I do not think Ramkumar's patch hurts anybody, but dropping the
> "nothing on the command line defaults to 'status' action" could.  So
> let's queue the patch as-is at least for now and leave the default
> discussion to a separarte thread if needed.

Please don't do that, because it breaks test 41 in t7400-submodule-bash. I'll add a hunk to remove the test and send a patch tomorrow.

Thanks.

Ram

Junio C Hamano· Sep 24, 2012, 18:45 UTC · re: Ramkumar Ramachandra · lore

Re: submodule: if $command was not matched, don't parse other args

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 9 quoted lines

> Junio C Hamano wrote:
>> OK, I do not think Ramkumar's patch hurts anybody, but dropping the
>> "nothing on the command line defaults to 'status' action" could.  So
>> let's queue the patch as-is at least for now and leave the default
>> discussion to a separarte thread if needed.
>
> Please don't do that, because it breaks test 41 in
> t7400-submodule-bash.  I'll add a hunk to remove the test and send a
> patch tomorrow.

I personally see no need waiting for something trivial like this. Isn't it sufficient to squash the following in? Is anything else needed?

 Documentation/git-submodule.txt | 1 -
 t/t7400-submodule-basic.sh      | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git i/Documentation/git-submodule.txt w/Documentation/git-submodule.txt
index 2de7bf0..b4683bb 100644
--- i/Documentation/git-submodule.txt
+++ w/Documentation/git-submodule.txt
@@ -112,7 +112,6 @@ status::
 	initialized, `+` if the currently checked out submodule commit
 	does not match the SHA-1 found in the index of the containing
 	repository and `U` if the submodule has merge conflicts.
-	This command is the default command for 'git submodule'.
 +
 If `--recursive` is specified, this command will recurse into nested
 submodules, and show their status as well.
diff --git i/t/t7400-submodule-basic.sh w/t/t7400-submodule-basic.sh
index 0278f48..442dc44 100755
--- i/t/t7400-submodule-basic.sh
+++ w/t/t7400-submodule-basic.sh
@@ -438,8 +438,8 @@ test_expect_success 'moving to a commit without submodule does not leave empty d
 	git checkout second
 '
 
-test_expect_success 'submodule <invalid-path> warns' '
-	test_failure_with_unknown_submodule
+test_expect_success 'submodule <invalid-subcommand> fails' '
+	test_must_fail git submodule no-such-subcommand
 '
 
 test_expect_success 'add submodules without specifying an explicit path' '

Ramkumar Ramachandra· Sep 24, 2012, 18:49 UTC · re: Junio C Hamano · lore

Re: submodule: if $command was not matched, don't parse other args

Junio C Hamano wrote:

Show 15 quoted lines

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>> OK, I do not think Ramkumar's patch hurts anybody, but dropping the
>>> "nothing on the command line defaults to 'status' action" could.  So
>>> let's queue the patch as-is at least for now and leave the default
>>> discussion to a separarte thread if needed.
>>
>> Please don't do that, because it breaks test 41 in
>> t7400-submodule-bash.  I'll add a hunk to remove the test and send a
>> patch tomorrow.
>
> I personally see no need waiting for something trivial like this.
> Isn't it sufficient to squash the following in?  Is anything else
> needed?

I think this is sufficient.  Thanks.

Ram