Don't print status output with submodule.<name>.ignore=all

Don't print status output with submodule.<name>.ignore=all

[brian m. carlson]

There are configuration options for each submodule that specify under what
circumstances git status should display output for that submodule.
Unfortunately, these settings were not being respected, and as such the tests
were marked TODO.

This patch series consists of two patches: the first is a fix for a confusing
variable name, and the latter actually makes git status not output the submodule
information.  The tests have been updated accordingly.

 git-submodule.sh  | 10 ++++++----
 t/t7508-status.sh |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

[PATCH 1/2] submodule: fix confusing variable name

[brian m. carlson]

cmd_summary reads the output of git diff, but reads in the submodule path into a
variable called name.  Since this variable does not contain the name of the
submodule, but the path, rename it to be clearer what data it actually holds.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-submodule.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2979197..30b7fc1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1032,13 +1032,13 @@ cmd_summary() {
 	# Get modified modules cared by user
 	modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" |
 		sane_egrep '^:([0-7]* )?160000' |
-		while read mod_src mod_dst sha1_src sha1_dst status name
+		while read mod_src mod_dst sha1_src sha1_dst status path
 		do
 			# Always show modules deleted or type-changed (blob<->module)
-			test $status = D -o $status = T && echo "$name" && continue
+			test $status = D -o $status = T && echo "$path" && continue
 			# Also show added or modified modules which are checked out
-			GIT_DIR="$name/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
-			echo "$name"
+			GIT_DIR="$path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
+			echo "$path"
 		done
 	)
 
-- 
1.8.4.rc1

Re: [PATCH 1/2] submodule: fix confusing variable name

[Jonathan Nieder]

brian m. carlson wrote:

> cmd_summary reads the output of git diff, but reads in the submodule
> path into a variable called name.  Since this variable does not
> contain the name of the submodule, but the path, rename it to be
> clearer what data it actually holds.

Nice.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 1/2] submodule: fix confusing variable name

[Jens Lehmann]

Am 03.08.2013 20:14, schrieb Jonathan Nieder:
> brian m. carlson wrote:
> 
>> cmd_summary reads the output of git diff, but reads in the submodule
>> path into a variable called name.  Since this variable does not
>> contain the name of the submodule, but the path, rename it to be
>> clearer what data it actually holds.
> 
> Nice.

I totally agree. Paths and names of submodules are often confused
(especially as they are identical for a submodule that was never
moved), so cleaning up our sources so they use the correct term is
The Right Thing.

But we'll have to use sm_path here (like everywhere else in the
submodule script), because we'll run into problems under Windows
otherwise (see 64394e3ae9 for details). Apart from that the patch
is fine.

Re: [PATCH 1/2] submodule: fix confusing variable name

[Fredrik Gustafsson]

On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:
> But we'll have to use sm_path here (like everywhere else in the
> submodule script), because we'll run into problems under Windows
> otherwise (see 64394e3ae9 for details). Apart from that the patch
> is fine.

We're still using path= in the foreach-script. Or rather, we're setting
it. From what I can see and from the commit message 64394e3ae9 it could
possible be a problem.

Not sure how to solve it though... Just a simple correction would break
all script depending on that.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH 1/2] submodule: fix confusing variable name

[Jens Lehmann]

Am 04.08.2013 23:29, schrieb Fredrik Gustafsson:
> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:
>> But we'll have to use sm_path here (like everywhere else in the
>> submodule script), because we'll run into problems under Windows
>> otherwise (see 64394e3ae9 for details). Apart from that the patch
>> is fine.
> 
> We're still using path= in the foreach-script. Or rather, we're setting
> it. From what I can see and from the commit message 64394e3ae9 it could
> possible be a problem.

The commit message of 64394e3ae9 explicitly talks about that one:

  We note that the foreach subcommand provides $path to user scripts
  (ie it is part of the API), so we can't simply rename it to $sm_path.

> Not sure how to solve it though... Just a simple correction would break
> all script depending on that.

That's exactly why that one is still there. We could deprecate it in
favor of $sm_path and remove it some time later, but I'm not convinced
it is an urgent issue. But me thinks the documentation of foreach should
be updated, as it still talks about $path.

Re: [PATCH 1/2] submodule: fix confusing variable name

[Ramsay Jones]

Fredrik Gustafsson wrote:
> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:
>> But we'll have to use sm_path here (like everywhere else in the
>> submodule script), because we'll run into problems under Windows
>> otherwise (see 64394e3ae9 for details). Apart from that the patch
>> is fine.
> 
> We're still using path= in the foreach-script. Or rather, we're setting
> it. From what I can see and from the commit message 64394e3ae9 it could
> possible be a problem.

Please do not use a $path variable in any script intended to be run on
windows; those poor souls who would otherwise have to fix the bugs will
thank you! :-D

Actually, it's not so much the use of a $path variable, rather the act
of _exporting_ such a variable that causes the problem. (Which is why
using $path with eval_gettext[ln] is such a problem, of course.)

As noted in the above commit, $path is unfortunately a documented part
of the public API for the foreach subcommand. However, the foreach
subcommand is (mostly) fine; given the fact that the user script is
eval-ed in a context in which $path is not exported. The reason for
the 'mostly' is simply that the user could shoot himself in the foot
by export-ing $path in their script, so that something like:

    $ git submodule foreach 'export path; echo $path `git rev-parse HEAD`'

will indeed fail (ie git rev-parse will not execute).

> Not sure how to solve it though... Just a simple correction would break
> all script depending on that.

$path is part of the public API, so we can't just remove it. It would
require a deprecation period, etc,.  (Adding/documenting $sm_path as an
alternative *may* be worth doing. dunno.)

HTH

ATB,
Ramsay Jones

Re: [PATCH 1/2] submodule: fix confusing variable name

[Fredrik Gustafsson]

On Thu, Aug 08, 2013 at 06:44:22PM +0100, Ramsay Jones wrote:
> $path is part of the public API, so we can't just remove it. It would
> require a deprecation period, etc,.  (Adding/documenting $sm_path as an
> alternative *may* be worth doing. dunno.)

Maybe something for git 2.0? Well, Jens and Junio is the ones who can
make sane decissions about this. I trust they make a good decision. The
state as now is that this is a bug for case insesitive systems.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH 1/2] submodule: fix confusing variable name

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> $path is part of the public API, so we can't just remove it. It would
> require a deprecation period, etc,.  (Adding/documenting $sm_path as an
> alternative *may* be worth doing. dunno.)

I think exporting sm_path (if not done already) and documenting the
transition may be a good starting point, but deprecation of $path
may be tricky.  There is no good way for us to warn people who
continue using $path and ask them to fix their fingers and scripts
to use $sm_path instead.

Re: [PATCH 1/2] submodule: fix confusing variable name

[Mark Levedahl]

On 08/08/2013 01:44 PM, Ramsay Jones wrote:
> Fredrik Gustafsson wrote:
>> On Sun, Aug 04, 2013 at 07:34:48PM +0200, Jens Lehmann wrote:
>>> But we'll have to use sm_path here (like everywhere else in the
>>> submodule script), because we'll run into problems under Windows
>>> otherwise (see 64394e3ae9 for details). Apart from that the patch
>>> is fine.
>>
>> We're still using path= in the foreach-script. Or rather, we're setting
>> it. From what I can see and from the commit message 64394e3ae9 it could
>> possible be a problem.
>
> Please do not use a $path variable in any script intended to be run on
> windows; those poor souls who would otherwise have to fix the bugs will
> thank you! :-D
>
> Actually, it's not so much the use of a $path variable, rather the act
> of _exporting_ such a variable that causes the problem. (Which is why
> using $path with eval_gettext[ln] is such a problem, of course.)
>

Please note that especially in this case, Cygwin != Windows. Cygwin 
allows $path, $Path, $PATH, etc., to all coexist and be accessed case 
sensitively. Exporting $path causes no problem, either. Should the eval 
invoke a Windows program, $PATH is converted to Windows format and 
exported, the other case-sensitive variants of path remain in the 
environment and can be accessed by any program implementing 
case-sensitive lookup as well. Not sure what will happen with 
case-insensitive lookups, but a quick test showed that cmd.exe is not 
bothered by $path given $PATH exists.

Mark

[PATCH 2/2] submodule: don't print status output with ignore=all

[brian m. carlson]

git status prints information for submodules, but it should ignore the status of
those which have submodule.<name>.ignore set to all.  Fix it so that it does
properly ignore those which have that setting either in .git/config or in
.gitmodules.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-submodule.sh  | 2 ++
 t/t7508-status.sh | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 30b7fc1..5694ae6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1034,6 +1034,8 @@ cmd_summary() {
 		sane_egrep '^:([0-7]* )?160000' |
 		while read mod_src mod_dst sha1_src sha1_dst status path
 		do
+			name=$(module_name "$path")
+			test $(get_submodule_config "$name" ignore none) = all && continue
 			# Always show modules deleted or type-changed (blob<->module)
 			test $status = D -o $status = T && echo "$path" && continue
 			# Also show added or modified modules which are checked out
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index ac3d0fe..fb89fb9 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1316,7 +1316,7 @@ test_expect_success "--ignore-submodules=all suppresses submodule summary" '
 	test_i18ncmp expect output
 '
 
-test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
+test_expect_success '.gitmodules ignore=all suppresses submodule summary' '
 	git config --add -f .gitmodules submodule.subname.ignore all &&
 	git config --add -f .gitmodules submodule.subname.path sm &&
 	git status > output &&
@@ -1324,7 +1324,7 @@ test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
 	git config -f .gitmodules  --remove-section submodule.subname
 '
 
-test_expect_failure '.git/config ignore=all suppresses submodule summary' '
+test_expect_success '.git/config ignore=all suppresses submodule summary' '
 	git config --add -f .gitmodules submodule.subname.ignore none &&
 	git config --add -f .gitmodules submodule.subname.path sm &&
 	git config --add submodule.subname.ignore all &&
-- 
1.8.4.rc1

Re: [PATCH 2/2] submodule: don't print status output with ignore=all

[Jonathan Nieder]

brian m. carlson wrote:

> git status prints information for submodules, but it should ignore the status of
> those which have submodule.<name>.ignore set to all.  Fix it so that it does
> properly ignore those which have that setting either in .git/config or in
> .gitmodules.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  git-submodule.sh  | 2 ++
>  t/t7508-status.sh | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)

Thanks.  Cc-ing Jens, who wrote that test and knows this code much
better than I do. :)

[...]
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1034,6 +1034,8 @@ cmd_summary() {
>  		sane_egrep '^:([0-7]* )?160000' |
>  		while read mod_src mod_dst sha1_src sha1_dst status path
>  		do
> +			name=$(module_name "$path")
> +			test $(get_submodule_config "$name" ignore none) = all && continue
>  			# Always show modules deleted or type-changed (blob<->module)
>  			test $status = D -o $status = T && echo "$path" && continue

I'm not sure what the exact semantics should be here, though that's
mostly because of my unfamiliarity with submodules in general.

If I have '[submodule "favorite"] ignore = all' and I then replace
that submodule with a blob, should "git submodule status" not mention
that path?

If I just renamed a submodule, will 'module_name "$path"' do the right
thing with the old path?

(rest of the patch kept unsnipped for reference)
>  			# Also show added or modified modules which are checked out
> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
> index ac3d0fe..fb89fb9 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1316,7 +1316,7 @@ test_expect_success "--ignore-submodules=all suppresses submodule summary" '
>  	test_i18ncmp expect output
>  '
>  
> -test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
> +test_expect_success '.gitmodules ignore=all suppresses submodule summary' '
>  	git config --add -f .gitmodules submodule.subname.ignore all &&
>  	git config --add -f .gitmodules submodule.subname.path sm &&
>  	git status > output &&
> @@ -1324,7 +1324,7 @@ test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
>  	git config -f .gitmodules  --remove-section submodule.subname
>  '
>  
> -test_expect_failure '.git/config ignore=all suppresses submodule summary' '
> +test_expect_success '.git/config ignore=all suppresses submodule summary' '
>  	git config --add -f .gitmodules submodule.subname.ignore none &&
>  	git config --add -f .gitmodules submodule.subname.path sm &&
>  	git config --add submodule.subname.ignore all &&
> -- 
> 1.8.4.rc1

Re: [PATCH 2/2] submodule: don't print status output with ignore=all

[Jens Lehmann]

Am 03.08.2013 20:24, schrieb Jonathan Nieder:
> brian m. carlson wrote:
> 
>> git status prints information for submodules, but it should ignore the status of
>> those which have submodule.<name>.ignore set to all.  Fix it so that it does
>> properly ignore those which have that setting either in .git/config or in
>> .gitmodules.

I'm a bit confused. The commit message talks about "git status", but the code
you changed handles "git submodule summary". Looks like you are trying to fix
the output of status when the status.submodulesummary option is set, right?
That's a good thing to do.

But your patch also changes the default behavior of "git submodule summary",
which is a change in behavior as that is currently not configurable via the
ignore option (and I believe it should stay that way for backward compatibility
reasons unless actual users provide sound reasons to change that). So a NACK
on this patch from me (and a note to self that tests are missing that should
have failed due to this change).

As a short term solution you could honor the submodule.<name>.ignore setting
only if --for-status is used, as that is explicitly given by "git status" when
it forks the "git submodule summary" script (to make it prepend "# " to each
line, which it could do easily itself nowadays using recently added code ;-).

If you want to fix that issue and make git status perform a lot better, you
should make the status.submodulesummary use what we already have in the diff
machinery instead of forking the submodule script (which it does for hysterical
raisins). Basically you'd need to run just what "git diff-files" and "git
diff-index HEAD" run when they are given the --submodule option, prepend "# "
to the output and limit it to the amount of lines configured via the
status.submodulesummary option. Then we could get rid of the --for-status
option of submodule summary and move some more functionality from that script
into core git.

I'll be glad to help you fixing this problem either way.

>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>>  git-submodule.sh  | 2 ++
>>  t/t7508-status.sh | 4 ++--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> Thanks.  Cc-ing Jens, who wrote that test and knows this code much
> better than I do. :)
>
> [...]
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -1034,6 +1034,8 @@ cmd_summary() {
>>  		sane_egrep '^:([0-7]* )?160000' |
>>  		while read mod_src mod_dst sha1_src sha1_dst status path
>>  		do
>> +			name=$(module_name "$path")
>> +			test $(get_submodule_config "$name" ignore none) = all && continue
>>  			# Always show modules deleted or type-changed (blob<->module)
>>  			test $status = D -o $status = T && echo "$path" && continue
> 
> I'm not sure what the exact semantics should be here, though that's
> mostly because of my unfamiliarity with submodules in general.
> 
> If I have '[submodule "favorite"] ignore = all' and I then replace
> that submodule with a blob, should "git submodule status" not mention
> that path?
> 
> If I just renamed a submodule, will 'module_name "$path"' do the right
> thing with the old path?
> 
> (rest of the patch kept unsnipped for reference)
>>  			# Also show added or modified modules which are checked out
>> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
>> index ac3d0fe..fb89fb9 100755
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -1316,7 +1316,7 @@ test_expect_success "--ignore-submodules=all suppresses submodule summary" '
>>  	test_i18ncmp expect output
>>  '
>>  
>> -test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
>> +test_expect_success '.gitmodules ignore=all suppresses submodule summary' '
>>  	git config --add -f .gitmodules submodule.subname.ignore all &&
>>  	git config --add -f .gitmodules submodule.subname.path sm &&
>>  	git status > output &&
>> @@ -1324,7 +1324,7 @@ test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
>>  	git config -f .gitmodules  --remove-section submodule.subname
>>  '
>>  
>> -test_expect_failure '.git/config ignore=all suppresses submodule summary' '
>> +test_expect_success '.git/config ignore=all suppresses submodule summary' '
>>  	git config --add -f .gitmodules submodule.subname.ignore none &&
>>  	git config --add -f .gitmodules submodule.subname.path sm &&
>>  	git config --add submodule.subname.ignore all &&
>> -- 
>> 1.8.4.rc1
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] submodule: don't print status output with ignore=all

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On Sun, Aug 04, 2013 at 08:24:09PM +0200, Jens Lehmann wrote:
> I'm a bit confused. The commit message talks about "git status", but the code
> you changed handles "git submodule summary". Looks like you are trying to fix
> the output of status when the status.submodulesummary option is set, right?
> That's a good thing to do.
> 
> But your patch also changes the default behavior of "git submodule summary",
> which is a change in behavior as that is currently not configurable via the
> ignore option (and I believe it should stay that way for backward compatibility
> reasons unless actual users provide sound reasons to change that). So a NACK
> on this patch from me (and a note to self that tests are missing that should
> have failed due to this change).

Right, that wasn't the intent.

> As a short term solution you could honor the submodule.<name>.ignore setting
> only if --for-status is used, as that is explicitly given by "git status" when
> it forks the "git submodule summary" script (to make it prepend "# " to each
> line, which it could do easily itself nowadays using recently added code ;-).

I think I'm going to go this route.  My goal is to fix up the TODO tests
and make them work so I can get more familiar with the code.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] submodule: don't print status output with ignore=all

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

On Sat, Aug 03, 2013 at 11:24:20AM -0700, Jonathan Nieder wrote:
> If I have '[submodule "favorite"] ignore = all' and I then replace
> that submodule with a blob, should "git submodule status" not mention
> that path?

Yes, I think it should.  I'll fix this in the reroll.

> If I just renamed a submodule, will 'module_name "$path"' do the right
> thing with the old path?

module_name uses whatever's in .gitmodules.  I'm not sure what you mean
by "renamed a submodule", since "git mv foo bar" fails with:

  vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq
  fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq

Can you provide me a set of steps to reproduce that operation so I can
test it effectively?

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] submodule: don't print status output with ignore=all

[Jonathan Nieder]

brian m. carlson wrote:

> module_name uses whatever's in .gitmodules.  I'm not sure what you mean
> by "renamed a submodule", since "git mv foo bar" fails with:
>
>   vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq
>   fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq
>
> Can you provide me a set of steps to reproduce that operation so I can
> test it effectively?

Ah, I forgot Jens's "submodule-mv" patch series has not hit master yet.
You can get it by running

	git merge origin/pu^{/jl/submodule-mv}^2

before building git.

Thanks,
Jonathan

Re: [PATCH 2/2] submodule: don't print status output with ignore=all

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On Sun, Aug 11, 2013 at 04:03:17PM +0000, brian m. carlson wrote:
> On Sat, Aug 03, 2013 at 11:24:20AM -0700, Jonathan Nieder wrote:
> > If I just renamed a submodule, will 'module_name "$path"' do the right
> > thing with the old path?
> 
> module_name uses whatever's in .gitmodules.  I'm not sure what you mean
> by "renamed a submodule", since "git mv foo bar" fails with:
> 
>   vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq
>   fatal: source directory is empty, source=.vim/bundle/ctrlp, destination=.vim/bundle/ctrlq

Okay, I've tested this against next and it seems that the code handles
it properly (at least I think it does).  I left the following code

  # Always show modules deleted or type-changed (blob<->module)
  test $status = D -o $status = T && echo "$sm_path" && continue

before the ignore code, so I adjusted the new ignore code so that it
prints both the new and the old path.  I don't know that it's possible
to print neither in that case, since we no longer can look up the old
path in .gitmodules.  I'll be sending my reroll shortly.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]