diff mbox series

[v2,5/5] sparse-checkout: use repo_config_set_worktree_gently()

Message ID 06457fafa78e18b7bb7f6d87408b0759d98344d8.1640114048.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Sparse checkout: fix bug with worktree of bare repo | expand

Commit Message

Derrick Stolee Dec. 21, 2021, 7:14 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

The previous change added repo_config_set_worktree_gently() to assist
writing config values into the worktree.config file, especially when
that may not have been initialized.

When the base repo is bare, running 'git sparse-checkout init' in a
worktree will create the config.worktree file for the worktree, but that
will start causing the worktree to parse the bare repo's core.bare=true
value and start treating the worktree as bare. This causes more problems
as other commands are run in that worktree.

The fix is to have this assignment into config.worktree be handled by
the repo_config_set_worktree_gently() helper.

Reported-by: Sean Allred <allred.sean@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/sparse-checkout.c          | 25 ++++++++-----------------
 sparse-index.c                     | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh | 16 +++++++++++++++-
 3 files changed, 26 insertions(+), 25 deletions(-)

Comments

Eric Sunshine Dec. 22, 2021, 5:48 a.m. UTC | #1 
On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the worktree.config file, especially when
> that may not have been initialized.
>
> When the base repo is bare, running 'git sparse-checkout init' in a
> worktree will create the config.worktree file for the worktree, but that
> will start causing the worktree to parse the bare repo's core.bare=true
> value and start treating the worktree as bare. This causes more problems
> as other commands are run in that worktree.
>
> The fix is to have this assignment into config.worktree be handled by
> the repo_config_set_worktree_gently() helper.
>
> Reported-by: Sean Allred <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -71,6 +71,20 @@ test_expect_success 'git sparse-checkout init' '
> +test_expect_success 'init in a worktree of a bare repo' '

Nit: Although `init` doing an incomplete job of enabling per-worktree
config was indeed the source of the problem, it actually manifested
when invoking other commands, such as `set`. Consequently, it may be
slightly misleading to talk about `init` in the test title. A title
such as "worktree of a bare repo" might be good enough. Anyhow, just a
nit.

> +       test_when_finished rm -rf bare worktree &&
> +       git clone --bare repo bare &&
> +       git -C bare worktree add ../worktree &&
> +       (
> +               cd worktree &&
> +               git sparse-checkout init &&
> +               test_must_fail git config core.bare &&

Nit: I'm rather "meh" on explicitly checking `core.bare` here since
it's not particularly relevant to the test: just testing `init + set`
alone is enough to trigger the bug which begat this patch series.
Future readers of this test might even be confused by the presence of
this `core.bare` check.

> +               git sparse-checkout set /*

The `/*` is expanding to all entries in the root of the filesystem,
which probably isn't what you intended. I suspect you want literal
"/*", in which case you need to quote it:

    git sparse-checkout set "/*"

> +       ) &&
> +       git -C bare config --list --show-origin >actual &&
> +       grep "file:config.worktree      core.bare=true" actual

As mentioned above, I'm fairly meh on this part (and perhaps leaning
toward the negative) since it places too much emphasis on a low-level
detail. I _could_ see this as a test of the new function which
upgrades the repo to per-worktree config, but that's not what _this_
test is about.

> +'
diff mbox series

Patch

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 34447f87cd8..ec2c9a146cc 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -356,26 +356,17 @@  enum sparse_checkout_mode {
 
 static int set_config(enum sparse_checkout_mode mode)
 {
-	const char *config_path;
-
-	if (upgrade_repository_format(the_repository, 1) < 0)
-		die(_("unable to upgrade repository format to enable worktreeConfig"));
-	if (git_config_set_gently("extensions.worktreeConfig", "true")) {
-		error(_("failed to set extensions.worktreeConfig setting"));
+	if (repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckout",
+					    mode ? "true" : "false") ||
+	    repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckoutCone",
+					    mode == MODE_CONE_PATTERNS ?
+						"true" : "false"))
 		return 1;
-	}
-
-	config_path = git_path("config.worktree");
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckout",
-				      mode ? "true" : NULL);
-
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckoutCone",
-				      mode == MODE_CONE_PATTERNS ? "true" : NULL);
 
 	if (mode == MODE_NO_PATTERNS)
-		set_sparse_index_config(the_repository, 0);
+		return set_sparse_index_config(the_repository, 0);
 
 	return 0;
 }
diff --git a/sparse-index.c b/sparse-index.c
index a1d505d50e9..e93609999e0 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -99,13 +99,9 @@  static int convert_to_sparse_rec(struct index_state *istate,
 
 int set_sparse_index_config(struct repository *repo, int enable)
 {
-	int res;
-	char *config_path = repo_git_path(repo, "config.worktree");
-	res = git_config_set_in_file_gently(config_path,
-					    "index.sparse",
-					    enable ? "true" : NULL);
-	free(config_path);
-
+	int res = repo_config_set_worktree_gently(repo,
+						  "index.sparse",
+						  enable ? "true" : "false");
 	prepare_repo_settings(repo);
 	repo->settings.sparse_index = enable;
 	return res;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..b563ccfeb36 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -71,6 +71,20 @@  test_expect_success 'git sparse-checkout init' '
 	check_files repo a
 '
 
+test_expect_success 'init in a worktree of a bare repo' '
+	test_when_finished rm -rf bare worktree &&
+	git clone --bare repo bare &&
+	git -C bare worktree add ../worktree &&
+	(
+		cd worktree &&
+		git sparse-checkout init &&
+		test_must_fail git config core.bare &&
+		git sparse-checkout set /*
+	) &&
+	git -C bare config --list --show-origin >actual &&
+	grep "file:config.worktree	core.bare=true" actual
+'
+
 test_expect_success 'git sparse-checkout list after init' '
 	git -C repo sparse-checkout list >actual &&
 	cat >expect <<-\EOF &&
@@ -219,7 +233,7 @@  test_expect_success 'sparse-index enabled and disabled' '
 		test-tool -C repo read-cache --table >cache &&
 		! grep " tree " cache &&
 		git -C repo config --list >config &&
-		! grep index.sparse config
+		test_cmp_config -C repo false index.sparse
 	)
 '