* [PATCHv4 1/6] submodule foreach: correct path display in recursive submodules
2016-03-30 1:27 [PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir Stefan Beller
@ 2016-03-30 1:27 ` Stefan Beller
2016-03-30 20:05 ` Junio C Hamano
2016-03-30 1:27 ` [PATCHv4 2/6] submodule update --init: correct path handling " Stefan Beller
` (4 subsequent siblings)
5 siblings, 1 reply; 8+ messages in thread
From: Stefan Beller @ 2016-03-30 1:27 UTC (permalink / raw)
To: gitster; +Cc: git, jacob.keller, Stefan Beller
The `prefix` was put in front of the display path unconditionally.
This is wrong as any relative path computation would need to be at
the front, so include the prefix into the display path.
The new test replicates the previous test with the difference of executing
from a sub directory. By executing from a sub directory all we would
expect all displayed paths to be prefixed by '../'.
Prior to this patch the test would report
Entering 'nested1/nested2/../nested3'
instead of the expected
Entering '../nested1/nested2/nested3'
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
* reworded commit message
* when writing the commit message I discovered a new way to fix the bug
(fix the computation of the displaypath instead of its parameters
wt_prefix and prefix.)
The result is the same, but I am not yet sure if I like it more.
git-submodule.sh | 6 +++---
t/t7407-submodule-foreach.sh | 20 ++++++++++++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..b3f248c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -413,8 +413,8 @@ cmd_foreach()
die_if_unmatched "$mode"
if test -e "$sm_path"/.git
then
- displaypath=$(relative_path "$sm_path")
- say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
+ displaypath=$(relative_path "$prefix$sm_path")
+ say "$(eval_gettext "Entering '\$displaypath'")"
name=$(git submodule--helper name "$sm_path")
(
prefix="$prefix$sm_path/"
@@ -434,7 +434,7 @@ cmd_foreach()
cmd_foreach "--recursive" "$@"
fi
) <&3 3<&- ||
- die "$(eval_gettext "Stopping at '\$prefix\$displaypath'; script returned non-zero status.")"
+ die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
fi
done
}
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 7ca10b8..776b349 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -178,6 +178,26 @@ test_expect_success 'test messages from "foreach --recursive"' '
'
cat > expect <<EOF
+Entering '../nested1'
+Entering '../nested1/nested2'
+Entering '../nested1/nested2/nested3'
+Entering '../nested1/nested2/nested3/submodule'
+Entering '../sub1'
+Entering '../sub2'
+Entering '../sub3'
+EOF
+
+test_expect_success 'test messages from "foreach --recursive" from subdirectory' '
+ (
+ cd clone2 &&
+ mkdir untracked &&
+ cd untracked &&
+ git submodule foreach --recursive >../../actual
+ ) &&
+ test_i18ncmp expect actual
+'
+
+cat > expect <<EOF
nested1-nested1
nested2-nested2
nested3-nested3
--
2.8.0.2.gb331331
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCHv4 1/6] submodule foreach: correct path display in recursive submodules
2016-03-30 1:27 ` [PATCHv4 1/6] submodule foreach: correct path display in recursive submodules Stefan Beller
@ 2016-03-30 20:05 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2016-03-30 20:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, jacob.keller
Stefan Beller <sbeller@google.com> writes:
> The `prefix` was put in front of the display path unconditionally.
> This is wrong as any relative path computation would need to be at
> the front, so include the prefix into the display path.
>
> The new test replicates the previous test with the difference of executing
> from a sub directory. By executing from a sub directory all we would
> expect all displayed paths to be prefixed by '../'.
>
> Prior to this patch the test would report
> Entering 'nested1/nested2/../nested3'
> instead of the expected
> Entering '../nested1/nested2/nested3'
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Definitely easier to read and helps understand what is going on in
the changed codepath much better. Very well done.
Thanks.
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 7ca10b8..776b349 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -178,6 +178,26 @@ test_expect_success 'test messages from "foreach --recursive"' '
> '
>
> cat > expect <<EOF
> +Entering '../nested1'
> +Entering '../nested1/nested2'
> +Entering '../nested1/nested2/nested3'
> +Entering '../nested1/nested2/nested3/submodule'
> +Entering '../sub1'
> +Entering '../sub2'
> +Entering '../sub3'
> +EOF
> +
> +test_expect_success 'test messages from "foreach --recursive" from subdirectory' '
> + (
> + cd clone2 &&
> + mkdir untracked &&
> + cd untracked &&
> + git submodule foreach --recursive >../../actual
> + ) &&
> + test_i18ncmp expect actual
> +'
> +
> +cat > expect <<EOF
> nested1-nested1
> nested2-nested2
> nested3-nested3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv4 2/6] submodule update --init: correct path handling in recursive submodules
2016-03-30 1:27 [PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir Stefan Beller
2016-03-30 1:27 ` [PATCHv4 1/6] submodule foreach: correct path display in recursive submodules Stefan Beller
@ 2016-03-30 1:27 ` Stefan Beller
2016-03-30 1:27 ` [PATCHv4 3/6] submodule status: " Stefan Beller
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2016-03-30 1:27 UTC (permalink / raw)
To: gitster; +Cc: git, jacob.keller, Stefan Beller
When calling `git submodule init` from a recursive instance of
`git submodule update --recursive`, the reported path is wrong as it
skips the nested submodules.
The new test demonstrates a failure in the code prior to this patch.
Instead of getting the expected
Submodule 'submodule' (${pwd}/submodule) registered for path '../super/submodule'
the `super` directory is omitted and you get
Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
instead.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
* Use `git -C <path>` instead of `cd <path> && git`.
* Reword commit message
git-submodule.sh | 2 +-
t/t7406-submodule-update.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index b3f248c..9fffa5c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -473,7 +473,7 @@ cmd_init()
die_if_unmatched "$mode"
name=$(git submodule--helper name "$sm_path") || exit
- displaypath=$(relative_path "$sm_path")
+ displaypath=$(relative_path "$prefix$sm_path")
# Copy url setting when it is not set yet
if test -z "$(git config "submodule.$name.url")"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..d4745f4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
git submodule add ../none none &&
test_tick &&
git commit -m "none"
+ ) &&
+ git clone . recursivesuper &&
+ ( cd recursivesuper
+ git submodule add ../super super
)
'
@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
)
'
+supersha1=$(git -C super rev-parse HEAD)
+mergingsha1=$(git -C super/merging rev-parse HEAD)
+nonesha1=$(git -C super/none rev-parse HEAD)
+rebasingsha1=$(git -C super/rebasing rev-parse HEAD)
+submodulesha1=$(git -C super/submodule rev-parse HEAD)
+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
+
+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
+ ) &&
+ test_cmp expect actual
+'
+
apos="'";
test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
--
2.8.0.2.gb331331
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCHv4 3/6] submodule status: correct path handling in recursive submodules
2016-03-30 1:27 [PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir Stefan Beller
2016-03-30 1:27 ` [PATCHv4 1/6] submodule foreach: correct path display in recursive submodules Stefan Beller
2016-03-30 1:27 ` [PATCHv4 2/6] submodule update --init: correct path handling " Stefan Beller
@ 2016-03-30 1:27 ` Stefan Beller
2016-03-30 1:27 ` [PATCHv4 4/6] submodule update: align reporting path for custom command execution Stefan Beller
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2016-03-30 1:27 UTC (permalink / raw)
To: gitster; +Cc: git, jacob.keller, Stefan Beller
The new test which is a replica of the previous test except
that it executes from a sub directory. Prior to this patch
the test failed by having too many '../' prefixed:
--- expect 2016-03-29 19:02:33.087336115 +0000
+++ actual 2016-03-29 19:02:33.359343311 +0000
@@ -1,7 +1,7 @@
b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
-+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
- 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 (heads/master)
- 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../nested1/nested2/nested3/submodule (heads/master)
++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
+ 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 (heads/master)
+ 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../../../../nested1/nested2/nested3/submodule (heads/master)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)
The path code in question:
displaypath=$(relative_path "$prefix$sm_path")
prefix=$displaypath
if recursive:
eval cmd_status
That way we change `prefix` each iteration to contain another
'../', because of the the relative_path computation is done
on an already computed relative path.
We must call relative_path exactly once with `wt_prefix` non empty.
Further calls in recursive instances to to calculate the displaypath
already incorporate the correct prefix from before. Fix the issue by
clearing `wt_prefix` in recursive calls.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
git-submodule.sh | 1 +
t/t7407-submodule-foreach.sh | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index 9fffa5c..1024774 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1159,6 +1159,7 @@ cmd_status()
(
prefix="$displaypath/"
clear_local_git_env
+ wt_prefix=
cd "$sm_path" &&
eval cmd_status
) ||
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 776b349..4b35e12 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached --recursive" preserves the --cached
test_cmp expect actual
'
+nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
+
+cat > expect <<EOF
+ $nested1sha1 ../nested1 (heads/master)
++$nested2sha1 ../nested1/nested2 (file2)
+ $nested3sha1 ../nested1/nested2/nested3 (heads/master)
+ $submodulesha1 ../nested1/nested2/nested3/submodule (heads/master)
+ $sub1sha1 ../sub1 ($sub1sha1_short)
+ $sub2sha1 ../sub2 ($sub2sha1_short)
+ $sub3sha1 ../sub3 (heads/master)
+EOF
+
+test_expect_success 'test "status --recursive" from sub directory' '
+ (
+ cd clone3 &&
+ mkdir tmp && cd tmp &&
+ git submodule status --recursive > ../../actual
+ ) &&
+ test_cmp expect actual
+'
+
test_expect_success 'use "git clone --recursive" to checkout all submodules' '
git clone --recursive super clone4 &&
(
--
2.8.0.2.gb331331
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCHv4 4/6] submodule update: align reporting path for custom command execution
2016-03-30 1:27 [PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir Stefan Beller
` (2 preceding siblings ...)
2016-03-30 1:27 ` [PATCHv4 3/6] submodule status: " Stefan Beller
@ 2016-03-30 1:27 ` Stefan Beller
2016-03-30 1:27 ` [PATCHv4 5/6] submodule update: test recursive path reporting from subdirectory Stefan Beller
2016-03-30 1:27 ` [PATCHv4 6/6] t7407: make expectation as clear as possible Stefan Beller
5 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2016-03-30 1:27 UTC (permalink / raw)
To: gitster; +Cc: git, jacob.keller, Stefan Beller
In the predefined actions (merge, rebase, none, checkout), we use
the display path, which is relative to the current working directory.
Also use the display path when running a custom command.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
git-submodule.sh | 4 ++--
t/t7406-submodule-update.sh | 29 ++++++++++++++++++++++++++---
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 1024774..753a90d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -802,8 +802,8 @@ Maybe you want to use 'update --init'?")"
;;
!*)
command="${update_module#!}"
- die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
- say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': '\$command \$sha1'")"
+ die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"
+ say_msg="$(eval_gettext "Submodule path '\$displaypath': '\$command \$sha1'")"
must_die_on_failure=yes
;;
*)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index d4745f4..01dd324 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in .git/config' '
)
'
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 'submodule'
+EOF
+
test_expect_success 'submodule update - command in .git/config catches failure' '
(cd super &&
git config submodule.submodule.update "!false"
) &&
(cd super/submodule &&
- git reset --hard HEAD^
+ git reset --hard $submodulesha1^
) &&
(cd super &&
- test_must_fail git submodule update submodule
- )
+ test_must_fail git submodule update submodule 2>../actual
+ ) &&
+ test_cmp actual expect
+'
+
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../submodule'
+EOF
+
+test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '
+ (cd super &&
+ git config submodule.submodule.update "!false"
+ ) &&
+ (cd super/submodule &&
+ git reset --hard $submodulesha1^
+ ) &&
+ (cd super &&
+ mkdir tmp && cd tmp &&
+ test_must_fail git submodule update ../submodule 2>../../actual
+ ) &&
+ test_cmp actual expect
'
test_expect_success 'submodule init does not copy command into .git/config' '
--
2.8.0.2.gb331331
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCHv4 5/6] submodule update: test recursive path reporting from subdirectory
2016-03-30 1:27 [PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir Stefan Beller
` (3 preceding siblings ...)
2016-03-30 1:27 ` [PATCHv4 4/6] submodule update: align reporting path for custom command execution Stefan Beller
@ 2016-03-30 1:27 ` Stefan Beller
2016-03-30 1:27 ` [PATCHv4 6/6] t7407: make expectation as clear as possible Stefan Beller
5 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2016-03-30 1:27 UTC (permalink / raw)
To: gitster; +Cc: git, jacob.keller, Stefan Beller
This patch is just a test and fixes no bug as there is currently no bug
in the path handling of `submodule update`.
In `submodule update` we make a call to `submodule--helper list --prefix
"$wt_prefix"` which looks a bit brittle and likely to introduce a bug
for the path handling. It is not a bug as the prefix is ignored inside
the submodule helper for now. If this test breaks eventually, we want
to make sure the `wt_prefix` is passed correctly into recursive submodules.
Hint: In recursive submodules we expect `wt_prefix` to be empty.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7406-submodule-update.sh | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 01dd324..e5af4b4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -379,6 +379,26 @@ test_expect_success 'submodule update - command in .git/config catches failure -
test_cmp actual expect
'
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'
+Failed to recurse into submodule path '../super'
+EOF
+
+test_expect_success 'recursive submodule update - command in .git/config catches failure -- subdirectory' '
+ (cd recursivesuper &&
+ git submodule update --remote super &&
+ git add super &&
+ git commit -m "update to latest to have more than one commit in submodules"
+ ) &&
+ git -C recursivesuper/super config submodule.submodule.update "!false" &&
+ git -C recursivesuper/super/submodule reset --hard $submodulesha1^ &&
+ (cd recursivesuper &&
+ mkdir -p tmp && cd tmp &&
+ test_must_fail git submodule update --recursive ../super 2>../../actual
+ ) &&
+ test_cmp actual expect
+'
+
test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
H=$(git ls-files -s submodule | cut -d" " -f2) &&
--
2.8.0.2.gb331331
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCHv4 6/6] t7407: make expectation as clear as possible
2016-03-30 1:27 [PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir Stefan Beller
` (4 preceding siblings ...)
2016-03-30 1:27 ` [PATCHv4 5/6] submodule update: test recursive path reporting from subdirectory Stefan Beller
@ 2016-03-30 1:27 ` Stefan Beller
5 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2016-03-30 1:27 UTC (permalink / raw)
To: gitster; +Cc: git, jacob.keller, Stefan Beller
Not everyone (including me) grasps the sed expression in a split second as
they would grasp the 4 lines printed as is.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7407-submodule-foreach.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 4b35e12..6ba5daf 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -262,8 +262,12 @@ test_expect_success 'test "status --recursive"' '
test_cmp expect actual
'
-sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
-mv -f expect2 expect
+cat > expect <<EOF
+ $nested1sha1 nested1 (heads/master)
++$nested2sha1 nested1/nested2 (file2~1)
+ $nested3sha1 nested1/nested2/nested3 (heads/master)
+ $submodulesha1 nested1/nested2/nested3/submodule (heads/master)
+EOF
test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
(
--
2.8.0.2.gb331331
^ permalink raw reply related [flat|nested] 8+ messages in thread