diff mbox series

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

Message ID 20221109004708.97668-4-chooglen@google.com (mailing list archive)
State New, archived
Headers show
Series git: remove --super-prefix | expand

Commit Message

Glen Choo Nov. 9, 2022, 12:47 a.m. UTC
Following the precedent of previous commit, teach "git read-tree" the
"--submodule-prefix" flag, replacing its use of the global
"--super-prefix" flag.

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

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/read-tree.c         |  4 ++++
 submodule.c                 | 22 +++++++------------
 t/t1001-read-tree-m-2way.sh |  4 ++--
 t/t5616-partial-clone.sh    | 43 +++++++++++++++++++++++++++++++++++++
 unpack-trees.c              | 32 +++++++++++++--------------
 5 files changed, 73 insertions(+), 32 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 9, 2022, 3:13 a.m. UTC | #1
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?
diff mbox series

Patch

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index f4cbe460b9..7aedab6951 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -148,6 +148,10 @@  int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 		OPT_CALLBACK_F(0, "recurse-submodules", NULL,
 			    "checkout", "control recursive updating of submodules",
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
+		OPT_CALLBACK_F(0, "submodule-prefix", NULL, "path",
+			       "internal, path from root of top-level superproject tree to this repo",
+			       PARSE_OPT_HIDDEN, option_parse_submodule_prefix),
+
 		OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
 		OPT_END()
 	};
diff --git a/submodule.c b/submodule.c
index d84345a0b4..d3d6abc816 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2073,14 +2073,6 @@  void submodule_unset_core_worktree(const struct submodule *sub)
 	strbuf_release(&config_path);
 }
 
-static const char *get_super_prefix_or_empty(void)
-{
-	const char *s = get_super_prefix();
-	if (!s)
-		s = "";
-	return s;
-}
-
 static int submodule_has_dirty_index(const struct submodule *sub)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -2108,10 +2100,11 @@  static void submodule_reset_index(const char *path)
 	cp.no_stdin = 1;
 	cp.dir = path;
 
-	strvec_pushf(&cp.args, "--super-prefix=%s%s/",
-		     get_super_prefix_or_empty(), path);
 	/* TODO: determine if this might overwright untracked files */
-	strvec_pushl(&cp.args, "read-tree", "-u", "--reset", NULL);
+	strvec_push(&cp.args, "read-tree");
+	strvec_pushf(&cp.args, "--submodule-prefix=%s%s/",
+		     get_submodule_prefix(), path);
+	strvec_pushl(&cp.args, "-u", "--reset", NULL);
 
 	strvec_push(&cp.args, empty_tree_oid_hex());
 
@@ -2191,9 +2184,10 @@  int submodule_move_head(const char *path,
 	cp.no_stdin = 1;
 	cp.dir = path;
 
-	strvec_pushf(&cp.args, "--super-prefix=%s%s/",
-		     get_super_prefix_or_empty(), path);
-	strvec_pushl(&cp.args, "read-tree", "--recurse-submodules", NULL);
+	strvec_push(&cp.args, "read-tree");
+	strvec_pushf(&cp.args, "--submodule-prefix=%s%s/",
+		     get_submodule_prefix(), path);
+	strvec_push(&cp.args, "--recurse-submodules");
 
 	if (flags & SUBMODULE_MOVE_HEAD_DRY_RUN)
 		strvec_push(&cp.args, "-n");
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 516a6112fd..9cfba1a2af 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -366,11 +366,11 @@  test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
 	test -f a/b
 '
 
-test_expect_success 'read-tree supports the super-prefix' '
+test_expect_success 'read-tree supports --submodule-prefix' '
 	cat <<-EOF >expect &&
 		error: Updating '\''fictional/a'\'' would lose untracked files in it
 	EOF
-	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
+	test_must_fail git read-tree --submodule-prefix fictional/ -u -m "$treeH" "$treeM" 2>actual &&
 	test_cmp expect actual
 '
 
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^ :/
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/unpack-trees.c b/unpack-trees.c
index bae812156c..930a2a46f1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -71,7 +71,7 @@  static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
-static const char *super_prefixed(const char *path)
+static const char *submodule_prefixed(const char *path)
 {
 	/*
 	 * It is necessary and sufficient to have two static buffers
@@ -79,28 +79,28 @@  static const char *super_prefixed(const char *path)
 	 * error() using the unpack_*_errors[] templates we see above.
 	 */
 	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
-	static int super_prefix_len = -1;
+	static int submodule_prefix_len = -1;
 	static unsigned idx = ARRAY_SIZE(buf) - 1;
 
-	if (super_prefix_len < 0) {
-		const char *super_prefix = get_super_prefix();
-		if (!super_prefix) {
-			super_prefix_len = 0;
+	if (submodule_prefix_len < 0) {
+		const char *submodule_prefix = get_submodule_prefix();
+		if (!submodule_prefix) {
+			submodule_prefix_len = 0;
 		} else {
 			int i;
 			for (i = 0; i < ARRAY_SIZE(buf); i++)
-				strbuf_addstr(&buf[i], super_prefix);
-			super_prefix_len = buf[0].len;
+				strbuf_addstr(&buf[i], submodule_prefix);
+			submodule_prefix_len = buf[0].len;
 		}
 	}
 
-	if (!super_prefix_len)
+	if (!submodule_prefix_len)
 		return path;
 
 	if (++idx >= ARRAY_SIZE(buf))
 		idx = 0;
 
-	strbuf_setlen(&buf[idx], super_prefix_len);
+	strbuf_setlen(&buf[idx], submodule_prefix_len);
 	strbuf_addstr(&buf[idx], path);
 
 	return buf[idx].buf;
@@ -236,7 +236,7 @@  static int add_rejected_path(struct unpack_trees_options *o,
 		return -1;
 
 	if (!o->show_all_errors)
-		return error(ERRORMSG(o, e), super_prefixed(path));
+		return error(ERRORMSG(o, e), submodule_prefixed(path));
 
 	/*
 	 * Otherwise, insert in a list for future display by
@@ -263,7 +263,7 @@  static void display_error_msgs(struct unpack_trees_options *o)
 			error_displayed = 1;
 			for (i = 0; i < rejects->nr; i++)
 				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
-			error(ERRORMSG(o, e), super_prefixed(path.buf));
+			error(ERRORMSG(o, e), submodule_prefixed(path.buf));
 			strbuf_release(&path);
 		}
 		string_list_clear(rejects, 0);
@@ -290,7 +290,7 @@  static void display_warning_msgs(struct unpack_trees_options *o)
 			warning_displayed = 1;
 			for (i = 0; i < rejects->nr; i++)
 				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
-			warning(ERRORMSG(o, e), super_prefixed(path.buf));
+			warning(ERRORMSG(o, e), submodule_prefixed(path.buf));
 			strbuf_release(&path);
 		}
 		string_list_clear(rejects, 0);
@@ -2958,8 +2958,8 @@  int bind_merge(const struct cache_entry * const *src,
 	if (a && old)
 		return o->quiet ? -1 :
 			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
-			      super_prefixed(a->name),
-			      super_prefixed(old->name));
+			      submodule_prefixed(a->name),
+			      submodule_prefixed(old->name));
 	if (!a)
 		return keep_entry(old, o);
 	else
@@ -3020,7 +3020,7 @@  int stash_worktree_untracked_merge(const struct cache_entry * const *src,
 
 	if (worktree && untracked)
 		return error(_("worktree and untracked commit have duplicate entries: %s"),
-			     super_prefixed(worktree->name));
+			     submodule_prefixed(worktree->name));
 
 	return merged_entry(worktree ? worktree : untracked, NULL, o);
 }