Re: [RFC PATCH 3/4] read-tree: teach --submodule-prefix

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 9, 2022, 03:13 UTC
Message-ID: <221109.86y1skq08e.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221109004708.97668-4-chooglen@google.com>

On Tue, Nov 08 2022, Glen Choo wrote:

Other things aside:

Show 56 quoted lines

> This also fixes an existing bug where "git --super-prefix=<path>
> read-tree" (typically invoked by "git restore") in a partial clone with
> submodules could fail because we fetch promisor objects with "git
> fetch", but "git fetch" doesn't support "--super-prefix".
> [...]
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 037941b95d..9bec57a047 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -644,6 +644,49 @@ test_expect_success 'repack does not loosen promisor objects' '
>  	grep "loosen_unused_packed_objects/loosened:0" trace
>  '
>  
> +test_expect_success 'setup src repo with submodules' '
> +	test_config_global protocol.file.allow always &&
> +
> +	git init src-sub &&
> +	git -C src-sub config uploadpack.allowfilter 1 &&
> +	git -C src-sub config uploadpack.allowanysha1inwant 1 &&
> +
> +	# This blob must be missing in the subsequent commit.
> +	echo foo >src-sub/file &&
> +	git -C src-sub add file &&
> +	git -C src-sub commit -m "submodule one" &&
> +	SUB_ONE=$(git -C src-sub rev-parse HEAD) &&
> +
> +	echo bar >src-sub/file &&
> +	git -C src-sub add file &&
> +	git -C src-sub commit -m "submodule two" &&
> +	SUB_TWO=$(git -C src-sub rev-parse HEAD) &&
> +
> +	git init src-super &&
> +	git -C src-super config uploadpack.allowfilter 1 &&
> +	git -C src-super config uploadpack.allowanysha1inwant 1 &&
> +	git -C src-super submodule add ../src-sub src-sub &&
> +
> +	git -C src-super/src-sub checkout $SUB_ONE &&
> +	git -C src-super add src-sub &&
> +	git -C src-super commit -m "superproject one" &&
> +
> +	git -C src-super/src-sub checkout $SUB_TWO &&
> +	git -C src-super add src-sub &&
> +	git -C src-super commit -m "superproject two"
> +'
> +
> +test_expect_success 'lazy-fetch in submodule succeeds' '
> +	test_when_finished "rm -rf src-super src-sub client" &&
> +
> +	test_config_global protocol.file.allow always &&
> +	git clone --filter=blob:none --also-filter-submodules \
> +		--recurse-submodules "file://$(pwd)/src-super" client &&
> +
> +	# Trigger lazy-fetch from the superproject
> +	git -C client restore --recurse-submodules --source=HEAD^ :/
> +'
> +

If I take this test addition on top of "master", and don't apply any of the C changes on this topic it'll fail in "fetch", and then in "index-pack"/"unpack-objects" etc., as "fetch" invokes those, due to us using the --super-prefix.

But just this test addition and this code change would also "fix it":
	
	diff --git a/builtin/fetch.c b/builtin/fetch.c
	index 7378cafeec9..c5709a9e37b 100644
	--- a/builtin/fetch.c
	+++ b/builtin/fetch.c
	@@ -2114,6 +2114,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
	 	int result = 0;
	 	int prune_tags_ok = 1;
	 
	+	unsetenv(GIT_SUPER_PREFIX_ENVIRONMENT);
	+
	 	packet_trace_identity("fetch");
	 
	 	/* Record the command line for the reflog */
	diff --git a/git.c b/git.c
	index 10202a7f126..b5e63a0a57b 100644
	--- a/git.c
	+++ b/git.c
	@@ -531,7 +531,7 @@ static struct cmd_struct commands[] = {
	 	{ "env--helper", cmd_env__helper },
	 	{ "fast-export", cmd_fast_export, RUN_SETUP },
	 	{ "fast-import", cmd_fast_import, RUN_SETUP | NO_PARSEOPT },
	-	{ "fetch", cmd_fetch, RUN_SETUP },
	+	{ "fetch", cmd_fetch, RUN_SETUP | SUPPORT_SUPER_PREFIX /* not really* */ },
	 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
	 	{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
	 	{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },

Doesn't that do an end-run around the goals of this topic, but just in a much simpler way?

I.e. I'm all for not propagating the --super-prefix any more than we need to, but part of that is because once we start passing it, we set it in the environment, and then we'll *keep* passing it. So we've had some commands "accept" it, when really they don't care, they're just being invoked by other commands that needed it originally.

But maybe we can just unsetenv() the prefix before invoking any built-in that doesn't have SUPPORT_SUPER_PREFIX, or not set it in the environment in the first place, but rather carry it forward more explicitly only with the "--super-prefix" flag to "git" (adn then only to those specific commands?