Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2018, 20:36 UTC
Message-ID: <xmqqmutoznwe.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180814110525.17801-8-ao2@ao2.it>

Antonio Ospite <ao2@ao2.it> writes:

Show 9 quoted lines

>  	/* Equivalent to ACTION_SET in builtin/config.c */
> -	if (argc == 3)
> +	if (argc == 3) {
> +		struct object_id oid;
> +
> +		/*
> +		 * If the .gitmodules file is not in the working tree but it
> +		 * is in the current branch, stop, as writing new values (and
> +		 * staging them) would blindly overwrite ALL the old content.

Hmph, "the file is missing" certainly is a condition we would want to notice, but wouldn't we in general want to prevent us from overwriting any local modification, where "missing" is merely a very special case of local modification? I am wondering if we would want to stop if .gitmodules file exists both in the working tree and in the index, and the contents of them differ, or something like that.

Show 11 quoted lines

> diff --git a/git-submodule.sh b/git-submodule.sh
> index ff258e2e8c..b1cb187227 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -159,6 +159,13 @@ cmd_add()
>  		shift
>  	done
>  
> +	# For more details about this check, see
> +	# builtin/submodule--helper.c::module_config()
> +	if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1

No SP between redirection '>' and its target '/dev/null'.

More importantly, I think it is better to add a submodule--helper subcommand that exposes the check in question, as the code is already written ;-) That approach will guarantee that the logic and the message stay the same between here and in the C code. Then you do not even need these two line comment.

> +	then
> +		 die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
> +	fi
> +

Show 26 quoted lines

> diff --git a/submodule-config.c b/submodule-config.c
> index b7ef055c63..088dabb56f 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "dir.h"
>  #include "repository.h"
>  #include "config.h"
>  #include "submodule-config.h"
> @@ -603,8 +604,19 @@ static void submodule_cache_check_init(struct repository *repo)
>  static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
>  {
>  	if (repo->worktree) {
> -		char *file = repo_worktree_path(repo, GITMODULES_FILE);
> -		git_config_from_file(fn, file, data);
> +		struct git_config_source config_source = { 0 };
> +		const struct config_options opts = { 0 };
> +		struct object_id oid;
> +		char *file;
> +
> +		file = repo_worktree_path(repo, GITMODULES_FILE);
> +		if (file_exists(file))
> +			config_source.file = file;
> +		else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
> +			config_source.blob = GITMODULES_HEAD;

What is the reason why we fall back directly to HEAD when working tree file does not exist? I thought that our usual fallback was to the version in the index for other things like .gitignore/attribute and this codepath look like an oddball. Are you trying to handle the case where we are in a bare repository without any file checked out (and there is not even the index)?

Show 24 quoted lines

> diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
> new file mode 100755
> index 0000000000..5341e9b012
> --- /dev/null
> +++ b/t/t7416-submodule-sparse-gitmodules.sh
> @@ -0,0 +1,90 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
> +#
> +
> +test_description='Test reading/writing .gitmodules when not in the working tree
> +
> +This test verifies that, when .gitmodules is in the current branch but is not
> +in the working tree reading from it still works but writing to it does not.
> +
> +The test setup uses a sparse checkout, however the same scenario can be set up
> +also by committing .gitmodules and then just removing it from the filesystem.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'sparse checkout setup which hides .gitmodules' '
> +	echo file > file &&

No SP between redirection '>' and its target 'file'.

Show 14 quoted lines

> +	git add file &&
> +	test_tick &&
> +	git commit -m upstream &&
> +	git clone . super &&
> +	git clone super submodule &&
> +	git clone super new_submodule &&
> +	(cd super &&
> +		git submodule add ../submodule &&
> +		test_tick &&
> +		git commit -m submodule &&
> +		cat >.git/info/sparse-checkout <<\EOF &&
> +/*
> +!/.gitmodules
> +EOF

You can use <<-\EOF and indent the body of the here-doc, which makes the result easier to read, i.e.

		cat >target <<-\EOF &&
		line 1
		line 2
		EOF

> +		git config core.sparsecheckout true &&
> +		git read-tree -m -u HEAD &&

That's old fashioned---I am curious if this has to be one-way merge or can just be a usual "git checkout" (I am merely curious; not suggesting to change anything).

Show 9 quoted lines

> +		test_path_is_missing .gitmodules
> +	)
> +'
> +
> +test_expect_success 'reading gitmodules config file when it is not checked out' '
> +	(cd super &&
> +		echo "../submodule" >expected &&
> +		git submodule--helper config submodule.submodule.url >actual &&
> +		test_cmp expected actual

A minor style thing, but I thought that it was more common in our tests to call the expected output 'expect' (which has the same number of letters as 'actual') than 'expected'.

More importantly, do we want a subshell, or is something like this sufficient?

	echo "../submodule" >expected &&
	git -C super submodule--helper config ... >actual &&
	test_cmp expect actual

The same comment applies to many tests I see below (omitted).