Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

On Mon, 8 Oct 2018 15:19:00 -0700 Stefan Beller <sbeller@google.com> wrote:

OK, I agree, let's re-check also *after* we tried and failed to set a config value, just to be sure that the code does not get accidentally changed in the future to create the file. I'll add the check.

Except for the "summary" test which was not even exercising the config_from_gitmodule path, checking exist status should be sufficient to verify that "submodule--helper config" does not fail, but we can surely do better.

I will add checks to confirm that not only the commands exited without errors but they also achieved the desired effect, to validate the actual high-level use case advertised by the test file. This should be more future-proof.

And I think I'll merge the summary and the update tests.

Mmh, this should have been "submodule update --remote" in the first place to have any effect, I'll take the chance and rewrite this test in a different way and also check the effect of the update operation, and the repository status.

I'll be something like this:

ORIG_SUBMODULE=$(git -C submodule rev-parse HEAD) ORIG_UPSTREAM=$(git -C upstream rev-parse HEAD) ORIG_SUPER=$(git -C super rev-parse HEAD)

test_expect_success 're-updating submodule when the gitmodules config is not checked out' '
	test_when_finished "git -C submodule reset --hard $ORIG_SUBMODULE;
	                    git -C upstream reset --hard $ORIG_UPSTREAM;
	                    git -C super reset --hard $ORIG_SUPER;
	                    git -C upstream submodule update --remote;
	                    git -C super pull;
	                    git -C super submodule update --remote" &&
	(cd submodule &&
		echo file2 >file2 &&
		git add file2 &&
		test_tick &&
		git commit -m "add file2 to submodule"
	) &&
	(cd upstream &&
		git submodule update --remote &&
		git add submodule &&
		test_tick &&
		git commit -m "Update submodule"
	) &&
	git -C super pull &&
	# The --for-status options reads the gitmdoules config
	git -C super submodule summary --for-status >actual &&
	cat >expect <<-\EOF &&
	* submodule 951c301...a939200 (1):
	  < add file2 to submodule
	
	EOF
	test_cmp expect actual &&
	# Test that the update actually succeeds
	test_path_is_missing super/submodule/file2 &&
	git -C super submodule update &&
	test_cmp submodule/file2 super/submodule/file2 &&
	git -C super status --short >output &&
	test_must_be_empty output
'

Maybe a little overkill?

The "upstream" repo will be added in test 1 to better clarify the roles of the involved repositories.

The commit ids should be stable because of test_tick, shouldn't they?

Thanks for the comments, they helped improving the quality of the tests once again.

I'll wait a few days before sending a v7, hopefully someone will find time to take another look at patch 9 and comment also on patch 10, and give an opinion on the "mergeability" status of the whole patchset.

Ciao ciao,
   Antonio