[v5,4/5] sparse-checkout: set worktree-config correctly

Commit Message

Derrick Stolee Jan. 31, 2022, 3 p.m. UTC

From: Derrick Stolee <[e-mail]>

The previous change added repo_config_set_worktree_gently() to assist
writing config values into the config.worktree file, if enabled. An
earlier change added init_worktree_config() as a helper to initialize
extensions.worktreeConfig if not already enabled.

Let the sparse-checkout builtin use these helpers 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, since this is not
    a requirement for understanding extensions.worktreeConfig.

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

Reported-by: Sean Allred <[e-mail]>
Helped-by: Eric Sunshine <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/git-sparse-checkout.txt | 16 +++++++++++----
 builtin/sparse-checkout.c             | 28 +++++++++++++--------------
 sparse-index.c                        | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh    |  4 ++--
 4 files changed, 30 insertions(+), 28 deletions(-)

Comments

Eric Sunshine Feb. 6, 2022, 10:21 a.m. UTC | #1

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the config.worktree file, if enabled. An
> earlier change added init_worktree_config() as a helper to initialize
> extensions.worktreeConfig if not already enabled.
>
> Let the sparse-checkout builtin use these helpers 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, since this is not
>     a requirement for understanding extensions.worktreeConfig.
>
>  2. If the main worktree is bare, then this command will not put the
>     worktree in a broken state.

Although the three of four of us involved in this discussion
understand what "broken state" means, such a description may be too
vague for future readers. To help them out, perhaps we can do a better
job of conveying the nature of the actual breakage by rewriting all of
the above like this:

    `git sparse-checkout set/init` enables worktree-specific
    configuration[*] by setting extensions.worktreeConfig=true, but
    neglects to perform the additional necessary bookkeeping of
    relocating `core.bare=true` and `core.worktree` from
    $GIT_COMMON_DIR/config to $GIT_COMMON_DIR/config.worktree, as
    documented in git-worktree.txt. As a result of this oversight,
    these settings, which are nonsensical for secondary worktrees, can
    cause Git commands to incorrectly consider a worktree bare (in the
    case of `core.bare`) or operate on the wrong worktree (in the case
    of `core.worktree`). Fix this problem by taking advantage of the
    recently-added init_worktree_config() which enables
    `extensions.worktreeConfig` and takes care of necessary
    bookkeeping.

    While at it, for backward-compatibility reasons, also stop
    upgrading the repository format to "1" since doing so is
    (unintentionally) not required to take advantage of
    `extensions.worktreeConfig`, as explained by 11664196ac ("Revert
    "check_repository_format_gently(): refuse extensions for old
    repositories"", 2020-07-15).

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

Perhaps this paragraph can become the "[*]" footnote I referenced in
the above rewrite.

> Reported-by: Sean Allred <[e-mail]>
> Helped-by: Eric Sunshine <[e-mail]>
> Signed-off-by: Derrick Stolee <[e-mail]>

Patch

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index b81dbe06543..94dad137b96 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -31,13 +31,21 @@  COMMANDS
 	Describe the patterns in the sparse-checkout file.
 
 'set'::
-	Enable the necessary config settings
-	(extensions.worktreeConfig, core.sparseCheckout,
-	core.sparseCheckoutCone) if they are not already enabled, and
-	write a set of patterns to the sparse-checkout file from the
+	Enable the necessary sparse-checkout config settings
+	(`core.sparseCheckout`, `core.sparseCheckoutCone`, and
+	`index.sparse`) if they are not already set to the desired values,
+	and write a set of patterns to the sparse-checkout file from the
 	list of arguments following the 'set' subcommand. Update the
 	working directory to match the new patterns.
 +
+To ensure that adjusting the sparse-checkout settings within a worktree
+does not alter the sparse-checkout settings in other worktrees, the 'set'
+subcommand will upgrade your repository config to use worktree-specific
+config if not already present. The sparsity defined by the arguments to
+the 'set' subcommand are stored in the worktree-specific sparse-checkout
+file. See linkgit:git-worktree[1] and the documentation of
+`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
++
 When the `--stdin` option is provided, the patterns are read from
 standard in as a newline-delimited list instead of from the arguments.
 +
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 679c1070368..314c8d61f80 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -15,6 +15,7 @@ 
 #include "wt-status.h"
 #include "quote.h"
 #include "sparse-index.h"
+#include "worktree.h"
 
 static const char *empty_base = "";
 
@@ -359,26 +360,23 @@  enum sparse_checkout_mode {
 
 static int set_config(enum sparse_checkout_mode mode)
 {
-	const char *config_path;
-
-	if (upgrade_repository_format(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"));
+	/* Update to use worktree config, if not already. */
+	if (init_worktree_config(the_repository)) {
+		error(_("failed to initialize worktree config"));
 		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 (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;
 
 	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 42776984fe7..be6ea4ffe33 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -117,7 +117,7 @@  test_expect_success 'switching to cone mode with non-cone mode patterns' '
 		cd bad-patterns &&
 		git sparse-checkout init &&
 		git sparse-checkout add dir &&
-		git config core.sparseCheckoutCone true &&
+		git config --worktree core.sparseCheckoutCone true &&
 		test_must_fail git sparse-checkout add dir 2>err &&
 		grep "existing sparse-checkout patterns do not use cone mode" err
 	)
@@ -256,7 +256,7 @@  test_expect_success 'sparse-index enabled and disabled' '
 		test_cmp expect actual &&
 
 		git -C repo config --list >config &&
-		! grep index.sparse config
+		test_cmp_config -C repo false index.sparse
 	)
 '