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

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 29.07.2012 08:22, schrieb Junio C Hamano:
> Heiko Voigt <hvoigt@hvoigt.net> 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.

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 ;-).

>> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
>> ---
>>  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
>