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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 13, 2019, 21:12 UTC
Message-ID: <87sgtivz2m.fsf@evledraar.gmail.com>
In-Reply-To: <20190513175903.126079-1-bavison@riscosopen.org>

On Mon, May 13 2019, Ben Avison wrote:

Show 5 quoted lines

> @@ -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)".

Show 8 quoted lines

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