* [PATCH] submodule status: properly pass options with --recursive
@ 2012-10-25 22:20 Jens Lehmann
2012-10-26 13:15 ` Jeff King
0 siblings, 1 reply; 12+ messages in thread
From: Jens Lehmann @ 2012-10-25 22:20 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano, Jeff King
When renaming orig_args to orig_flags in 98dbe63d (submodule: only
preserve flags across recursive status/update invocations) the call site
of the recursive cmd_status was forgotten. At that place orig_args is
still passed into the recursion, which is always empty now. This clears
all options when recursing, as that variable is never set.
Fix that by renaming orig_args to orig_flags there too and add a test to
catch that bug.
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
I noticed that when reviewing Phil's "Teach --recursive to submodule
sync" patch.
git-submodule.sh | 2 +-
t/t7407-submodule-foreach.sh | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..c089d48 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -990,7 +990,7 @@ cmd_status()
prefix="$displaypath/"
clear_local_git_env
cd "$sm_path" &&
- eval cmd_status "$orig_args"
+ eval cmd_status "$orig_flags"
) ||
die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
fi
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..eca36b5 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -245,6 +245,14 @@ test_expect_success 'ensure "status --cached --recursive" preserves the --cached
test_cmp expect actual
'
+test_expect_success 'ensure "status --quiet --recursive" preserves the --quiet flag' '
+ (
+ cd clone3 &&
+ git submodule status --quiet --recursive -- nested1 > ../actual
+ ) &&
+ ! test -s actual
+'
+
test_expect_success 'use "git clone --recursive" to checkout all submodules' '
git clone --recursive super clone4 &&
(
--
1.8.0.dirty
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] submodule status: properly pass options with --recursive
2012-10-25 22:20 [PATCH] submodule status: properly pass options with --recursive Jens Lehmann
@ 2012-10-26 13:15 ` Jeff King
2012-10-26 19:07 ` Phil Hord
0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2012-10-26 13:15 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Git Mailing List, Junio C Hamano
On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:
> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
> preserve flags across recursive status/update invocations) the call site
> of the recursive cmd_status was forgotten. At that place orig_args is
> still passed into the recursion, which is always empty now. This clears
> all options when recursing, as that variable is never set.
>
> Fix that by renaming orig_args to orig_flags there too and add a test to
> catch that bug.
Thanks. I back-ported your patch on top of 98dbe63d so it can go to
'maint'. I'm curious, though: why didn't the test right before (which
checks recursion for --cached) catch this?
-Peff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] submodule status: properly pass options with --recursive
2012-10-26 13:15 ` Jeff King
@ 2012-10-26 19:07 ` Phil Hord
2012-10-26 19:13 ` [PATCH] t7407: Fix recursive submodule test Phil Hord
2012-10-26 19:26 ` [PATCH] submodule status: properly pass options with --recursive Jens Lehmann
0 siblings, 2 replies; 12+ messages in thread
From: Phil Hord @ 2012-10-26 19:07 UTC (permalink / raw)
To: Jeff King; +Cc: Jens Lehmann, Git Mailing List, Junio C Hamano
On Fri, Oct 26, 2012 at 9:15 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:
>
>> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
>> preserve flags across recursive status/update invocations) the call site
>> of the recursive cmd_status was forgotten. At that place orig_args is
>> still passed into the recursion, which is always empty now. This clears
>> all options when recursing, as that variable is never set.
>>
>> Fix that by renaming orig_args to orig_flags there too and add a test to
>> catch that bug.
>
> Thanks. I back-ported your patch on top of 98dbe63d so it can go to
> 'maint'. I'm curious, though: why didn't the test right before (which
> checks recursion for --cached) catch this?
I was wondering the same thing about why 'git submodule sync
--recursive --quiet' succeeded, so I checked on it. The answer is
that "--quiet" sets GIT_QUIET=1, which is then inherited by the
recursive call. Indeed, Jens' new test passes even without the
accompanying fix. :-\
The 'status --recursive --cached' test passes for two reasons. The
first is that the test is checking for a cached change in a level-1
submodule, but it is the level-2+ submodules which do not get the
flags passed down correctly in "$@". The 2nd reason is that the
$cached variable is _also_ inherited by the recursive call to
cmd_status, specifically because it is a call to cmd_status() and not
to '$SHELL git submodule status', where the latter would have cleared
the flags at startup, but the former does not.
I'm a bit wary about "fixing" the flags for the recursion machinery.
I'm starting to think $orig_flags is moot in almost all cases. It
looks like clearing all the flags on each iteration would break 'git
submodule foreach --recursive --quiet' because it does not use
$orig_flags at all. Who knows what other surprises lurk there.
Here's a fix for the test, though, even though it still does not fail
in the absence of the $orig_flags patch.
-- >8 --
diff --git i/t/t7407-submodule-foreach.sh w/t/t7407-submodule-foreach.sh
index 9b69fe2..8442b32 100755
--- i/t/t7407-submodule-foreach.sh
+++ w/t/t7407-submodule-foreach.sh
@@ -226,14 +226,14 @@ test_expect_success 'test "status --recursive"' '
test_cmp expect actual
'
-sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d"
< expect > expect2
+sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2
(file2~1)/;/sub[1-3]/d" < expect > expect2
mv -f expect2 expect
test_expect_success 'ensure "status --cached --recursive" preserves
the --cached flag' '
(
cd clone3 &&
(
- cd nested1 &&
+ cd nested1/nested2 &&
test_commit file2
) &&
git submodule status --cached --recursive -- nested1 > ../actual
@@ -245,6 +245,14 @@ test_expect_success 'ensure "status --cached
--recursive" preserves the --cached
test_cmp expect actual
'
test_expect_success 'use "git clone --recursive" to checkout all submodules' '
git clone --recursive super clone4 &&
(
Phil
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] t7407: Fix recursive submodule test
2012-10-26 19:07 ` Phil Hord
@ 2012-10-26 19:13 ` Phil Hord
2012-10-26 19:29 ` Jens Lehmann
2012-10-26 19:26 ` [PATCH] submodule status: properly pass options with --recursive Jens Lehmann
1 sibling, 1 reply; 12+ messages in thread
From: Phil Hord @ 2012-10-26 19:13 UTC (permalink / raw)
To: Git Mailing List; +Cc: phil.hord, Jens Lehmann, Jeff King, Phil Hord
A test in t7404-submodule-foreach purports to test that
the --cached flag is properly noticed by --recursive calls
to the foreach command as it descends into nested
submodules. However, the test really does not perform this
test since the change it looks for is in a top-level
submodule handled by the first invocation of the command.
To properly test for the flag being passed to recursive
invocations, the change must be buried deeper in the
hierarchy.
Move the change one level deeper so it properly verifies
the recursive machinery of the 'git submodule status'
command.
Signed-off-by: Phil Hord <hordp@cisco.com>
---
t/t7407-submodule-foreach.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..107b4b7 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -226,14 +226,14 @@ test_expect_success 'test "status --recursive"' '
test_cmp expect actual
'
-sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d" < expect > expect2
+sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
mv -f expect2 expect
test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
(
cd clone3 &&
(
- cd nested1 &&
+ cd nested1/nested2 &&
test_commit file2
) &&
git submodule status --cached --recursive -- nested1 > ../actual
--
1.8.0.2.gd008466.dirty
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] submodule status: properly pass options with --recursive
2012-10-26 19:07 ` Phil Hord
2012-10-26 19:13 ` [PATCH] t7407: Fix recursive submodule test Phil Hord
@ 2012-10-26 19:26 ` Jens Lehmann
2012-10-26 19:44 ` [PATCHv3 0/2] Teach --recursive to submodule sync Phil Hord
1 sibling, 1 reply; 12+ messages in thread
From: Jens Lehmann @ 2012-10-26 19:26 UTC (permalink / raw)
To: Phil Hord; +Cc: Jeff King, Git Mailing List, Junio C Hamano
Am 26.10.2012 21:07, schrieb Phil Hord:
> On Fri, Oct 26, 2012 at 9:15 AM, Jeff King <peff@peff.net> wrote:
>> On Fri, Oct 26, 2012 at 12:20:29AM +0200, Jens Lehmann wrote:
>>
>>> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
>>> preserve flags across recursive status/update invocations) the call site
>>> of the recursive cmd_status was forgotten. At that place orig_args is
>>> still passed into the recursion, which is always empty now. This clears
>>> all options when recursing, as that variable is never set.
>>>
>>> Fix that by renaming orig_args to orig_flags there too and add a test to
>>> catch that bug.
>>
>> Thanks. I back-ported your patch on top of 98dbe63d so it can go to
>> 'maint'. I'm curious, though: why didn't the test right before (which
>> checks recursion for --cached) catch this?
>
> I was wondering the same thing about why 'git submodule sync
> --recursive --quiet' succeeded, so I checked on it. The answer is
> that "--quiet" sets GIT_QUIET=1, which is then inherited by the
> recursive call. Indeed, Jens' new test passes even without the
> accompanying fix. :-\
Dang, you're right! At least that explains why nobody noticed that
so far ... (and that's what you get for skipping the "does the test
fail without your fix?" part because the fix is so obvious :-( )
> The 'status --recursive --cached' test passes for two reasons. The
> first is that the test is checking for a cached change in a level-1
> submodule, but it is the level-2+ submodules which do not get the
> flags passed down correctly in "$@". The 2nd reason is that the
> $cached variable is _also_ inherited by the recursive call to
> cmd_status, specifically because it is a call to cmd_status() and not
> to '$SHELL git submodule status', where the latter would have cleared
> the flags at startup, but the former does not.
>
> I'm a bit wary about "fixing" the flags for the recursion machinery.
> I'm starting to think $orig_flags is moot in almost all cases. It
> looks like clearing all the flags on each iteration would break 'git
> submodule foreach --recursive --quiet' because it does not use
> $orig_flags at all. Who knows what other surprises lurk there.
I agree, it looks like ripping out the orig_flags would be the way
to go.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] t7407: Fix recursive submodule test
2012-10-26 19:13 ` [PATCH] t7407: Fix recursive submodule test Phil Hord
@ 2012-10-26 19:29 ` Jens Lehmann
2012-10-28 21:37 ` [PATCH] submodule status: remove unused orig_* variables Jens Lehmann
0 siblings, 1 reply; 12+ messages in thread
From: Jens Lehmann @ 2012-10-26 19:29 UTC (permalink / raw)
To: Phil Hord; +Cc: Git Mailing List, phil.hord, Jeff King
Am 26.10.2012 21:13, schrieb Phil Hord:
> A test in t7404-submodule-foreach purports to test that
> the --cached flag is properly noticed by --recursive calls
> to the foreach command as it descends into nested
> submodules. However, the test really does not perform this
> test since the change it looks for is in a top-level
> submodule handled by the first invocation of the command.
> To properly test for the flag being passed to recursive
> invocations, the change must be buried deeper in the
> hierarchy.
>
> Move the change one level deeper so it properly verifies
> the recursive machinery of the 'git submodule status'
> command.
Me thinks we should definitely do this.
> Signed-off-by: Phil Hord <hordp@cisco.com>
> ---
> t/t7407-submodule-foreach.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 9b69fe2..107b4b7 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -226,14 +226,14 @@ test_expect_success 'test "status --recursive"' '
> test_cmp expect actual
> '
>
> -sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d" < expect > expect2
> +sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
> mv -f expect2 expect
>
> test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
> (
> cd clone3 &&
> (
> - cd nested1 &&
> + cd nested1/nested2 &&
> test_commit file2
> ) &&
> git submodule status --cached --recursive -- nested1 > ../actual
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3 0/2] Teach --recursive to submodule sync
2012-10-26 19:26 ` [PATCH] submodule status: properly pass options with --recursive Jens Lehmann
@ 2012-10-26 19:44 ` Phil Hord
2012-10-26 19:44 ` [PATCHv3 1/2] " Phil Hord
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Phil Hord @ 2012-10-26 19:44 UTC (permalink / raw)
To: git; +Cc: phil.hord, Jens Lehmann, Jeff King
[PATCHv3 1/2] Teach --recursive to submodule sync
Now with less noise and no redundant flags passed to the recursive call.
[PATCHv3 2/2] Add tests for submodule sync --recursive
The test remains unchanged.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv3 1/2] Teach --recursive to submodule sync
2012-10-26 19:44 ` [PATCHv3 0/2] Teach --recursive to submodule sync Phil Hord
@ 2012-10-26 19:44 ` Phil Hord
2012-10-26 19:44 ` [PATCHv3 2/2] Add tests for submodule sync --recursive Phil Hord
2012-10-28 21:02 ` [PATCHv3 0/2] Teach --recursive to submodule sync Jens Lehmann
2 siblings, 0 replies; 12+ messages in thread
From: Phil Hord @ 2012-10-26 19:44 UTC (permalink / raw)
To: git; +Cc: phil.hord, Jens Lehmann, Jeff King, Phil Hord
The submodule sync command was somehow left out when
--recursive was added to the other submodule commands.
Teach sync to handle the --recursive switch by recursing
when we're in a submodule we are sync'ing.
Change the report during sync to show submodule-path
instead of submodule-name to be consistent with the other
submodule commands and to help recursed paths make sense.
Signed-off-by: Phil Hord <hordp@cisco.com>
---
git-submodule.sh | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..0ca3af2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -11,7 +11,7 @@ USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <r
or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
or: $dashless [--quiet] foreach [--recursive] <command>
- or: $dashless [--quiet] sync [--] [<path>...]"
+ or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
OPTIONS_SPEC=
. git-sh-setup
. git-sh-i18n
@@ -1010,6 +1010,10 @@ cmd_sync()
GIT_QUIET=1
shift
;;
+ --recursive)
+ recursive=1
+ shift
+ ;;
--)
shift
break
@@ -1051,7 +1055,7 @@ cmd_sync()
if git config "submodule.$name.url" >/dev/null 2>/dev/null
then
- say "$(eval_gettext "Synchronizing submodule url for '\$name'")"
+ say "$(eval_gettext "Synchronizing submodule url for '\$prefix\$sm_path'")"
git config submodule."$name".url "$super_config_url"
if test -e "$sm_path"/.git
@@ -1061,6 +1065,12 @@ cmd_sync()
cd "$sm_path"
remote=$(get_default_remote)
git config remote."$remote".url "$sub_origin_url"
+
+ if test -n "$recursive"
+ then
+ prefix="$prefix$sm_path/"
+ eval cmd_sync
+ fi
)
fi
fi
--
1.8.0.3.g9dae067
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCHv3 2/2] Add tests for submodule sync --recursive
2012-10-26 19:44 ` [PATCHv3 0/2] Teach --recursive to submodule sync Phil Hord
2012-10-26 19:44 ` [PATCHv3 1/2] " Phil Hord
@ 2012-10-26 19:44 ` Phil Hord
2012-10-28 21:02 ` [PATCHv3 0/2] Teach --recursive to submodule sync Jens Lehmann
2 siblings, 0 replies; 12+ messages in thread
From: Phil Hord @ 2012-10-26 19:44 UTC (permalink / raw)
To: git; +Cc: phil.hord, Jens Lehmann, Jeff King, Phil Hord
Signed-off-by: Phil Hord <hordp@cisco.com>
---
t/t7403-submodule-sync.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 524d5c1..94e26c4 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -17,18 +17,25 @@ test_expect_success setup '
git commit -m upstream &&
git clone . super &&
git clone super submodule &&
+ (cd submodule &&
+ git submodule add ../submodule sub-submodule &&
+ test_tick &&
+ git commit -m "sub-submodule"
+ ) &&
(cd super &&
git submodule add ../submodule submodule &&
test_tick &&
git commit -m "submodule"
) &&
git clone super super-clone &&
- (cd super-clone && git submodule update --init) &&
+ (cd super-clone && git submodule update --init --recursive) &&
git clone super empty-clone &&
(cd empty-clone && git submodule init) &&
git clone super top-only-clone &&
git clone super relative-clone &&
- (cd relative-clone && git submodule update --init)
+ (cd relative-clone && git submodule update --init --recursive) &&
+ git clone super recursive-clone &&
+ (cd recursive-clone && git submodule update --init --recursive)
'
test_expect_success 'change submodule' '
@@ -46,6 +53,11 @@ test_expect_success 'change submodule url' '
git pull
) &&
mv submodule moved-submodule &&
+ (cd moved-submodule &&
+ git config -f .gitmodules submodule.sub-submodule.url ../moved-submodule &&
+ test_tick &&
+ git commit -a -m moved-sub-submodule
+ ) &&
(cd super &&
git config -f .gitmodules submodule.submodule.url ../moved-submodule &&
test_tick &&
@@ -61,6 +73,9 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
test -d "$(cd super-clone/submodule &&
git config remote.origin.url
)" &&
+ test ! -d "$(cd super-clone/submodule/sub-submodule &&
+ git config remote.origin.url
+ )" &&
(cd super-clone/submodule &&
git checkout master &&
git pull
@@ -70,6 +85,25 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
)
'
+test_expect_success '"git submodule sync --recursive" should update all submodule URLs' '
+ (cd super-clone &&
+ (cd submodule &&
+ git pull --no-recurse-submodules
+ ) &&
+ git submodule sync --recursive
+ ) &&
+ test -d "$(cd super-clone/submodule &&
+ git config remote.origin.url
+ )" &&
+ test -d "$(cd super-clone/submodule/sub-submodule &&
+ git config remote.origin.url
+ )" &&
+ (cd super-clone/submodule/sub-submodule &&
+ git checkout master &&
+ git pull
+ )
+'
+
test_expect_success '"git submodule sync" should update known submodule URLs' '
(cd empty-clone &&
git pull &&
@@ -107,6 +141,23 @@ test_expect_success '"git submodule sync" handles origin URL of the form foo/bar
#actual foo/submodule
test "$(git config remote.origin.url)" = "../foo/submodule"
)
+ (cd submodule/sub-submodule &&
+ test "$(git config remote.origin.url)" != "../../foo/submodule"
+ )
+ )
+'
+
+test_expect_success '"git submodule sync --recursive" propagates changes in origin' '
+ (cd recursive-clone &&
+ git remote set-url origin foo/bar &&
+ git submodule sync --recursive &&
+ (cd submodule &&
+ #actual foo/submodule
+ test "$(git config remote.origin.url)" = "../foo/submodule"
+ )
+ (cd submodule/sub-submodule &&
+ test "$(git config remote.origin.url)" = "../../foo/submodule"
+ )
)
'
--
1.8.0.3.g9dae067
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCHv3 0/2] Teach --recursive to submodule sync
2012-10-26 19:44 ` [PATCHv3 0/2] Teach --recursive to submodule sync Phil Hord
2012-10-26 19:44 ` [PATCHv3 1/2] " Phil Hord
2012-10-26 19:44 ` [PATCHv3 2/2] Add tests for submodule sync --recursive Phil Hord
@ 2012-10-28 21:02 ` Jens Lehmann
2 siblings, 0 replies; 12+ messages in thread
From: Jens Lehmann @ 2012-10-28 21:02 UTC (permalink / raw)
To: Phil Hord; +Cc: git, phil.hord, Jeff King
Am 26.10.2012 21:44, schrieb Phil Hord:
> [PATCHv3 1/2] Teach --recursive to submodule sync
>
> Now with less noise and no redundant flags passed to the recursive call.
>
>
> [PATCHv3 2/2] Add tests for submodule sync --recursive
>
> The test remains unchanged.
Both are looking good.
Acked-By: Jens Lehmann <Jens.Lehmann@web.de>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] submodule status: remove unused orig_* variables
2012-10-26 19:29 ` Jens Lehmann
@ 2012-10-28 21:37 ` Jens Lehmann
2012-10-29 7:27 ` Jeff King
0 siblings, 1 reply; 12+ messages in thread
From: Jens Lehmann @ 2012-10-28 21:37 UTC (permalink / raw)
To: Phil Hord; +Cc: Git Mailing List, phil.hord, Jeff King
When renaming orig_args to orig_flags in 98dbe63d (submodule: only
preserve flags across recursive status/update invocations) the call site
of the recursive cmd_status was forgotten. At that place orig_args is
still passed into the recursion, which is always empty since then. This
did not break anything because the orig_flags logic is not needed at all
when a function from the submodule script is called with eval, as that
inherits all the variables set by the option parsing done in the first
level of the recursion.
Now that we know that orig_flags and orig_args aren't needed at all,
let's just remove them from cmd_status().
Thanks-to: Phil Hord <hordp@cisco.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
Am 26.10.2012 21:29, schrieb Jens Lehmann:
> Am 26.10.2012 21:13, schrieb Phil Hord:
>> A test in t7404-submodule-foreach purports to test that
>> the --cached flag is properly noticed by --recursive calls
>> to the foreach command as it descends into nested
>> submodules. However, the test really does not perform this
>> test since the change it looks for is in a top-level
>> submodule handled by the first invocation of the command.
>> To properly test for the flag being passed to recursive
>> invocations, the change must be buried deeper in the
>> hierarchy.
>>
>> Move the change one level deeper so it properly verifies
>> the recursive machinery of the 'git submodule status'
>> command.
>
> Me thinks we should definitely do this.
And I also think this patch should go on top of Phil's patch after what
we learned so far.
I'm currently checking if we can also safely remove orig_flags from
cmd_update(). At least the test suite runs fine without it ...
git-submodule.sh | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..c287464 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -926,7 +926,6 @@ cmd_summary() {
cmd_status()
{
# parse $args after "submodule ... status".
- orig_flags=
while test $# -ne 0
do
case "$1" in
@@ -950,7 +949,6 @@ cmd_status()
break
;;
esac
- orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
shift
done
@@ -990,7 +988,7 @@ cmd_status()
prefix="$displaypath/"
clear_local_git_env
cd "$sm_path" &&
- eval cmd_status "$orig_args"
+ eval cmd_status
) ||
die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
fi
--
1.8.0.42.g2ea983b
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] submodule status: remove unused orig_* variables
2012-10-28 21:37 ` [PATCH] submodule status: remove unused orig_* variables Jens Lehmann
@ 2012-10-29 7:27 ` Jeff King
0 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2012-10-29 7:27 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Phil Hord, Git Mailing List, phil.hord
On Sun, Oct 28, 2012 at 10:37:16PM +0100, Jens Lehmann wrote:
> When renaming orig_args to orig_flags in 98dbe63d (submodule: only
> preserve flags across recursive status/update invocations) the call site
> of the recursive cmd_status was forgotten. At that place orig_args is
> still passed into the recursion, which is always empty since then. This
> did not break anything because the orig_flags logic is not needed at all
> when a function from the submodule script is called with eval, as that
> inherits all the variables set by the option parsing done in the first
> level of the recursion.
>
> Now that we know that orig_flags and orig_args aren't needed at all,
> let's just remove them from cmd_status().
>
> Thanks-to: Phil Hord <hordp@cisco.com>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
I'll replace your earlier fix (which just fixes the s/args/flags/ and
adds a test) with Phil's patch, and this on top.
Thanks.
-Peff
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-10-29 7:27 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-25 22:20 [PATCH] submodule status: properly pass options with --recursive Jens Lehmann
2012-10-26 13:15 ` Jeff King
2012-10-26 19:07 ` Phil Hord
2012-10-26 19:13 ` [PATCH] t7407: Fix recursive submodule test Phil Hord
2012-10-26 19:29 ` Jens Lehmann
2012-10-28 21:37 ` [PATCH] submodule status: remove unused orig_* variables Jens Lehmann
2012-10-29 7:27 ` Jeff King
2012-10-26 19:26 ` [PATCH] submodule status: properly pass options with --recursive Jens Lehmann
2012-10-26 19:44 ` [PATCHv3 0/2] Teach --recursive to submodule sync Phil Hord
2012-10-26 19:44 ` [PATCHv3 1/2] " Phil Hord
2012-10-26 19:44 ` [PATCHv3 2/2] Add tests for submodule sync --recursive Phil Hord
2012-10-28 21:02 ` [PATCHv3 0/2] Teach --recursive to submodule sync Jens Lehmann
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).