Subject: Re: [PATCH] clone: add `--remote-submodules` flag

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 13 May 2019 23:12:17 +0200

On Mon, May 13 2019, Ben Avison wrote:

> @@ -792,6 +795,11 @@ static int checkout(int submodule_progress)
>  		if (option_verbosity < 0)
>  			argv_array_push(&args, "--quiet");
>
> +		if (option_remote_submodules == 1) {

I see you copied this from code above the context, but to check a bool
variable just use "if (var)" not "if (var == 1)".

> +test_expect_success 'check the default is --no-remote-submodules' '
> +	test_when_finished "rm -rf super_clone" &&
> +	git clone --recurse-submodules --no-remote-submodules "file://$pwd/." super_clone &&
> +	(
> +		cd super_clone/sub &&
> +		git diff --exit-code sub_when_added_to_super
> +	)
> +'

This isn't testing the default, it just tests --no-remote-submodules,
i.e. if you change the "static int" assignment to "1" (to make this new
option the default) all these tests will still pass.

Just fixing that means we finally test for this (the entire test suite
would pass before with this on, showing a concerning lack of test
coverage in the submodule tests...), but with just
s/--no-remote-submodules // here we'll test for it.

You might want to have another test to explicitly check what happens
with --no-remote-submodules, but I don't think it's worth the effort, at
that point you're just testing the getopt machinery.