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

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

[Ramkumar Ramachandra]

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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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?

> 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.

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

[Jens Lehmann]

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.

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

[Marc Branchaud]

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.
> 
> 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.

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

[Junio C Hamano]

Marc Branchaud <mbranchaud@xiplink.com> writes:

> 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.

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

[Ramkumar Ramachandra]

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

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

[Junio C Hamano]

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?

 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' '

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> 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