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

From: Jens Lehmann <jens.lehmann@web.de>
Date: Jul 29, 2012, 15:29 UTC
Message-ID: <501556CF.1000605@web.de>
In-Reply-To: <7vtxwrw0g0.fsf@alter.siamese.dyndns.org>

Am 29.07.2012 08:22, schrieb Junio C Hamano:

Show 14 quoted lines

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

Show 5 quoted lines

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

Show 23 quoted lines

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