diff mbox series

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

Message ID b200819c1bb564695b68d8ed2caebfca0302f93f.1640727143.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Sparse checkout: fix bug with worktree of bare repo | expand

Commit Message

Derrick Stolee Dec. 28, 2021, 9:32 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, if enabled.

Let the sparse-checkout builtin use this helper instead of attempting to
initialize the worktree config on its own. This changes behavior of 'git
sparse-checkout set' in a few important ways:

 1. Git will no longer upgrade the repository format and add the
    worktree config extension. The user should run 'git worktree
    init-worktree-config' to enable this feature.

 2. If worktree config is disabled, then this command will set the
    core.sparseCheckout (and possibly core.sparseCheckoutCone and
    index.sparse) values in the common config file.

 3. If the main worktree is bare, then this command will not put the
    worktree in a broken state.

The main reason to use worktree-specific config for the sparse-checkout
builtin was to avoid enabling sparse-checkout patterns in one and
causing a loss of files in another. If a worktree does not have a
sparse-checkout patterns file, then the sparse-checkout logic will not
kick in on that worktree.

This new logic introduces a new user pattern that could lead to some
confusion. Suppose a user has not upgraded to worktree config and
follows these steps in order:

 1. Enable sparse-checkout in a worktree.

 2. Disable sparse-checkout in that worktree without deleting that
    worktree's sparse-checkout file.

 3. Enable sparse-checkout in another worktree.

After these steps, the first worktree will have sparse-checkout enabled
with whatever patterns exist. The worktree does not immediately have
those patterns applied, but a variety of Git commands would apply the
sparse-checkout patterns and update the worktree state to reflect those
patterns. This situation is likely very rare and the workaround is to
upgrade to worktree specific config on purpose. Users already in this
state used the sparse-checkout builtin with a version that upgraded to
worktree config, anyway.

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 | 57 +++++++++++++++++++++++++-----
 3 files changed, 60 insertions(+), 32 deletions(-)

Comments

Eric Sunshine Dec. 30, 2021, 9:01 a.m. UTC | #1 
On Tue, Dec 28, 2021 at 4:32 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, if enabled.

s/worktree.config/config.worktree/

(I made the same mistake several times earlier in this discussion.)

> Let the sparse-checkout builtin use this helper instead of attempting to
> initialize the worktree config on its own. This changes behavior of 'git
> sparse-checkout set' in a few important ways:

"worktree config" is a bit ambiguous here. It might be clearer to say
"enable per-worktree configuration" or "enable worktree-specific
configuration".

>  1. Git will no longer upgrade the repository format and add the
>     worktree config extension. The user should run 'git worktree
>     init-worktree-config' to enable this feature.
>
>  2. If worktree config is disabled, then this command will set the
>     core.sparseCheckout (and possibly core.sparseCheckoutCone and
>     index.sparse) values in the common config file.
>
>  3. If the main worktree is bare, then this command will not put the
>     worktree in a broken state.

There are a few other cases of ambiguity in this enumerated list, as
well, and the need for `s/main worktree is bare/repository is bare/`,
however, if you end up restructuring this series the to follow the
recipe Elijah set forth at the bottom of [1], then most of this
patch's commit message will be rewritten anyhow, so my mention of
these minor issues will be moot.

[1]: https://lore.kernel.org/git/CABPp-BHuO3B366uJuODMQo-y449p8cAMVn0g2MTcO5di3Xa7Zg@mail.gmail.com/

> The main reason to use worktree-specific config for the sparse-checkout
> builtin was to avoid enabling sparse-checkout patterns in one and
> causing a loss of files in another. If a worktree does not have a
> sparse-checkout patterns file, then the sparse-checkout logic will not
> kick in on that worktree.
>
> This new logic introduces a new user pattern that could lead to some
> confusion. Suppose a user has not upgraded to worktree config and
> follows these steps in order:
>
>  1. Enable sparse-checkout in a worktree.
>
>  2. Disable sparse-checkout in that worktree without deleting that
>     worktree's sparse-checkout file.
>
>  3. Enable sparse-checkout in another worktree.
>
> After these steps, the first worktree will have sparse-checkout enabled
> with whatever patterns exist. The worktree does not immediately have
> those patterns applied, but a variety of Git commands would apply the
> sparse-checkout patterns and update the worktree state to reflect those
> patterns. This situation is likely very rare and the workaround is to
> upgrade to worktree specific config on purpose. Users already in this
> state used the sparse-checkout builtin with a version that upgraded to
> worktree config, anyway.
>
> Reported-by: Sean Allred <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
diff mbox series

Patch

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 08f8df2648c..d0d6749593e 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -359,26 +359,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 447a8669e02..15403158c49 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -146,15 +146,54 @@  test_expect_success 'interaction with clone --no-checkout (unborn index)' '
 '
 
 test_expect_success 'set enables config' '
-	git init empty-config &&
+	git init initial-config &&
 	(
-		cd empty-config &&
+		cd initial-config &&
+		test_commit file file &&
+		mkdir dir &&
+		test_commit dir dir/file &&
+		git worktree add --detach ../initial-worktree &&
+		git sparse-checkout set --cone
+	) &&
+	test_cmp_config -C initial-config true core.sparseCheckout &&
+	test_cmp_config -C initial-worktree true core.sparseCheckout &&
+	test_cmp_config -C initial-config true core.sparseCheckoutCone &&
+	test_cmp_config -C initial-worktree true core.sparseCheckoutCone &&
+
+	# initial-config has a sparse-checkout file
+	# that only contains files at root.
+	ls initial-config >only-file &&
+	cat >expect <<-EOF &&
+	file
+	EOF
+	test_cmp expect only-file &&
+
+	# initial-worktree does not have its own sparse-checkout
+	# file, so the repply does not modify the worktree at all.
+	git -C initial-worktree sparse-checkout reapply &&
+	ls initial-worktree >all &&
+	cat >expect <<-EOF &&
+	dir
+	file
+	EOF
+	test_cmp expect all
+'
+
+test_expect_success 'set enables worktree config, if enabled' '
+	git init worktree-config &&
+	(
+		cd worktree-config &&
 		test_commit test file &&
-		test_path_is_missing .git/config.worktree &&
-		git sparse-checkout set nothing &&
-		test_path_is_file .git/config.worktree &&
-		test_cmp_config true core.sparseCheckout
-	)
+		git worktree add --detach ../worktree-config2 &&
+		git worktree init-worktree-config &&
+		git sparse-checkout set --cone &&
+		git config --worktree core.sparseCheckout &&
+		git config --worktree core.sparseCheckoutCone
+	) &&
+	test_cmp_config -C worktree-config true core.sparseCheckout &&
+	test_must_fail git -C worktree-config2 core.sparseCheckout &&
+	test_cmp_config -C worktree-config true core.sparseCheckoutCone &&
+	test_must_fail git -C worktree-config2 core.sparseCheckoutCone
 '
 
 test_expect_success 'set sparse-checkout using builtin' '
@@ -202,6 +241,7 @@  test_expect_success 'add to sparse-checkout' '
 '
 
 test_expect_success 'cone mode: match patterns' '
+	git -C repo worktree init-worktree-config &&
 	git -C repo config --worktree core.sparseCheckoutCone true &&
 	rm -rf repo/a repo/folder1 repo/folder2 &&
 	git -C repo read-tree -mu HEAD 2>err &&
@@ -241,7 +281,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
 	)
 '
 
@@ -380,6 +420,7 @@  test_expect_success 'fail when lock is taken' '
 '
 
 test_expect_success '.gitignore should not warn about cone mode' '
+	git -C repo worktree init-worktree-config &&
 	git -C repo config --worktree core.sparseCheckoutCone true &&
 	echo "**/bin/*" >repo/.gitignore &&
 	git -C repo reset --hard 2>err &&