Re: [PATCH] cleanup argument passing in submodule status command

Heiko Voigt [off-list ref] writes:

Note: This is a code cleanup and does not fix any bugs. As a side effect
the variables containing the parsed flags to "git submodule status" are
passed down recursively. So everything was already behaving as expected.

If that is the case, shouldn't we stop passing anything down, if we
want it to be a "clean-up only, no behaviour changes" patch?  While
at it, we may want to kill that code to accumulate the original
options in orig_flags because we haven't been using the variable.

We _know_ $orig_args has been empty, i.e. the code has been working
fine with only cmd_status there.  Nobody has tried what happens when
we pass the original arguments to cmd_status on that line.  The
patch changes the behaviour of the code; it makes the command line
parsing "while" loop to run again, and if the code that accumulates
original options in orig_flags have been buggy, now that bug will be
exposed.

Am 29.07.2012 08:22, schrieb Junio C Hamano:

Heiko Voigt [off-list ref] writes:

quoted

Note: This is a code cleanup and does not fix any bugs. As a side effect
the variables containing the parsed flags to "git submodule status" are
passed down recursively. So everything was already behaving as expected.

If that is the case, shouldn't we stop passing anything down, if we
want it to be a "clean-up only, no behaviour changes" patch?  While
at it, we may want to kill that code to accumulate the original
options in orig_flags because we haven't been using the variable.

We _know_ $orig_args has been empty, i.e. the code has been working
fine with only cmd_status there.  Nobody has tried what happens when
we pass the original arguments to cmd_status on that line.

I tried today. Before this change no arguments got passed down and
afterwards they are (but just the arguments, no submodule paths
were passed on in either case; which is what Kevin fixed in the
commit Heiko referenced). Three arguments are allowed for "git
submodule status":

--recursive:
It doesn't matter if we pass that on or not because $recursive is
reused when "eval cmd_status" is executed.

--quiet:
Same as recursive, GIT_QUIET is set the first time and then reused
in the recursion.

--cached:
This was dropped when recursing into submodules but isn't anymore
with Heiko's change, so we do have a change in behavior here.

 The
patch changes the behaviour of the code; it makes the command line
parsing "while" loop to run again, and if the code that accumulates
original options in orig_flags have been buggy, now that bug will be
exposed.

Hmm, when --cached is used together with --recursive, I would expect
it to show the commit stored in the index for the deeper submodules
too (and not magically switch to show their HEAD again after the
first level of submodules). To me this looks like a bug which Kevin
accidentally introduced and nobody noticed and/or reported until now.

So I'd vote for making this a bugfix patch for "git submodule status
--cached --recursive" (and would love to see a test for it ;-).

quoted

Signed-off-by: Heiko Voigt <redacted>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index dba4d39..3a3f0a4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -961,7 +961,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

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