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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2014, 22:56 UTC
Message-ID: <xmqq8utrdcuh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <52EFF2EA.9060709@web.de>

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?

Show 9 quoted lines

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

Show 15 quoted lines

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