[PATCH 0/3] preparatory patches for the submodule groups

[PATCH 0/3] preparatory patches for the submodule groups

[Stefan Beller]

These patches build on top of origin/sb/submodule-init and are preparatoy for
the submodule groups series. Junio asked to send these preparatory patches
as its own series to have a better mental focus when reviewing the groups
series later.

When sending out a similar series end of last week[1], I went to far and included
some questionable patches in the series, this series is about the minimum needed.
(patch 1 is a test suite correctness patch, which would not be strictly needed)

Thanks,
Stefan


[1] http://thread.gmane.org/gmane.comp.version-control.git/293087

Stefan Beller (3):
  submodule deinit test: fix broken && chain in subshell
  submodule deinit: lose requirement for giving '.'
  submodule init: redirect stdout to stderr

 builtin/submodule--helper.c |  3 ++-
 git-submodule.sh            |  5 -----
 t/t7400-submodule-basic.sh  |  7 +++++--
 t/t7406-submodule-update.sh | 24 ++++++++++++++++++------
 4 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.8.0.37.gb114fff.dirty

[PATCH 1/3] submodule deinit test: fix broken && chain in subshell

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7400-submodule-basic.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 814ee63..90d80d3 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -914,7 +914,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '
 		git init &&
 		>file &&
 		git add file &&
-		git commit -m "repo should not be empty"
+		git commit -m "repo should not be empty" &&
 		git submodule deinit .
 	)
 '
-- 
2.8.0.37.gb114fff.dirty

Re: [PATCH 1/3] submodule deinit test: fix broken && chain in subshell

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7400-submodule-basic.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 814ee63..90d80d3 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -914,7 +914,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>  		git init &&
>  		>file &&
>  		git add file &&
> -		git commit -m "repo should not be empty"
> +		git commit -m "repo should not be empty" &&
>  		git submodule deinit .
>  	)
>  '

Thanks.

As this was introduced by 84ba959b (submodule: fix regression for
deinit without submodules, 2016-03-22), which was merged to the
mainline at v2.8.0-rc4-8-g2a4c8c3 and applies there, let's queue it
there.

Re: [PATCH 1/3] submodule deinit test: fix broken && chain in subshell

[Stefan Beller]

On Tue, May 3, 2016 at 9:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  t/t7400-submodule-basic.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index 814ee63..90d80d3 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -914,7 +914,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>>               git init &&
>>               >file &&
>>               git add file &&
>> -             git commit -m "repo should not be empty"
>> +             git commit -m "repo should not be empty" &&
>>               git submodule deinit .
>>       )
>>  '
>
> Thanks.
>
> As this was introduced by 84ba959b (submodule: fix regression for
> deinit without submodules, 2016-03-22), which was merged to the
> mainline at v2.8.0-rc4-8-g2a4c8c3 and applies there, let's queue it
> there.

Ok, I'll reroll there with an improved 2/3 on top of v2.8.0-rc4-8-g2a4c8c3.

[PATCH 2/3] submodule deinit: lose requirement for giving '.'

[Stefan Beller]

The discussion in [1] realized that '.' is a faulty suggestion as
there is a corner case where it fails:

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

Allow no argument for `submodule deinit` to mean all submodules
and add a test to check for the corner case of an empty repository.

There is no need to update the documentation as it did not describe the
special case '.' to remove all submodules.

[1] http://news.gmane.org/gmane.comp.version-control.git/289535

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh           | 5 -----
 t/t7400-submodule-basic.sh | 5 ++++-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 82e95a9..d689265 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -428,11 +428,6 @@ cmd_deinit()
 		shift
 	done
 
-	if test $# = 0
-	then
-		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
-	fi
-
 	git submodule--helper list --prefix "$wt_prefix" "$@" |
 	while read mode sha1 stage sm_path
 	do
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 90d80d3..9af47b5 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -11,6 +11,10 @@ subcommands of git submodule.
 
 . ./test-lib.sh
 
+test_expect_success 'submodule deinit works on empty repository' '
+	git submodule deinit
+'
+
 test_expect_success 'setup - initial commit' '
 	>t &&
 	git add t &&
@@ -948,7 +952,6 @@ 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 &&
-	test_must_fail git submodule deinit &&
 	git submodule deinit . >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
-- 
2.8.0.37.gb114fff.dirty

Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The discussion in [1] realized that '.' is a faulty suggestion as
> there is a corner case where it fails:
>
>> "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
>
> Allow no argument for `submodule deinit` to mean all submodules
> and add a test to check for the corner case of an empty repository.
>
> There is no need to update the documentation as it did not describe the
> special case '.' to remove all submodules.

OK, and the reason why there is no need to update the actual code,
other than the "do not allow" gate, is because "submodule--helper
list" aka module_list already knows to list everything if no
pathspec is given.  Am I reading the code (not in the patch)
correctly?

> [1] http://news.gmane.org/gmane.comp.version-control.git/289535

The old discussion thread raises a good point.  The refusal to
accept no-pathspec form for a potentially destructive "deinit" may
have been a safety measure, even though the suggested way to tell
the command "Yes, I know I want to deinit everything" was not a
good one (i.e. it resulted in an error if your project did not have
any files tracked yet).

So possible ways forward may be

 - to remove the safety altogether; or
 - keep the safety, but give a better suggestion to say "Yes, deinit
   everything".

And this patch decides to take the former approach?

I am wondering if this can be solved in a cleaner way to teach
"deinit" take a new "--all" option instead, e.g. something like...

diff --git a/git-submodule.sh b/git-submodule.sh
index 82e95a9..4b84116 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -405,6 +405,7 @@ cmd_init()
 cmd_deinit()
 {
 	# parse $args after "submodule ... deinit".
+	deinit_all=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -414,6 +415,9 @@ cmd_deinit()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
+		-a|--all)
+			deinit_all=t
+			;;
 		--)
 			shift
 			break
@@ -428,9 +432,9 @@ cmd_deinit()
 		shift
 	done
 
-	if test $# = 0
+	if test $# = 0 && test -z "$deinit_all"
 	then
-		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
+		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
 	fi
 
 	git submodule--helper list --prefix "$wt_prefix" "$@" |


That would work even in the pathological "empty directory that has
nothing to match even '.'" case without losing the safety, no?

Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'

[Stefan Beller]

On Tue, May 3, 2016 at 10:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The discussion in [1] realized that '.' is a faulty suggestion as
>> there is a corner case where it fails:
>>
>>> "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
>>
>> Allow no argument for `submodule deinit` to mean all submodules
>> and add a test to check for the corner case of an empty repository.
>>
>> There is no need to update the documentation as it did not describe the
>> special case '.' to remove all submodules.
>
> OK, and the reason why there is no need to update the actual code,
> other than the "do not allow" gate, is because "submodule--helper
> list" aka module_list already knows to list everything if no
> pathspec is given.  Am I reading the code (not in the patch)
> correctly?

You are correct.

>
>> [1] http://news.gmane.org/gmane.comp.version-control.git/289535
>
> The old discussion thread raises a good point.  The refusal to
> accept no-pathspec form for a potentially destructive "deinit" may
> have been a safety measure, even though the suggested way to tell
> the command "Yes, I know I want to deinit everything" was not a
> good one (i.e. it resulted in an error if your project did not have
> any files tracked yet).
>
> So possible ways forward may be
>
>  - to remove the safety altogether; or
>  - keep the safety, but give a better suggestion to say "Yes, deinit
>    everything".
>
> And this patch decides to take the former approach?

Yes.

>
> I am wondering if this can be solved in a cleaner way to teach
> "deinit" take a new "--all" option instead, e.g. something like...
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 82e95a9..4b84116 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -405,6 +405,7 @@ cmd_init()
>  cmd_deinit()
>  {
>         # parse $args after "submodule ... deinit".
> +       deinit_all=
>         while test $# -ne 0
>         do
>                 case "$1" in
> @@ -414,6 +415,9 @@ cmd_deinit()
>                 -q|--quiet)
>                         GIT_QUIET=1
>                         ;;
> +               -a|--all)
> +                       deinit_all=t
> +                       ;;
>                 --)
>                         shift
>                         break
> @@ -428,9 +432,9 @@ cmd_deinit()
>                 shift
>         done
>
> -       if test $# = 0
> +       if test $# = 0 && test -z "$deinit_all"
>         then
> -               die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> +               die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
>         fi
>
>         git submodule--helper list --prefix "$wt_prefix" "$@" |
>
>
> That would work even in the pathological "empty directory that has
> nothing to match even '.'" case without losing the safety, no?

It would work for the case

    git submodule deinit --all # as you would expect.

    git submodule deinit --all COPYIN* # would break
    git submodule deinit --all . # may break
    git submodule deinit --all path/to/some/submodules/ # would be unclear to me

So maybe we want to add a check that no pathspec arguments are given when
--all is given?

Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'

[Junio C Hamano]

On Tue, May 3, 2016 at 11:07 AM, Stefan Beller <sbeller@google.com> wrote:
>
> So maybe we want to add a check that no pathspec arguments are
> given when
> --all is given?

Yeah, I overlooked that case.  Just like "commit -a path" errors
out, we should.

Re: [PATCH 2/3] submodule deinit: lose requirement for giving '.'

[Stefan Beller]

On Tue, May 3, 2016 at 12:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Tue, May 3, 2016 at 11:07 AM, Stefan Beller <sbeller@google.com> wrote:
>>
>> So maybe we want to add a check that no pathspec arguments are
>> given when
>> --all is given?
>
> Yeah, I overlooked that case.  Just like "commit -a path" errors
> out, we should.

Thanks for the patch, though. I'll add the error-out in a resend of the series?

[PATCH 3/3] submodule init: redirect stdout to stderr

[Stefan Beller]

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.

We want to init submodules from the helper for `submodule update`
in a later patch and the stdout output of said helper is consumed
by the parts of `submodule update` which are still written in shell.
So we have to be careful which messages are on stdout.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c |  3 ++-
 t/t7406-submodule-update.sh | 24 ++++++++++++++++++------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5d05393..7f0941d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -366,7 +366,8 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 			die(_("Failed to register url for submodule path '%s'"),
 			    displaypath);
 		if (!quiet)
-			printf(_("Submodule '%s' (%s) registered for path '%s'\n"),
+			fprintf(stderr,
+				_("Submodule '%s' (%s) registered for path '%s'\n"),
 				sub->name, url, displaypath);
 	}
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index fd741f5..5f27879 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -108,24 +108,36 @@ pwd=$(pwd)
 
 cat <<EOF >expect
 Submodule path '../super': checked out '$supersha1'
-Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
-Submodule 'none' ($pwd/none) registered for path '../super/none'
-Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
-Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
 Submodule path '../super/merging': checked out '$mergingsha1'
 Submodule path '../super/none': checked out '$nonesha1'
 Submodule path '../super/rebasing': checked out '$rebasingsha1'
 Submodule path '../super/submodule': checked out '$submodulesha1'
 EOF
 
+cat <<EOF >expect2
+Submodule 'merging' ($pwd/merging) registered for path '../super/merging'
+Submodule 'none' ($pwd/none) registered for path '../super/none'
+Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing'
+Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule'
+Cloning into '$pwd/recursivesuper/super/merging'...
+done.
+Cloning into '$pwd/recursivesuper/super/none'...
+done.
+Cloning into '$pwd/recursivesuper/super/rebasing'...
+done.
+Cloning into '$pwd/recursivesuper/super/submodule'...
+done.
+EOF
+
 test_expect_success 'submodule update --init --recursive from subdirectory' '
 	git -C recursivesuper/super reset --hard HEAD^ &&
 	(cd recursivesuper &&
 	 mkdir tmp &&
 	 cd tmp &&
-	 git submodule update --init --recursive ../super >../../actual
+	 git submodule update --init --recursive ../super >../../actual 2>../../actual2
 	) &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_cmp expect2 actual2
 '
 
 apos="'";
-- 
2.8.0.37.gb114fff.dirty

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Stefan Beller (3):
>   submodule deinit test: fix broken && chain in subshell
>   submodule deinit: lose requirement for giving '.'
>   submodule init: redirect stdout to stderr

So...

 * I'll take "&&-chain" patch on a separate topic on 84ba959, to be
   later merged to 'master' and probably to 'maint'.

 * I'll queue the "send diag message to stderr" patch on top of
   sb/submodule-init.

 * As to the second one, I prefer to hear opinions from others
   before choosing the possible two approaches.  Perhaps losing the
   "safety" is acceptable.  Otherwise, we could use the one I sent
   but with a "-a and pathspec are incompatible" fix.  That can be
   on its own separate topic.

Thanks.

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Stefan Beller]

On Tue, May 3, 2016 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Stefan Beller (3):
>>   submodule deinit test: fix broken && chain in subshell
>>   submodule deinit: lose requirement for giving '.'
>>   submodule init: redirect stdout to stderr
>
> So...
>
>  * I'll take "&&-chain" patch on a separate topic on 84ba959, to be
>    later merged to 'master' and probably to 'maint'.
>
>  * I'll queue the "send diag message to stderr" patch on top of
>    sb/submodule-init.
>
>  * As to the second one, I prefer to hear opinions from others
>    before choosing the possible two approaches.  Perhaps losing the
>    "safety" is acceptable.  Otherwise, we could use the one I sent
>    but with a "-a and pathspec are incompatible" fix.  That can be
>    on its own separate topic.

I have your patch here and have a "-a and pathspec are incompatible" fix
build on top.
* I do wonder if we want to have the shortform '-a' though.
* I think we want to head for consistency, eventually.
   e.g. commands with no arguments such as tag, branch
   give a list of their respective domain.

   Subcommands do not give lists by default, e.g.
   `git stash clear`, `git remote prune`
   which are the moral equivalent to
   `git submodule deinit` just work as they were told, no --switch needed.
   However Jonathan suggests we may want to reserve the no arguments space
   for future use and use the '--all' instead.

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I have your patch here and have a "-a and pathspec are incompatible" fix
> build on top.
> * I do wonder if we want to have the shortform '-a' though.

I do not particularly care.  I was merely matching the other two
options there.

> * I think we want to head for consistency, eventually.
>    e.g. commands with no arguments such as tag, branch
>    give a list of their respective domain.

Isn't that a historical mistake we are regretting, though?  Only
after many other operation modes were invented and "create X" proves
not to be the only primary modes we had to invent "tag -l" and
"branch -l".  Aren't we better off not having "no option means list"
kind of default?

>    Subcommands do not give lists by default, e.g.
>    `git stash clear`, `git remote prune`
>    which are the moral equivalent to
>    `git submodule deinit` just work as they were told, no --switch needed.

I wouldn't say "git rm" should remove everything by extending that
logic, but I can certainly buy if somebody argues "git submodule
deinit" is not destructive enough to warrant extra safety.

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Stefan Beller]

On Tue, May 3, 2016 at 2:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I have your patch here and have a "-a and pathspec are incompatible" fix
>> build on top.
>> * I do wonder if we want to have the shortform '-a' though.
>
> I do not particularly care.  I was merely matching the other two
> options there.
>
>> * I think we want to head for consistency, eventually.
>>    e.g. commands with no arguments such as tag, branch
>>    give a list of their respective domain.
>
> Isn't that a historical mistake we are regretting, though?  Only
> after many other operation modes were invented and "create X" proves
> not to be the only primary modes we had to invent "tag -l" and
> "branch -l".  Aren't we better off not having "no option means list"
> kind of default?

listing is not destructive, and I really like to not type a single dash
for some commands.

>
>>    Subcommands do not give lists by default, e.g.
>>    `git stash clear`, `git remote prune`
>>    which are the moral equivalent to
>>    `git submodule deinit` just work as they were told, no --switch needed.
>
> I wouldn't say "git rm" should remove everything by extending that
> logic, but I can certainly buy if somebody argues "git submodule
> deinit" is not destructive enough to warrant extra safety.

`git rm` is a command, not a subcommand though.

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>> * I think we want to head for consistency, eventually.
>>>    e.g. commands with no arguments such as tag, branch
>>>    give a list of their respective domain.
>>
>> Isn't that a historical mistake we are regretting, though?  Only
>> after many other operation modes were invented and "create X" proves
>> not to be the only primary modes we had to invent "tag -l" and
>> "branch -l".  Aren't we better off not having "no option means list"
>> kind of default?
>
> listing is not destructive, and I really like to not type a single dash
> for some commands.

Actually, listing is destructive to the user's cognitive state.

I wouldn't be surprised if many people wished that "git branch" did
not list (and required "git branch -l" to list) to scroll everything
they are looking away but instead showed what the current branch is.

>>>    Subcommands do not give lists by default, e.g.
>>>    `git stash clear`, `git remote prune`
>>>    which are the moral equivalent to
>>>    `git submodule deinit` just work as they were told, no --switch needed.
>>
>> I wouldn't say "git rm" should remove everything by extending that
>> logic, but I can certainly buy if somebody argues "git submodule
>> deinit" is not destructive enough to warrant extra safety.
>
> `git rm` is a command, not a subcommand though.

Yes, it is a command, just like branch and tag you brought up.

"git branch -d" is not a command, but a mode of operation.  If we
did the "mode word" UI [*1*], it would have been a subcommand.  And
I certainly would not say it should remove everything if there is no
argument on the command line.

I am not sure where you are going with that though anyway.

I think the "safety" is about forcing user to be more explicit when
triggering mass destruction, and I do not think it matters if that
destruction is done by a first-class subcommand of "git", or
subsubcommand.


[References]

*1* http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Stefan Beller]

On Tue, May 3, 2016 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>>> * I think we want to head for consistency, eventually.
>>>>    e.g. commands with no arguments such as tag, branch
>>>>    give a list of their respective domain.
>>>
>>> Isn't that a historical mistake we are regretting, though?  Only
>>> after many other operation modes were invented and "create X" proves
>>> not to be the only primary modes we had to invent "tag -l" and
>>> "branch -l".  Aren't we better off not having "no option means list"
>>> kind of default?
>>
>> listing is not destructive, and I really like to not type a single dash
>> for some commands.
>
> Actually, listing is destructive to the user's cognitive state.

Oh! I see.

>
> I wouldn't be surprised if many people wished that "git branch" did
> not list (and required "git branch -l" to list) to scroll everything
> they are looking away but instead showed what the current branch is.

Isn't that yet another more specialized mode of operation?

The difference being that showing the current branch is less lines of output
than showing all branches. (listing the branches could also be done similar
as `ls` lists files instead of `ls -1`)

>
>>>>    Subcommands do not give lists by default, e.g.
>>>>    `git stash clear`, `git remote prune`
>>>>    which are the moral equivalent to
>>>>    `git submodule deinit` just work as they were told, no --switch needed.
>>>
>>> I wouldn't say "git rm" should remove everything by extending that
>>> logic, but I can certainly buy if somebody argues "git submodule
>>> deinit" is not destructive enough to warrant extra safety.
>>
>> `git rm` is a command, not a subcommand though.
>
> Yes, it is a command, just like branch and tag you brought up.
>
> "git branch -d" is not a command, but a mode of operation.  If we
> did the "mode word" UI [*1*], it would have been a subcommand.  And
> I certainly would not say it should remove everything if there is no
> argument on the command line.

Right.

Another point of confusion is that the `submodule deinit` case
expects a path spec unlike all the other examples and path specs
have the notion for specifying "all of them" in different contexts, i.e.
'*' or '.' are valid "all path specs" in other programs.

We do not have a strong history for such "all of them" specifiers for
branches. (Well we could do a git branch -d refs/heads/* but these
are files actually, so we'd think of * as a natural character to do so)

There are no notions for "all of the stashes" (i.e.  `git stash clear`
would be weird if it expected a '*'.

I think '--all' is the right thing to do here then.

>
> I am not sure where you are going with that though anyway.

I am trying to asses the users expectations on when you expect
a "safety" feature and when to expect the operation to perform.

>
> I think the "safety" is about forcing user to be more explicit when
> triggering mass destruction, and I do not think it matters if that
> destruction is done by a first-class subcommand of "git", or
> subsubcommand.

Ok. I thought it mattered a bit as a subsubcommand is already more
explicit than a command. Compare `git stash clear` to the
fictional `git clear` command. I would expect a `git clear`
to not delete critical things without arguments. Maybe some minor things
or internal things like garbage collection.

`clear` sounding similar to `clean`, we could have a "submodule.requireForce"
similar to clean.requireForce which would change the behavior of
submodule deinit and defaults to the safe version. (I do not know the
history of clean.requireForce but that sounds like it is a safety measure
added once people complained about having no safety default.)

But this discussion strengthens my opinion that we should just have a switch
for deinit.

Thanks,
Stefan

>
>
> [References]
>
> *1* http://thread.gmane.org/gmane.comp.version-control.git/231376/focus=231478

Re: [PATCH 0/3] preparatory patches for the submodule groups

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> I wouldn't be surprised if many people wished that "git branch" did
>> not list (and required "git branch -l" to list) to scroll everything
>> they are looking away but instead showed what the current branch is.
>
> Isn't that yet another more specialized mode of operation?

Yes.  It all boils down to "what is the most common thing people
would want" and "which one of these operation modes deserve the
short-hand".

Most often, the first one that gets invented ends up squatting on
the short-and-sweet "by default without parameter, we do this" slot,
and we have to let it squat forever due to backward compatibility.

I think "git branch" that shows all 300 branches is a good example
of that--if we didn't have command line prompt support, I am
reasonably sure that "git branch --current-branch" would have been
added by popular request long time ago, and that would be a more
common thing people would want than "git branch -l".  I would not be
surprised if in an alternative universe in which both "list" and
"tell me current" were available back when the "no option default"
had not been defined yet, we gave the shortest-and-sweetest to "git
branch ---current-branch".