Re: [WIP/PATCH 3/9] Teach checkout the --[no-]recurse-submodules option

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

Am 03.02.2014 23:56, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> +	set_config_update_recurse_submodules(
>> +		parse_update_recurse_submodules_arg("--recurse-submodules-default",
>> +						    recurse_submodules_default),
>> +		recurse_submodules);
> 
> I think I saw these exact lines in another patch.  Perhaps the whole
> thing can become a helper function that lets the caller avoid typing
> the whole long strings that needs a strange/unfortunate line break? 

Right, that'd be better.

>> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
>> index 06b18f8..bc3e1ca 100755
>> --- a/t/t2013-checkout-submodule.sh
>> +++ b/t/t2013-checkout-submodule.sh
>> @@ -4,17 +4,57 @@ test_description='checkout can handle submodules'
>>
>>  . ./test-lib.sh
>>
>> +submodule_creation_must_succeed() {
> 
> Style: SP before (), i.e.
> 
> 	submodule_creation_must_succeed () {
> 
>> +	# checkout base ($1)
>> +	git checkout -f --recurse-submodules $1 &&
>> +	git diff-files --quiet &&
>> +	git diff-index --quiet --cached $1 &&
> 
> Please make it a habit to quote a parameter that is intended not to
> be split at $IFS (e.g. write these as "$1" not as $1).  Otherwise
> the reader has to wonder if this can be called with a "foo bar" and
> the expects it to be split into two.
> 
>> +	# checkout target ($2)
>> +	if test -d submodule; then
> 
> Style: no semicolons in standard control structure, i.e.
> 
> 	if test -d submodule
> 	then
> 
>> +		echo change>>submodule/first.t &&
> 
> Style: SP before but not after redirection operator, i.e.
> 
> 	echo foo >>bar
> 
>> +submodule_removal_must_succeed() {
> 
> Likewise.
> 
>> +	# checkout base ($1)
>> +	git checkout -f --recurse-submodules $1 &&
> 
> Likewise.
> 
>> +	echo first > file &&
> 
> Likewise.
> 
>> +test_expect_success '"checkout --recurse-submodules" replaces submodule with files' '
>> +	git checkout -f base &&
>> +	git checkout -b replace_submodule_with_dir &&
>> +	git update-index --force-remove submodule &&
>> +	rm -rf submodule/.git .gitmodules &&
>> +	git add .gitmodules submodule/* &&
>> +	git commit -m "submodule replaced" &&
>> +	git checkout -f base &&
>> +	git submodule update -f &&
>> +	git checkout --recurse-submodules replace_submodule_with_dir &&
>> +	test -d submodule &&
>> +	! test -e submodule/.git &&
>> +	test -f submodule/first.t &&
>> +	test -f submodule/second.t
>> +'
> 
> Hmmmm.  Is it sufficient for these files to just exist, or do we
> want to make sure they have expected contents?

Thanks, will consider all you remarks above in the ongoing work for
testing framework which should replace these tests.