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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Nov 08 2022, Glen Choo wrote:

Other things aside:

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