patch, 10 partssubmodule deinit test: fix broken && chain in subshell

> "submodule deinit ." may have "worked" in the sense that you would
> have at least one path in your tree and avoided this "nothing
> matches" most of the time.  It would have still failed with the
> exactly same error if run in an empty repository, i.e.
>
>        $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>        $ git init
>        $ rungit v2.6.6 submodule deinit .
>        error: pathspec '.' did not match any file(s) known to git.
>        Did you forget to 'git add'?
>        $ >file && git add file
>        $ rungit v2.6.6 submodule deinit .
>        $ echo $?
>        0

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-sh-setup.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index c48139a..5c02446 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -65,7 +65,7 @@ say () {
>  
>  if test -n "$OPTIONS_SPEC"; then
>  	usage() {
> -		"$0" -h
> +		"$0" -h 1>&2
>  		exit 1
>  	}

> Stefan Beller <sbeller@google.com> writes:
>
>> `git submodule asdf` would trigger displaying the usage of the submodule
>> command on stderr, however `git submodule -h` would display the usage on
>> stdout. Unify displaying help for shell commands on stderr.
>
> The primary output from "git cmd --help" is the usage message.  It
> is debatable why it should go to the standard error output when it
> is the primary thing the user asked for.

>
>
>
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  git-sh-setup.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
>> index c48139a..5c02446 100644
>> --- a/git-sh-setup.sh
>> +++ b/git-sh-setup.sh
>> @@ -65,7 +65,7 @@ say () {
>>
>>  if test -n "$OPTIONS_SPEC"; then
>>       usage() {
>> -             "$0" -h
>> +             "$0" -h 1>&2
>>               exit 1
>>       }

>     git --help |grep pull
>
> instead of
>
>     git --help 2>&1 |grep pull

> Stefan Beller <sbeller@google.com> writes:
>
>>     git --help |grep pull
>>
>> instead of
>>
>>     git --help 2>&1 |grep pull
>
> Not just that.  It makes me sad that it is unpredictable which
> stream a project happens to have chosen to send its help text and I
> end up almost always doing
>
>     random-command --help 2>&1 | less
>

> Reroute the output of stdout to stderr as it is just informative
> messages, not to be consumed by machines.
>
> This should not regress any scripts that try to parse the
> current output, as the output is already internationalized
> and therefore unstable.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index d689265..f4d500e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -271,7 +271,7 @@ Use -f if you really want to add it." >&2
>  				echo >&2 "$(eval_gettext "use the '--force' option. If the local git directory is not the correct repo")"
>  				die "$(eval_gettext "or you are unsure what this means choose another name with the '--name' option.")"
>  			else
> -				echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
> +				echo >&2 "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
>  			fi
>  		fi
>  		git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit

> Reroute the output of stdout to stderr as it is just informative
> messages, not to be consumed by machines.
>
> This should not regress any scripts that try to parse the
> current output, as the output is already internationalized
> and therefore unstable.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

>  git-submodule.sh           |  8 ++++----
>  t/t7400-submodule-basic.sh | 20 ++++++++++----------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index f4d500e..3f67f4e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -452,11 +452,11 @@ cmd_deinit()
>  				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
>  			fi
>  			rm -rf "$sm_path" &&
> -			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
> -			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
> +			say >&2 "$(eval_gettext "Cleared directory '\$displaypath'")" ||
> +			say >&2 "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
>  		fi
>  
> -		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
> +		mkdir "$sm_path" || say >&2 "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
>  
>  		# Remove the .git/config entries (unless the user already did it)
>  		if test -n "$(git config --get-regexp submodule."$name\.")"
> @@ -465,7 +465,7 @@ cmd_deinit()
>  			# the user later decides to init this submodule again
>  			url=$(git config submodule."$name".url)
>  			git config --remove-section submodule."$name" 2>/dev/null &&
> -			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
> +			say >&2 "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
>  		fi
>  	done
>  }
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index a6231f1..53644da 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -935,7 +935,7 @@ test_expect_success 'submodule deinit from subdirectory' '
>  	mkdir -p sub &&
>  	(
>  		cd sub &&
> -		git submodule deinit ../init >../output
> +		git submodule deinit ../init 2>../output
>  	) &&
>  	grep "\\.\\./init" output &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
> @@ -948,7 +948,7 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '
>  	git submodule update --init &&
>  	git config submodule.example.foo bar &&
>  	git config submodule.example2.frotz nitfol &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit . 2>actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> @@ -959,7 +959,7 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '
>  test_expect_success 'submodule deinit deinits a submodule when its work tree is missing or empty' '
>  	git submodule update --init &&
>  	rm -rf init example2/* example2/.git &&
> -	git submodule deinit init example2 >actual &&
> +	git submodule deinit init example2 2>actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
>  	test_i18ngrep ! "Cleared directory .init" actual &&
> @@ -973,7 +973,7 @@ test_expect_success 'submodule deinit fails when the submodule contains modifica
>  	test_must_fail git submodule deinit init &&
>  	test -n "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -f example2/.git &&
> -	git submodule deinit -f init >actual &&
> +	git submodule deinit -f init 2>actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test_i18ngrep "Cleared directory .init" actual &&
>  	rmdir init
> @@ -985,7 +985,7 @@ test_expect_success 'submodule deinit fails when the submodule contains untracke
>  	test_must_fail git submodule deinit init &&
>  	test -n "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -f example2/.git &&
> -	git submodule deinit -f init >actual &&
> +	git submodule deinit -f init 2>actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test_i18ngrep "Cleared directory .init" actual &&
>  	rmdir init
> @@ -1000,7 +1000,7 @@ test_expect_success 'submodule deinit fails when the submodule HEAD does not mat
>  	test_must_fail git submodule deinit init &&
>  	test -n "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -f example2/.git &&
> -	git submodule deinit -f init >actual &&
> +	git submodule deinit -f init 2>actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test_i18ngrep "Cleared directory .init" actual &&
>  	rmdir init
> @@ -1008,17 +1008,17 @@ test_expect_success 'submodule deinit fails when the submodule HEAD does not mat
>  
>  test_expect_success 'submodule deinit is silent when used on an uninitialized submodule' '
>  	git submodule update --init &&
> -	git submodule deinit init >actual &&
> +	git submodule deinit init 2>actual &&
>  	test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit init >actual &&
> +	git submodule deinit init 2>actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit . 2>actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit . 2>actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&

> This also improves performance for listing submodules, because
> S_ISGITLINK is both faster as match_pathspec as well as expected to
> be true in fewer cases, so putting it first in the condition will speed
> up the loop to compute all submodules.
>
> As this partially reverts 84ba959bbdf0 (submodule: fix regression for
> deinit without submodules, 2016-03-22), this also disallows the use
> of `git submodule deinit .` to deinit all submodules, when no
> submodules are present. `deinit .` continues to work on repositories,
> which have at least one submodule.
>
> CC: Per Cederqvist <cederp@opera.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>
>> Patch 10 is a controversial thing I'd assume as it breaks existing users.
>> We should take it for the next major release (i.e. 3.0)
>> I just want to put it out here now.
>
>  builtin/submodule--helper.c |  6 +++---
>  t/t7400-submodule-basic.sh  | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7f0941d..e41de3e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -242,9 +242,9 @@ static int module_list_compute(int argc, const char **argv,
>         for (i = 0; i < active_nr; i++) {
>                 const struct cache_entry *ce = active_cache[i];
>
> -               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> -                                   0, ps_matched, 1) ||
> -                   !S_ISGITLINK(ce->ce_mode))
> +               if (!S_ISGITLINK(ce->ce_mode) ||
> +                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +                                   0, ps_matched, 1))
>                         continue;
>
>                 ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 53644da..361e6f6 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -915,7 +915,20 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>                 >file &&
>                 git add file &&
>                 git commit -m "repo should not be empty" &&
> -               git submodule deinit .
> +               git submodule deinit
> +       )
> +'
> +
> +test_expect_success 'submodule deinit refuses to deinit a file' '
> +       test_when_finished "rm -rf newdirectory" &&
> +       mkdir newdirectory &&
> +       (
> +               cd newdirectory &&
> +               git init &&
> +               >file &&
> +               git add file &&
> +               git commit -m "repo should not be empty" &&
> +               test_must_fail git submodule deinit file
>         )
>  '
>
> --
> 2.8.0.32.g71f8beb.dirty
>

> After this change, what is the simplest way to programmatically
> deinit any submodule that may exist, without failing if there are
> none?
>
> "git commit" by default refuses to make an empty commit, but
> it has the --allow-empty option.
>
> "git rm -r ." by default fails if there are no files in the repository,
> but it has the --ignore-unmatch option.
>
> It makes sense that "git submodule deinit ." should fail if there
> are no submodules, but please add support for --ignore-unmatch
> at the same time.

>
>     /ceder
>
>
> On Sat, Apr 30, 2016 at 2:40 AM, Stefan Beller <sbeller@google.com> wrote:
>> This also improves performance for listing submodules, because
>> S_ISGITLINK is both faster as match_pathspec as well as expected to
>> be true in fewer cases, so putting it first in the condition will speed
>> up the loop to compute all submodules.
>>
>> As this partially reverts 84ba959bbdf0 (submodule: fix regression for
>> deinit without submodules, 2016-03-22), this also disallows the use
>> of `git submodule deinit .` to deinit all submodules, when no
>> submodules are present. `deinit .` continues to work on repositories,
>> which have at least one submodule.
>>
>> CC: Per Cederqvist <cederp@opera.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>>
>>> Patch 10 is a controversial thing I'd assume as it breaks existing users.
>>> We should take it for the next major release (i.e. 3.0)
>>> I just want to put it out here now.
>>
>>  builtin/submodule--helper.c |  6 +++---
>>  t/t7400-submodule-basic.sh  | 15 ++++++++++++++-
>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 7f0941d..e41de3e 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -242,9 +242,9 @@ static int module_list_compute(int argc, const char **argv,
>>         for (i = 0; i < active_nr; i++) {
>>                 const struct cache_entry *ce = active_cache[i];
>>
>> -               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
>> -                                   0, ps_matched, 1) ||
>> -                   !S_ISGITLINK(ce->ce_mode))
>> +               if (!S_ISGITLINK(ce->ce_mode) ||
>> +                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),
>> +                                   0, ps_matched, 1))
>>                         continue;
>>
>>                 ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index 53644da..361e6f6 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -915,7 +915,20 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>>                 >file &&
>>                 git add file &&
>>                 git commit -m "repo should not be empty" &&
>> -               git submodule deinit .
>> +               git submodule deinit
>> +       )
>> +'
>> +
>> +test_expect_success 'submodule deinit refuses to deinit a file' '
>> +       test_when_finished "rm -rf newdirectory" &&
>> +       mkdir newdirectory &&
>> +       (
>> +               cd newdirectory &&
>> +               git init &&
>> +               >file &&
>> +               git add file &&
>> +               git commit -m "repo should not be empty" &&
>> +               test_must_fail git submodule deinit file
>>         )
>>  '
>>
>> --
>> 2.8.0.32.g71f8beb.dirty
>>

> On Mon, May 2, 2016 at 1:26 AM, Per Cederqvist <cederp@opera.com> wrote:
>> After this change, what is the simplest way to programmatically
>> deinit any submodule that may exist, without failing if there are
>> none?
>>
>> "git commit" by default refuses to make an empty commit, but
>> it has the --allow-empty option.
>>
>> "git rm -r ." by default fails if there are no files in the repository,
>> but it has the --ignore-unmatch option.
>>
>> It makes sense that "git submodule deinit ." should fail if there
>> are no submodules, but please add support for --ignore-unmatch
>> at the same time.

>
> Oh right. I'll add the --ignore-unmatch option when rerolling this series.
>
> Thanks,
> Stefan
>
>>
>>     /ceder
>>
>>
>> On Sat, Apr 30, 2016 at 2:40 AM, Stefan Beller <sbeller@google.com> wrote:
>>> This also improves performance for listing submodules, because
>>> S_ISGITLINK is both faster as match_pathspec as well as expected to
>>> be true in fewer cases, so putting it first in the condition will speed
>>> up the loop to compute all submodules.
>>>
>>> As this partially reverts 84ba959bbdf0 (submodule: fix regression for
>>> deinit without submodules, 2016-03-22), this also disallows the use
>>> of `git submodule deinit .` to deinit all submodules, when no
>>> submodules are present. `deinit .` continues to work on repositories,
>>> which have at least one submodule.
>>>
>>> CC: Per Cederqvist <cederp@opera.com>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>
>>>
>>>> Patch 10 is a controversial thing I'd assume as it breaks existing users.
>>>> We should take it for the next major release (i.e. 3.0)
>>>> I just want to put it out here now.
>>>
>>>  builtin/submodule--helper.c |  6 +++---
>>>  t/t7400-submodule-basic.sh  | 15 ++++++++++++++-
>>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>>> index 7f0941d..e41de3e 100644
>>> --- a/builtin/submodule--helper.c
>>> +++ b/builtin/submodule--helper.c
>>> @@ -242,9 +242,9 @@ static int module_list_compute(int argc, const char **argv,
>>>         for (i = 0; i < active_nr; i++) {
>>>                 const struct cache_entry *ce = active_cache[i];
>>>
>>> -               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
>>> -                                   0, ps_matched, 1) ||
>>> -                   !S_ISGITLINK(ce->ce_mode))
>>> +               if (!S_ISGITLINK(ce->ce_mode) ||
>>> +                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),
>>> +                                   0, ps_matched, 1))
>>>                         continue;
>>>
>>>                 ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
>>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>>> index 53644da..361e6f6 100755
>>> --- a/t/t7400-submodule-basic.sh
>>> +++ b/t/t7400-submodule-basic.sh
>>> @@ -915,7 +915,20 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>>>                 >file &&
>>>                 git add file &&
>>>                 git commit -m "repo should not be empty" &&
>>> -               git submodule deinit .
>>> +               git submodule deinit
>>> +       )
>>> +'
>>> +
>>> +test_expect_success 'submodule deinit refuses to deinit a file' '
>>> +       test_when_finished "rm -rf newdirectory" &&
>>> +       mkdir newdirectory &&
>>> +       (
>>> +               cd newdirectory &&
>>> +               git init &&
>>> +               >file &&
>>> +               git add file &&
>>> +               git commit -m "repo should not be empty" &&
>>> +               test_must_fail git submodule deinit file
>>>         )
>>>  '
>>>
>>> --
>>> 2.8.0.32.g71f8beb.dirty
>>>