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

[Junio C Hamano <gitster@pobox.com>]

Antonio Ospite <ao2@ao2.it> writes:

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

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

> 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)?

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

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

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