* [PATCH v3 0/2] submodule: Don't print status output with submodule.<name>.ignore=all
@ 2013-09-01 20:06 brian m. carlson
2013-09-01 20:06 ` [PATCH v3 1/2] submodule: fix confusing variable name brian m. carlson
2013-09-01 20:06 ` [PATCH v3 2/2] submodule: don't print status output with ignore=all brian m. carlson
0 siblings, 2 replies; 13+ messages in thread
From: brian m. carlson @ 2013-09-01 20:06 UTC (permalink / raw)
To: git; +Cc: jrnieder, Jens.Lehmann, judge.packham, gitster
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, and additional tests have been included to ensure that git
submodule summary is not affected by these changes unless the
--for-status option is given.
Changes from v2:
* Add tests to ensure that git submodule summary is not affected.
* Fix bug that caused git submodule summary to be affected.
Changes from v1:
* Handle moved submodules by not ignoring them.
* Use sm_path instead of path.
* Only ignore when --for-status is given.
brian m. carlson (2):
submodule: fix confusing variable name
submodule: don't print status output with ignore=all
git-submodule.sh | 15 +++++++++++----
t/t7401-submodule-summary.sh | 30 ++++++++++++++++++++++++++++++
t/t7508-status.sh | 4 ++--
3 files changed, 43 insertions(+), 6 deletions(-)
--
1.8.4.rc3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] submodule: fix confusing variable name
2013-09-01 20:06 [PATCH v3 0/2] submodule: Don't print status output with submodule.<name>.ignore=all brian m. carlson
@ 2013-09-01 20:06 ` brian m. carlson
2013-09-01 20:06 ` [PATCH v3 2/2] submodule: don't print status output with ignore=all brian m. carlson
1 sibling, 0 replies; 13+ messages in thread
From: brian m. carlson @ 2013-09-01 20:06 UTC (permalink / raw)
To: git; +Cc: jrnieder, Jens.Lehmann, judge.packham, gitster
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..38520db 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 sm_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 "$sm_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="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
+ echo "$sm_path"
done
)
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-01 20:06 [PATCH v3 0/2] submodule: Don't print status output with submodule.<name>.ignore=all brian m. carlson
2013-09-01 20:06 ` [PATCH v3 1/2] submodule: fix confusing variable name brian m. carlson
@ 2013-09-01 20:06 ` brian m. carlson
2013-09-03 19:17 ` Jens Lehmann
` (2 more replies)
1 sibling, 3 replies; 13+ messages in thread
From: brian m. carlson @ 2013-09-01 20:06 UTC (permalink / raw)
To: git; +Cc: jrnieder, Jens.Lehmann, judge.packham, gitster
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.
Not ignored are submodules that are added, deleted, or moved (which is
essentially a combination of the first two) because it is not easily possible to
determine the old path once a move has occurred, nor is it easily possible to
detect which adds and deletions are moves and which are not. This also
preserves the previous behavior of always listing modules which are to be
deleted.
Tests are included which verify that this change has no effect on git submodule
summary without the --for-status option.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
git-submodule.sh | 7 +++++++
t/t7401-submodule-summary.sh | 30 ++++++++++++++++++++++++++++++
t/t7508-status.sh | 4 ++--
3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 38520db..004b21c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,6 +1036,13 @@ cmd_summary() {
do
# Always show modules deleted or type-changed (blob<->module)
test $status = D -o $status = T && echo "$sm_path" && continue
+ # Respect the ignore setting for --for-status.
+ if test -n "$for_status"
+ then
+ name=$(module_name "$sm_path")
+ ignore_config=$(get_submodule_config "$name" ignore none)
+ test $status != A -a $ignore_config = all && continue
+ fi
# Also show added or modified modules which are checked out
GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
echo "$sm_path"
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index ac2434c..ca9441e 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -104,6 +104,36 @@ EOF
test_cmp expected actual
"
+test_expect_success '.gitmodules ignore=all has no effect' "
+ git config --add -f .gitmodules submodule.sm1.ignore all &&
+ git config --add -f .gitmodules submodule.sm1.path sm1 &&
+ git submodule summary >actual &&
+ cat >expected <<-EOF &&
+* sm1 $head1...$head2 (1):
+ > Add foo3
+
+EOF
+ test_cmp expected actual &&
+ git config -f .gitmodules --remove-section submodule.sm1
+"
+
+test_expect_success '.git/config ignore=all has no effect' "
+ git config --add -f .gitmodules submodule.sm1.ignore none &&
+ git config --add -f .gitmodules submodule.sm1.path sm1 &&
+ git config --add submodule.sm1.ignore all &&
+ git config --add submodule.sm1.path sm1 &&
+ git submodule summary >actual &&
+ cat >expected <<-EOF &&
+* sm1 $head1...$head2 (1):
+ > Add foo3
+
+EOF
+ test_cmp expected actual &&
+ git config --remove-section submodule.sm1 &&
+ git config -f .gitmodules --remove-section submodule.sm1
+"
+
+
commit_file sm1 &&
head3=$(
cd sm1 &&
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.rc3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-01 20:06 ` [PATCH v3 2/2] submodule: don't print status output with ignore=all brian m. carlson
@ 2013-09-03 19:17 ` Jens Lehmann
2013-09-03 19:53 ` Junio C Hamano
2013-09-04 6:31 ` Matthieu Moy
2 siblings, 0 replies; 13+ messages in thread
From: Jens Lehmann @ 2013-09-03 19:17 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, jrnieder, judge.packham, gitster, Matthieu Moy
Am 01.09.2013 22:06, schrieb 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.
>
> Not ignored are submodules that are added, deleted, or moved (which is
> essentially a combination of the first two) because it is not easily possible to
> determine the old path once a move has occurred, nor is it easily possible to
> detect which adds and deletions are moves and which are not. This also
> preserves the previous behavior of always listing modules which are to be
> deleted.
>
> Tests are included which verify that this change has no effect on git submodule
> summary without the --for-status option.
Thanks, that fixes the bug Matthieu noticed which is now tested for.
I only have some nits concerning the new tests, please see below.
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> git-submodule.sh | 7 +++++++
> t/t7401-submodule-summary.sh | 30 ++++++++++++++++++++++++++++++
> t/t7508-status.sh | 4 ++--
> 3 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 38520db..004b21c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1036,6 +1036,13 @@ cmd_summary() {
> do
> # Always show modules deleted or type-changed (blob<->module)
> test $status = D -o $status = T && echo "$sm_path" && continue
> + # Respect the ignore setting for --for-status.
> + if test -n "$for_status"
> + then
> + name=$(module_name "$sm_path")
> + ignore_config=$(get_submodule_config "$name" ignore none)
> + test $status != A -a $ignore_config = all && continue
> + fi
> # Also show added or modified modules which are checked out
> GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
> echo "$sm_path"
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> index ac2434c..ca9441e 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -104,6 +104,36 @@ EOF
> test_cmp expected actual
> "
>
> +test_expect_success '.gitmodules ignore=all has no effect' "
> + git config --add -f .gitmodules submodule.sm1.ignore all &&
> + git config --add -f .gitmodules submodule.sm1.path sm1 &&
> + git submodule summary >actual &&
> + cat >expected <<-EOF &&
> +* sm1 $head1...$head2 (1):
> + > Add foo3
> +
> +EOF
> + test_cmp expected actual &&
> + git config -f .gitmodules --remove-section submodule.sm1
> +"
> +
> +test_expect_success '.git/config ignore=all has no effect' "
> + git config --add -f .gitmodules submodule.sm1.ignore none &&
> + git config --add -f .gitmodules submodule.sm1.path sm1 &&
> + git config --add submodule.sm1.ignore all &&
> + git config --add submodule.sm1.path sm1 &&
> + git submodule summary >actual &&
> + cat >expected <<-EOF &&
> +* sm1 $head1...$head2 (1):
> + > Add foo3
> +
> +EOF
> + test_cmp expected actual &&
> + git config --remove-section submodule.sm1 &&
> + git config -f .gitmodules --remove-section submodule.sm1
> +"
> +
> +
> commit_file sm1 &&
> head3=$(
> cd sm1 &&
I think we should do both tests in one go: just turn on all ignore
options and test against that. Additionally we should also set
diff.ignoreSubmodules too, while there is no need to set the path
in .git/config. And I believe we can drop the --add option from
the config command, so I'd propose something like this:
-------------------------8<---------------------------
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index ac2434c..81ae7c9 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -104,6 +104,24 @@ EOF
test_cmp expected actual
"
+test_expect_success 'no ignore=all setting has any effect' "
+ git config -f .gitmodules submodule.sm1.path sm1 &&
+ git config -f .gitmodules submodule.sm1.ignore all &&
+ git config submodule.sm1.ignore all &&
+ git config diff.ignoreSubmodules all &&
+ git submodule summary >actual &&
+ cat >expected <<-EOF &&
+* sm1 $head1...$head2 (1):
+ > Add foo3
+
+EOF
+ test_cmp expected actual &&
+ git config --unset diff.ignoreSubmodules &&
+ git config --remove-section submodule.sm1 &&
+ git config -f .gitmodules --remove-section submodule.sm1
+"
+
+
commit_file sm1 &&
head3=$(
cd sm1 &&
-------------------------8<---------------------------
> 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 &&
>
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-01 20:06 ` [PATCH v3 2/2] submodule: don't print status output with ignore=all brian m. carlson
2013-09-03 19:17 ` Jens Lehmann
@ 2013-09-03 19:53 ` Junio C Hamano
2013-09-04 20:01 ` Jens Lehmann
2013-09-04 6:31 ` Matthieu Moy
2 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2013-09-03 19:53 UTC (permalink / raw)
To: Jens.Lehmann; +Cc: git, jrnieder, judge.packham, brian m. carlson
Jens, I see 1/2 is the same as the previous one you already acked.
Does this update to 2/2 look good to you? Sorry, but I lost track
of the discussion that led to this reroll, hence a ping.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-01 20:06 ` [PATCH v3 2/2] submodule: don't print status output with ignore=all brian m. carlson
2013-09-03 19:17 ` Jens Lehmann
2013-09-03 19:53 ` Junio C Hamano
@ 2013-09-04 6:31 ` Matthieu Moy
2013-09-04 20:40 ` Jens Lehmann
2 siblings, 1 reply; 13+ messages in thread
From: Matthieu Moy @ 2013-09-04 6:31 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, jrnieder, Jens.Lehmann, judge.packham, gitster
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Tests are included which verify that this change has no effect on git submodule
> summary without the --for-status option.
I still don't understand why this is needed. Why do we want "git status"
and "git submodule summary" to display different information? Wasn't it
a nice property that the part of "git status" about submodule is the
same as "git submodule summary"?
This should at least be explained in the commit message IMHO.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-03 19:53 ` Junio C Hamano
@ 2013-09-04 20:01 ` Jens Lehmann
2013-09-04 20:57 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Jens Lehmann @ 2013-09-04 20:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jrnieder, judge.packham, brian m. carlson
Am 03.09.2013 21:53, schrieb Junio C Hamano:
> Jens, I see 1/2 is the same as the previous one you already acked.
Yep.
> Does this update to 2/2 look good to you? Sorry, but I lost track
> of the discussion that led to this reroll, hence a ping.
v3 fixes the bug Matthieu noticed, I only had some remarks to the
new test Brian added. If you could replace his patch to t7401 with
the following diff it's an ack from me on this one too.
-------------------------8<---------------------------
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index ac2434c..81ae7c9 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -104,6 +104,24 @@ EOF
test_cmp expected actual
"
+test_expect_success 'no ignore=all setting has any effect' "
+ git config -f .gitmodules submodule.sm1.path sm1 &&
+ git config -f .gitmodules submodule.sm1.ignore all &&
+ git config submodule.sm1.ignore all &&
+ git config diff.ignoreSubmodules all &&
+ git submodule summary >actual &&
+ cat >expected <<-EOF &&
+* sm1 $head1...$head2 (1):
+ > Add foo3
+
+EOF
+ test_cmp expected actual &&
+ git config --unset diff.ignoreSubmodules &&
+ git config --remove-section submodule.sm1 &&
+ git config -f .gitmodules --remove-section submodule.sm1
+"
+
+
commit_file sm1 &&
head3=$(
cd sm1 &&
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-04 6:31 ` Matthieu Moy
@ 2013-09-04 20:40 ` Jens Lehmann
2013-09-05 6:30 ` Matthieu Moy
0 siblings, 1 reply; 13+ messages in thread
From: Jens Lehmann @ 2013-09-04 20:40 UTC (permalink / raw)
To: Matthieu Moy; +Cc: brian m. carlson, git, jrnieder, judge.packham, gitster
Am 04.09.2013 08:31, schrieb Matthieu Moy:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> Tests are included which verify that this change has no effect on git submodule
>> summary without the --for-status option.
>
> I still don't understand why this is needed.
To avoid a change in behavior for "git submodule summary", as that
never honored the submodule.*.ignore nor the diff.ignoreSubmodules
setting (and I don't think it ever should).
> Why do we want "git status"
> and "git submodule summary" to display different information? Wasn't it
> a nice property that the part of "git status" about submodule is the
> same as "git submodule summary"?
I changed that in 2010 (1.7.3) to make the output of status consistent,
meaning that submodules that didn't show up in the regular status output
don't appear in the summary part either. And I still believe it is the
right thing to do for the target audience of the ignore settings, as
they want to hide any changes in submodules that are either expensive to
traverse or uninteresting for the developer until they explicitly use
the submodule commands, which still behave as they always did (I might
be wrong here as I'm not in that group myself, but so far no one spoke
up).
> This should at least be explained in the commit message IMHO.
Fine by me, what would you propose to clarify that? (Though I have the
suspicion that the explanation will be three years late ;-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-04 20:01 ` Jens Lehmann
@ 2013-09-04 20:57 ` Junio C Hamano
2013-09-04 21:26 ` Jens Lehmann
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2013-09-04 20:57 UTC (permalink / raw)
To: Jens Lehmann; +Cc: git, jrnieder, judge.packham, brian m. carlson
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Am 03.09.2013 21:53, schrieb Junio C Hamano:
>> Jens, I see 1/2 is the same as the previous one you already acked.
>
> Yep.
>
>> Does this update to 2/2 look good to you? Sorry, but I lost track
>> of the discussion that led to this reroll, hence a ping.
>
> v3 fixes the bug Matthieu noticed, I only had some remarks to the
> new test Brian added. If you could replace his patch to t7401 with
> the following diff it's an ack from me on this one too.
>
> -------------------------8<---------------------------
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> index ac2434c..81ae7c9 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -104,6 +104,24 @@ EOF
> test_cmp expected actual
> "
>
> +test_expect_success 'no ignore=all setting has any effect' "
> + git config -f .gitmodules submodule.sm1.path sm1 &&
> + git config -f .gitmodules submodule.sm1.ignore all &&
> + git config submodule.sm1.ignore all &&
> + git config diff.ignoreSubmodules all &&
> + git submodule summary >actual &&
> + cat >expected <<-EOF &&
> +* sm1 $head1...$head2 (1):
> + > Add foo3
> +
> +EOF
> + test_cmp expected actual &&
> + git config --unset diff.ignoreSubmodules &&
> + git config --remove-section submodule.sm1 &&
> + git config -f .gitmodules --remove-section submodule.sm1
> +"
> +
> +
> commit_file sm1 &&
> head3=$(
> cd sm1 &&
Thanks.
The above patch makes the <<-EOF situation that already exists in
this script worse. The only reason we would say -EOF not EOF is
because we would want to indent the here-document to align with the
rest of the command sequence, so we should either indent with HT, or
drop the dash. I suspect the original did it that way fearing that
someday the indentation of the submodule difference list might start
using HT, but I do not think that is likely to happen, so my vote
goes to keeping '-' and indenting.
We need a clean-up patch after this series settles.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-04 20:57 ` Junio C Hamano
@ 2013-09-04 21:26 ` Jens Lehmann
0 siblings, 0 replies; 13+ messages in thread
From: Jens Lehmann @ 2013-09-04 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jrnieder, judge.packham, brian m. carlson
Am 04.09.2013 22:57, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Am 03.09.2013 21:53, schrieb Junio C Hamano:
>>> Does this update to 2/2 look good to you? Sorry, but I lost track
>>> of the discussion that led to this reroll, hence a ping.
>>
>> v3 fixes the bug Matthieu noticed, I only had some remarks to the
>> new test Brian added. If you could replace his patch to t7401 with
>> the following diff it's an ack from me on this one too.
>>
>> -------------------------8<---------------------------
>> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
>> index ac2434c..81ae7c9 100755
>> --- a/t/t7401-submodule-summary.sh
>> +++ b/t/t7401-submodule-summary.sh
>> @@ -104,6 +104,24 @@ EOF
>> test_cmp expected actual
>> "
>>
>> +test_expect_success 'no ignore=all setting has any effect' "
>> + git config -f .gitmodules submodule.sm1.path sm1 &&
>> + git config -f .gitmodules submodule.sm1.ignore all &&
>> + git config submodule.sm1.ignore all &&
>> + git config diff.ignoreSubmodules all &&
>> + git submodule summary >actual &&
>> + cat >expected <<-EOF &&
>> +* sm1 $head1...$head2 (1):
>> + > Add foo3
>> +
>> +EOF
>> + test_cmp expected actual &&
>> + git config --unset diff.ignoreSubmodules &&
>> + git config --remove-section submodule.sm1 &&
>> + git config -f .gitmodules --remove-section submodule.sm1
>> +"
>> +
>> +
>> commit_file sm1 &&
>> head3=$(
>> cd sm1 &&
>
> Thanks.
>
> The above patch makes the <<-EOF situation that already exists in
> this script worse. The only reason we would say -EOF not EOF is
> because we would want to indent the here-document to align with the
> rest of the command sequence, so we should either indent with HT, or
> drop the dash. I suspect the original did it that way fearing that
> someday the indentation of the submodule difference list might start
> using HT, but I do not think that is likely to happen, so my vote
> goes to keeping '-' and indenting.
>
> We need a clean-up patch after this series settles.
Ok, will do (unless someone else volunteers ;-).
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-04 20:40 ` Jens Lehmann
@ 2013-09-05 6:30 ` Matthieu Moy
2013-09-05 8:05 ` Matthieu Moy
2013-09-06 0:19 ` brian m. carlson
0 siblings, 2 replies; 13+ messages in thread
From: Matthieu Moy @ 2013-09-05 6:30 UTC (permalink / raw)
To: Jens Lehmann; +Cc: brian m. carlson, git, jrnieder, judge.packham, gitster
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Am 04.09.2013 08:31, schrieb Matthieu Moy:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>>> Tests are included which verify that this change has no effect on git submodule
>>> summary without the --for-status option.
>>
>> I still don't understand why this is needed.
>
> To avoid a change in behavior for "git submodule summary", as that
> never honored the submodule.*.ignore nor the diff.ignoreSubmodules
> setting (and I don't think it ever should).
I don't get it. If the goal is to keep the old behavior, then "git
status" shouldn't be changed either. Fixing bugs needs to change the
behavior.
IOW, why was it a bug that "git status" showed ignored submodules and
not a bug that "git submodule summary" did the same?
> Fine by me, what would you propose to clarify that? (Though I have the
> suspicion that the explanation will be three years late ;-)
I have no idea, as I do not understand the reason myself yet. I'm not a
frequent user of submodules and not a user of the ignore option at all,
so I can't tell what's best. I'd just like the new behavior to be
justified somewhere.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-05 6:30 ` Matthieu Moy
@ 2013-09-05 8:05 ` Matthieu Moy
2013-09-06 0:19 ` brian m. carlson
1 sibling, 0 replies; 13+ messages in thread
From: Matthieu Moy @ 2013-09-05 8:05 UTC (permalink / raw)
To: Jens Lehmann; +Cc: brian m. carlson, git, jrnieder, judge.packham, gitster
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Fine by me, what would you propose to clarify that? (Though I have the
>> suspicion that the explanation will be three years late ;-)
>
> I have no idea, as I do not understand the reason myself yet. I'm not a
> frequent user of submodules and not a user of the ignore option at all,
> so I can't tell what's best. I'd just like the new behavior to be
> justified somewhere.
I also think that the documentation in Documentation/config.txt could be
updated. Perhaps adding something like
To view the summary for submodules ignored by 'git status', one
can use 'git submodules summary' which shows a similar output
but does not honor this option.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/2] submodule: don't print status output with ignore=all
2013-09-05 6:30 ` Matthieu Moy
2013-09-05 8:05 ` Matthieu Moy
@ 2013-09-06 0:19 ` brian m. carlson
1 sibling, 0 replies; 13+ messages in thread
From: brian m. carlson @ 2013-09-06 0:19 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Jens Lehmann, git, jrnieder, judge.packham, gitster
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]
On Thu, Sep 05, 2013 at 08:30:53AM +0200, Matthieu Moy wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
> > Am 04.09.2013 08:31, schrieb Matthieu Moy:
> >> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >>
> >>> Tests are included which verify that this change has no effect on git submodule
> >>> summary without the --for-status option.
> >>
> >> I still don't understand why this is needed.
> >
> > To avoid a change in behavior for "git submodule summary", as that
> > never honored the submodule.*.ignore nor the diff.ignoreSubmodules
> > setting (and I don't think it ever should).
>
> I don't get it. If the goal is to keep the old behavior, then "git
> status" shouldn't be changed either. Fixing bugs needs to change the
> behavior.
>
> IOW, why was it a bug that "git status" showed ignored submodules and
> not a bug that "git submodule summary" did the same?
I looked at the tests, and the tests expected git status to respect the
ignore settings, but there were no tests that expected git submodule
summary to work this way. Perhaps this is a failure in the tests. I
got feedback that git submodule summary should *not* change, and nothing
until now to contradict that.
If the decision is that this should change both, then fine, I'll reroll.
But it's not reasonable to keep going back and forth: we should make a
decision and stick to it.
--
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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-09-06 0:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-01 20:06 [PATCH v3 0/2] submodule: Don't print status output with submodule.<name>.ignore=all brian m. carlson
2013-09-01 20:06 ` [PATCH v3 1/2] submodule: fix confusing variable name brian m. carlson
2013-09-01 20:06 ` [PATCH v3 2/2] submodule: don't print status output with ignore=all brian m. carlson
2013-09-03 19:17 ` Jens Lehmann
2013-09-03 19:53 ` Junio C Hamano
2013-09-04 20:01 ` Jens Lehmann
2013-09-04 20:57 ` Junio C Hamano
2013-09-04 21:26 ` Jens Lehmann
2013-09-04 6:31 ` Matthieu Moy
2013-09-04 20:40 ` Jens Lehmann
2013-09-05 6:30 ` Matthieu Moy
2013-09-05 8:05 ` Matthieu Moy
2013-09-06 0:19 ` brian m. carlson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).