Sparse checkout: fix bug with worktree of bare repo

This patch series includes a fix to the bug reported by Sean Allred [1] and diagnosed by Eric Sunshine [2].

The root cause is that 'git sparse-checkout init' writes to the worktree config without checking that core.bare might need to be set. This only matters when the base repository is bare, since creating the config.worktree file and enabling extensions.worktreeConfig will cause Git to treat the base repo's core.bare=false as important for this worktree.

This series fixes this, but also puts in place some helpers to prevent this from happening in the future. While here, some of the config paths are modified to take a repository struct.

The critical bits are in Patches 3 and 4 which introduce the helper and then consume it in builtin/sparse-checkout.c and sparse-index.c.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/

Thanks, -Stolee

Derrick Stolee (4):
setup: use a repository when upgrading format
config: make some helpers repo-aware
config: add repo_config_set_worktree_gently()
sparse-checkout: use repo_config_set_worktree_gently()

 builtin/sparse-checkout.c          | 25 +++++--------
 config.c                           | 56 ++++++++++++++++++++++++++++--
 config.h                           | 13 +++++++
 list-objects-filter-options.c      |  2 +-
 repository.h                       |  2 +-
 setup.c                            |  6 ++--
 sparse-index.c                     | 10 ++----
 t/t1091-sparse-checkout-builtin.sh | 14 +++++++-
 8 files changed, 95 insertions(+), 33 deletions(-)

base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

gitgitgadget

From: Derrick Stolee <[email protected]>

The upgrade_repository_format() helper previously was not aware of the possibility of multiple repositories. Add a 'struct repository *' parameter so it is possible to call it from a specific repository.

The implementation already referred to the_repository in one place, so that is an easy replacement. The use of git_config_set() is replaced with a call to repo_config_set().

---
 builtin/sparse-checkout.c     | 2 +-
 list-objects-filter-options.c | 2 +-
 repository.h                  | 2 +-
 setup.c                       | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

index d0f5c4702be..34447f87cd8 100644
@@ -358,7 +358,7 @@ static int set_config(enum sparse_checkout_mode mode)
 {
 	const char *config_path;
 
-	if (upgrade_repository_format(1) < 0)
+	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"));

index fd8d59f653a..6e21d12045e 100644
@@ -372,7 +372,7 @@ void partial_clone_register(
 			 */
 			return;
 	} else {
-		if (upgrade_repository_format(1) < 0)
+		if (upgrade_repository_format(the_repository, 1) < 0)
 			die(_("unable to upgrade repository format to support partial clone"));
 
 		/* Add promisor config for the remote */

index 98f95834706..d3fc1f7689d 100644
@@ -215,6 +215,6 @@ void prepare_repo_settings(struct repository *r);
  * Return 1 if upgrade repository format to target_version succeeded,
  * 0 if no upgrade is necessary, and -1 when upgrade is not possible.
  */
-int upgrade_repository_format(int target_version);
+int upgrade_repository_format(struct repository *, int target_version);
 
 #endif /* REPOSITORY_H */

index 347d7181ae9..90516664ce5 100644
@@ -595,14 +595,14 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 	return 0;
 }
 
-int upgrade_repository_format(int target_version)
+int upgrade_repository_format(struct repository *r, int target_version)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 	struct strbuf repo_version = STRBUF_INIT;
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 
-	strbuf_git_common_path(&sb, the_repository, "config");
+	strbuf_git_common_path(&sb, r, "config");
 	read_repository_format(&repo_fmt, sb.buf);
 	strbuf_release(&sb);
 

@@ -621,7 +621,7 @@ int upgrade_repository_format(int target_version)
 			     repo_fmt.unknown_extensions.items[0].string);
 
 	strbuf_addf(&repo_version, "%d", target_version);
-	git_config_set("core.repositoryformatversion", repo_version.buf);
+	repo_config_set(r, "core.repositoryformatversion", repo_version.buf);
 	strbuf_release(&repo_version);
 	return 1;
 }

gitgitgadget

From: Derrick Stolee <[email protected]>

As we prepare to add new config helpers to write into a config.worktree, let's make some existing methods be available for writing to a config file relative to a repository.

---
 config.c | 29 ++++++++++++++++++++++++++---
 config.h |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

index c5873f3a706..9c9eef16018 100644
@@ -2882,7 +2882,12 @@ int git_config_set_gently(const char *key, const char *value)
 
 void git_config_set(const char *key, const char *value)
 {
-	git_config_set_multivar(key, value, NULL, 0);
+	repo_config_set(the_repository, key, value);
+}
+
+void repo_config_set(struct repository *r, const char *key, const char *value)
+{
+	repo_config_set_multivar(r, key, value, NULL, 0);
 
 	trace2_cmd_set_config(key, value);
 }

@@ -3177,14 +3182,32 @@ void git_config_set_multivar_in_file(const char *config_filename,
 int git_config_set_multivar_gently(const char *key, const char *value,
 				   const char *value_pattern, unsigned flags)
 {
-	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
+	return repo_config_set_multivar_gently(the_repository, key, value,
+					       value_pattern, flags);
+}
+
+int repo_config_set_multivar_gently(struct repository *r, const char *key,
+				    const char *value,
+				    const char *value_pattern, unsigned flags)
+{
+	return git_config_set_multivar_in_file_gently(repo_git_path(r, "config"),
+						      key, value, value_pattern,
 						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
 			     const char *value_pattern, unsigned flags)
 {
-	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
+	repo_config_set_multivar(the_repository, key, value,
+				 value_pattern, flags);
+}
+
+void repo_config_set_multivar(struct repository *r, const char *key,
+			      const char *value, const char *value_pattern,
+			      unsigned flags)
+{
+	git_config_set_multivar_in_file(repo_git_path(r, "config"),
+					key, value, value_pattern,
 					flags);
 }
 

index f119de01309..5531fc018e3 100644
@@ -258,6 +258,11 @@ int git_config_set_gently(const char *, const char *);
  */
 void git_config_set(const char *, const char *);
 
+/**
+ * write config values to `.git/config`, takes a key/value pair as parameter.
+ */
+void repo_config_set(struct repository *, const char *, const char *);
+
 int git_config_parse_key(const char *, char **, size_t *);
 
 /*

@@ -281,6 +286,8 @@ int git_config_parse_key(const char *, char **, size_t *);
 
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
+int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
+void repo_config_set_multivar(struct repository *, const char *, const char *, const char *, unsigned);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**

gitgitgadget

From: Derrick Stolee <[email protected]>

When adding config values to the worktree config, we might enable the extensions.worktreeConfig setting and create the config.worktree file for the first time. When the base repository is bare, this creates a change of behavior for determining if the worktree is bare or not. A worktree off of a bare repository is assumed to be non-bare when extensions.worktreeConfig is disabled. When extensions.worktreeConfig is enabled but config.worktree is empty, the worktree is considered bare because the base repo's core.bare=true setting is used.

To avoid issues like this, create a helper that initializes all the right settings in the correct order. A caller will be added in the next change.

---
 config.c | 27 +++++++++++++++++++++++++++
 config.h |  6 ++++++
 2 files changed, 33 insertions(+)

index 9c9eef16018..67f3d5015ef 100644
@@ -2880,6 +2880,33 @@ int git_config_set_gently(const char *key, const char *value)
 	return git_config_set_multivar_gently(key, value, NULL, 0);
 }
 
+int repo_config_set_worktree_gently(struct repository *r,
+				    const char *key, const char *value)
+{
+	int res;
+	const char *config_filename = repo_git_path(r, "config.worktree");
+
+	/*
+	 * Ensure that core.bare reflects the current worktree, since the
+	 * logic for is_bare_repository() changes if extensions.worktreeConfig
+	 * is disabled.
+	 */
+	if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
+							  r->worktree ? "false" : "true",
+							  NULL, 0))) {
+		error(_("unable to set core.bare setting in worktree config"));
+		return res;
+	}
+	if (upgrade_repository_format(r, 1) < 0)
+		return error(_("unable to upgrade repository format to enable worktreeConfig"));
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
+		error(_("failed to set extensions.worktreeConfig setting"));
+		return res;
+	}
+
+	return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
+}
+
 void git_config_set(const char *key, const char *value)
 {
 	repo_config_set(the_repository, key, value);

index 5531fc018e3..d8db420849e 100644
@@ -253,6 +253,12 @@ void git_config_set_in_file(const char *, const char *, const char *);
 
 int git_config_set_gently(const char *, const char *);
 
+/**
+ * Write a config value into the config.worktree file for the current
+ * worktree. This will initialize extensions.worktreeConfig if necessary.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
+
 /**
  * write config values to `.git/config`, takes a key/value pair as parameter.
  */

gitgitgadget

...

Sorry, but I'm not following what you're suggesting, and I'm not sure what you mean by "core config file" and "base repo's config.worktree file". Also, we aren't specifically concerned that core.bare=false.

Conceptually the proper fix is quite simple. (Whether the actual implementation is simple is a different question; I haven't looked closely at the code yet to be able to answer that.) First, though, let's make clear what different config files are involved:

.git/config -- config shared by the repository and all worktrees (including the main worktree)

.git/config.worktree - config specific to the main worktree (or to the repository itself if bare)

.git/worktrees/<id>/config.worktree -- config specific to worktree <id>

In the above list, I'm using ".git/" loosely to mean either a bare repository (i.e. "bare.git") or the ".git/" directory within the main worktree; the difference is immaterial to this discussion. When extensions.worktreeConfig is false or unset, only the first item in the above list is consulted; when extensions.worktreeConfig is true, then the config.worktree files are consulted, as well (depending upon which worktree you're in).

Regarding the actual "fix": we want a new utility function which enables per-worktree configuration and handles all the required bookkeeping actions described in git-worktree.txt. Specifically, if per-worktree configuration is not already enabled, the function will need to:

(1) set extensions.worktreeConfig=true in .git/config

(1) relocate core.bare from .git/config to .git/config.worktree if that key exists

(2) relocate core.worktree from .git/config to .git/config.worktree if that key exists

That's it. It doesn't need to create or touch any .git/worktrees/<id>/config.worktree file(s); it should _not_ add a core.bare=false to .git/worktrees/<id>/config.worktree, as this v1 patch series does.

A couple additional notes:

First, I can't count to three.

Second, item (0) in the above list would be to upgrade the repository to version 1 since that's a prerequisite of using extensions (which you know already, but I want to be clear for any other readers that the new utility function should perform this step, as well).

You are describing (in better detail) what I meant in my message about what needs to change in this patch.

Yes, the current patch is incorrect. However, changing just that one aspect of this patch in the current method (in config.c) should make it behave the way you are advocating.

I should have a v2 up later today and we can talk in more specifics about that if you want to wait until then.

Thanks,
-Stolee

As discussed already in [1], [2], and [3], the solution implemented by this patch is undesirable, and I gave an outline in [4] about how I think the new utility function ought to be implemented instead, so I won't say anything further about that here. However, I do still have one or two review comments to make about the general approach taken by patch. See below...

[1]: https://lore.kernel.org/git/CAPig+cQPUe9REf+wgVNjyak_nk3V361h-48rTFgk6TGC7vJgOA@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cTVzMtiHzkJq7VRg4Xa3xhrq7KKCdK5OSDY6bvwKu_ynA@mail.gmail.com/
[3]: https://lore.kernel.org/git/[email protected]/
[4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/

    enable_per_worktree_config(r);
    config_path = repo_git_path(r, "config.worktree")
    git_config_set_in_file_gently(config_path, "core.sparseCheckout", ...);
    git_config_set_in_file_gently(config_path, "core.sparseCheckoutCone", ...);

I understand your desire to separate these concerns, and maybe there is value in having another method that _just_ does the "upgrade to worktree config". However, if we don't also create this helper method for setting worktree-specific config, then we are going to hit this issue again.

If we expect every caller that assigns config to the worktree to follow this sequence of events, then we should encapsulate that in a method so developers can discover it and call it instead of needing to write these lines over again. Just repeating the literal "config.worktree" in multiple places is enough justification for making a helper, let alone these more subtle issues around bare repos and non-bare worktrees.

The helper method will need clear documentation to say "this will upgrade the repository format and add extensions.worktreeConfig" so those new consumers are aware of the full functionality.

Thanks,
-Stolee

There are several good reasons for having a single-purpose function which upgrades to per-worktree config. Not only is it easier to discover such a function, but it is also easier to reason about the behavior when it does just this one thing. Moreover, aside from providing a common implementation for modules which may want or require the functionality (such as git sparse-checkout init), it would form a solid basis for a git-worktree command for enabling per-worktree configuration. And, such a command could be valuable for people who want to utilize per-worktree configuration for their own reasons (not related to git-sparse-checkout).

With only git sparse-checkout init wanting to write per-worktree config, thus far, it does not feel like a convincing argument that an automagical helper function which conflates upgrading the repository to per-worktree config _and_ writing a per-worktree config key is a good idea or that it will be needed again. But more on that below...

On the contrary, because it is such an unusual and potentially dangerous step to take (i.e. it changes the repository in a way which third-party tools may not understand), any module which absolutely _requires_ per-worktree config support should be enabling it explicitly rather than expecting it to happen implicitly and magically. By keeping these concerns separate, it is not only easier for people working on this code in the future to reason about the behavior, but it also provides a cleaner path for electively aborting the operation should that ever become desirable. For instance:

% git sparse-checkout init
WARNING: This operation will upgrade the repository format to
WARNING: version 1 and enable per-worktree configuration, thus
WARNING: potentially making the repository incompatible with
WARNING: third-party tools.

Are you sure you want to continue [y/N]?

Your response is also conflating the slight pain of repeated repo_git_path(r, "config.worktree") with the need to upgrade to per-worktree configuration, which highlights another issue...

The big question is: why does git-sparse-checkout mandate per-worktree configuration? I haven't followed sparse checkout development closely, so I may be missing some obvious reasons. I can see why you would want to _recommend_ and even nudge people to use per-worktree configuration, which you could do both in the documentation and even as a "HINT" from the git sparse-checkout init command, but absolutely forcing them into per-worktree configuration seems far too opinionated for a general-purpose Git command and closes the door unnecessarily on people who may have quite valid reasons for using sparse checkout _without_ per-worktree configuration (i.e. perhaps they only ever use a single worktree -- the main one -- or perhaps they really do want the sparse checkout to apply to every worktree). This unconditional enforcement of per-worktree config seems better suited for scalar which is intentionally opinionated.

With the view that git sparse-checkout init is too opinionated and closes doors unnecessarily, then git sparse-checkout init should not be upgrading the repository to per-worktree configuration unconditionally. Instead, either the documentation should recommend this step to users; for example:

It is recommended that sparse checkout be used with per-worktree configuration so that each worktree can have its own sparse settings. Per-worktree configuration can be enabled with the (fictitious) git worktree config --enable-per-worktree command:

% git worktree config --enable-per-worktree
% git sparse-checkout init

Or, enabling per-worktree configuration could be enabled _on-demand_ by git sparse-checkout init; for instance:

    % git sparse-checkout init --per-worktree

Although the immediate discussion is about git-sparse-checkout, this idea that a command adapts to the repository rather than demanding a specific repository arrangement, or indeed forcibly changing the repository arrangement, is far friendlier and leaves doors open which would otherwise be closed.

It also makes your proposed repo_config_set_worktree_gently() which "does the right thing" much more palatable since "does the right thing" no longer means forcibly changing the repository arrangement. Instead, this convenience function would simply pick the correct configuration file on behalf of the caller; namely, if extensions.worktreeConfig is disabled, then it writes to .git/config, whereas if extensions.worktreeConfig is enabled, then it writes to .git/config.worktree or .git/worktrees/<id>/config.worktree, depending upon the worktree you're in. That behavior would satisfy your desire to have a convenience function to modify the correct config file regardless of whether the repository has per-worktree configuration or not, and leaves git-sparse-checkout flexible enough to work with or without per-worktree configuration. I would have no problem with a repo_config_set_worktree_gently() function which works as described here, whereas I feel plenty uncomfortable with the repo_config_set_worktree_gently() implemented by this series.

Referring back to what you said earlier:

However, if we don't also create this helper method for setting worktree-specific config, then we are going to hit this issue again. (Stolee)

Yes, we might hit this issue in the future if some command absolutely requires per-worktree config, however, as outlined above, I think we should strive as much as possible to make commands work without requiring special repository arrangement, instead allowing people to opt-in to repository-wide changes. By avoiding unconditionally requiring the repository be configured in a particular way, we're less likely to break third-party tools.

From: Derrick Stolee <[email protected]>

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.

---
 builtin/sparse-checkout.c          | 25 ++++++++-----------------
 sparse-index.c                     | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh | 14 +++++++++++++-
 3 files changed, 24 insertions(+), 25 deletions(-)

index 34447f87cd8..ec2c9a146cc 100644
@@ -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;
 }

index a1d505d50e9..e93609999e0 100644
@@ -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;

index 272ba1b566b..3ff125c5fd6 100755
@@ -71,6 +71,18 @@ 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_cmp_config false core.bare &&
+		git sparse-checkout set /*
+	)
+'
+
 test_expect_success 'git sparse-checkout list after init' '
 	git -C repo sparse-checkout list >actual &&
 	cat >expect <<-\EOF &&

@@ -219,7 +231,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
 	)
 '
 

gitgitgadget

Thanks for jumping on this so quickly. Unfortunately, however, as mentioned in [1] and [2], I think the approach implemented here of setting core.bare=false in the worktree-specific config is fundamentally flawed since it only addresses the problem for worktrees in which git sparse-checkout init has been run, but leaves all other worktrees potentially broken (both existing and new worktrees). As far as I can see, the _only_ correct solution is for the new helper function to enable extensions.worktreeConfig _and_ relocate core.bare and core.worktree from .git/config to .git/worktree.config, thus implementing the requirements documented in git-worktree.txt.

I also raised a separate question in [2] about whether git sparse-checkout init or the new helper function should be warning the user that upgrading the repository format and setting extensions.worktreeConfig might break third-party tools. However, that question is tangential to the fix being addressed here and doesn't need to be addressed by this series.

[1]: https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cQPUe9REf+wgVNjyak_nk3V361h-48rTFgk6TGC7vJgOA@mail.gmail.com/

Thanks for clarifying what I had misread. I commented on Patch 3 at the place that should be changed to relocate the setting. The test in patch 4 could have multiple worktrees to verify that it works.

I'll plan on providing a v2 with that change tomorrow, leaving time to find any other glaring errors.

Let's continue to simmer on this one. If there is a clear direction for doing this (should it just be an advice message?) then we can do that whenever.

Thanks,
-Stolee

I sent several pages worth of response to patch [3/4] because (apparently) I don't know how to be laconic.

Let's make sure we agree on the proper approach and solution before firing off v2.

Indeed, no hurry on this one. It's entirely tangential to the present patch series, and requires discussion and thought; it can be tackled later (if at all).

This patch series includes a fix to the bug reported by Sean Allred [1] and diagnosed by Eric Sunshine [2].

The root cause is that 'git sparse-checkout init' writes to the worktree config without checking that core.bare might need to be set. This only matters when the base repository is bare, since creating the config.worktree file and enabling extensions.worktreeConfig will cause Git to treat the base repo's core.bare=false as important for this worktree.

This series fixes this, but also puts in place some helpers to prevent this from happening in the future. While here, some of the config paths are modified to take a repository struct.

The critical bits are in Patches 3, 4, and 5 which introduce a helper for upgrading to worktree config, a helper to write to worktree config, and then consume that config helper in builtin/sparse-checkout.c and sparse-index.c.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/

Update in v2
============

* Eric correctly pointed out that I was writing core.bare incorrectly. It should move out of the core config and into the core repository's worktree config.
* Patch 3 is new, separating the "upgrade" logic out of config.c, but it is still called by the config helper to make it painless to write worktree config.

Thanks, -Stolee

Derrick Stolee (5):
setup: use a repository when upgrading format
config: make some helpers repo-aware
worktree: add upgrade_to_worktree_config()
config: add repo_config_set_worktree_gently()
sparse-checkout: use repo_config_set_worktree_gently()

 builtin/sparse-checkout.c          | 25 +++++-----------
 config.c                           | 39 +++++++++++++++++++++++--
 config.h                           | 14 +++++++++
 list-objects-filter-options.c      |  2 +-
 repository.h                       |  2 +-
 setup.c                            |  6 ++--
 sparse-index.c                     | 10 ++-----
 t/t1091-sparse-checkout-builtin.sh | 16 +++++++++-
 worktree.c                         | 47 ++++++++++++++++++++++++++++++
 worktree.h                         | 12 ++++++++
 10 files changed, 140 insertions(+), 33 deletions(-)

base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v1:

 1:  28813703ff6 = 1:  889e69dc45d setup: use a repository when upgrading format
 2:  3b549770eb9 = 2:  3e01356815a config: make some helpers repo-aware
 -:  ----------- > 3:  ed8e2a7b19d worktree: add upgrade_to_worktree_config()
 3:  67993f6cff2 ! 4:  22896e9bb04 config: add repo_config_set_worktree_gently()

@@ Metadata
## Commit message ##
config: add repo_config_set_worktree_gently()

- When adding config values to the worktree config, we might enable the
- extensions.worktreeConfig setting and create the config.worktree file
- for the first time. When the base repository is bare, this creates a
- change of behavior for determining if the worktree is bare or not. A
- worktree off of a bare repository is assumed to be non-bare when
- extensions.worktreeConfig is disabled. When extensions.worktreeConfig is
- enabled but config.worktree is empty, the worktree is considered bare
- because the base repo's core.bare=true setting is used.
+ The previous change added upgrade_to_worktree_config() to assist
+ creating a worktree-specific config for the first time. However, this
+ requires every config writer to care about that upgrade before writing
+ to the worktree-specific config. In addition, callers need to know how
+ to generate the name of the config.worktree file and pass it to the
+ config API.

- To avoid issues like this, create a helper that initializes all the
- right settings in the correct order. A caller will be added in the next
- change.
+ To assist, create a new repo_config_set_worktree_gently() method in the
+ config API that handles the upgrade_to_worktree_config() method in
+ addition to assigning the value in the worktree-specific config. This
+ will be consumed by an upcoming change.

## config.c ##
+@@
+ #include "dir.h"
+ #include "color.h"
+ #include "refs.h"
++#include "worktree.h"
+
+ struct config_source {
+ struct config_source *prev;
@@ config.c: int git_config_set_gently(const char *key, const char *value)
return git_config_set_multivar_gently(key, value, NULL, 0);
}
@@ config.c: int git_config_set_gently(const char *key, const char *value)
+int repo_config_set_worktree_gently(struct repository *r,
+ const char *key, const char *value)
+{
-+ int res;
-+ const char *config_filename = repo_git_path(r, "config.worktree");
-+
-+ /*
-+ * Ensure that core.bare reflects the current worktree, since the
-+ * logic for is_bare_repository() changes if extensions.worktreeConfig
-+ * is disabled.
-+ */
-+ if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
-+ r->worktree ? "false" : "true",
-+ NULL, 0))) {
-+ error(_("unable to set core.bare setting in worktree config"));
-+ return res;
-+ }
-+ if (upgrade_repository_format(r, 1) < 0)
-+ return error(_("unable to upgrade repository format to enable worktreeConfig"));
-+ if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
-+ error(_("failed to set extensions.worktreeConfig setting"));
-+ return res;
-+ }
-+
-+ return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
++ return upgrade_to_worktree_config(r) ||
++ git_config_set_multivar_in_file_gently(
++ repo_git_path(r, "config.worktree"),
++ key, value, NULL, 0);
+}
+
void git_config_set(const char *key, const char *value)
@@ config.h: void git_config_set_in_file(const char *, const char *, const char *);

+/**
+ * Write a config value into the config.worktree file for the current
-+ * worktree. This will initialize extensions.worktreeConfig if necessary. ++ * worktree. This will initialize extensions.worktreeConfig if necessary, ++ * which might trigger some changes to the root repository's config file.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *); +
4: 6202f767f4a ! 5: 06457fafa78 sparse-checkout: use repo_config_set_worktree_gently()

     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'git sparse-checkout ini
      +	(
      +		cd worktree &&
      +		git sparse-checkout init &&
     -+		test_cmp_config false core.bare &&
     ++		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' '

gitgitgadget

From: Derrick Stolee <[email protected]>

The upgrade_repository_format() helper previously was not aware of the possibility of multiple repositories. Add a 'struct repository *' parameter so it is possible to call it from a specific repository.

The implementation already referred to the_repository in one place, so that is an easy replacement. The use of git_config_set() is replaced with a call to repo_config_set().

---
 builtin/sparse-checkout.c     | 2 +-
 list-objects-filter-options.c | 2 +-
 repository.h                  | 2 +-
 setup.c                       | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

index d0f5c4702be..34447f87cd8 100644
@@ -358,7 +358,7 @@ static int set_config(enum sparse_checkout_mode mode)
 {
 	const char *config_path;
 
-	if (upgrade_repository_format(1) < 0)
+	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"));

index fd8d59f653a..6e21d12045e 100644
@@ -372,7 +372,7 @@ void partial_clone_register(
 			 */
 			return;
 	} else {
-		if (upgrade_repository_format(1) < 0)
+		if (upgrade_repository_format(the_repository, 1) < 0)
 			die(_("unable to upgrade repository format to support partial clone"));
 
 		/* Add promisor config for the remote */

index 98f95834706..d3fc1f7689d 100644
@@ -215,6 +215,6 @@ void prepare_repo_settings(struct repository *r);
  * Return 1 if upgrade repository format to target_version succeeded,
  * 0 if no upgrade is necessary, and -1 when upgrade is not possible.
  */
-int upgrade_repository_format(int target_version);
+int upgrade_repository_format(struct repository *, int target_version);
 
 #endif /* REPOSITORY_H */

index 347d7181ae9..90516664ce5 100644
@@ -595,14 +595,14 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 	return 0;
 }
 
-int upgrade_repository_format(int target_version)
+int upgrade_repository_format(struct repository *r, int target_version)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 	struct strbuf repo_version = STRBUF_INIT;
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 
-	strbuf_git_common_path(&sb, the_repository, "config");
+	strbuf_git_common_path(&sb, r, "config");
 	read_repository_format(&repo_fmt, sb.buf);
 	strbuf_release(&sb);
 

@@ -621,7 +621,7 @@ int upgrade_repository_format(int target_version)
 			     repo_fmt.unknown_extensions.items[0].string);
 
 	strbuf_addf(&repo_version, "%d", target_version);
-	git_config_set("core.repositoryformatversion", repo_version.buf);
+	repo_config_set(r, "core.repositoryformatversion", repo_version.buf);
 	strbuf_release(&repo_version);
 	return 1;
 }

gitgitgadget

From: Derrick Stolee <[email protected]>

As we prepare to add new config helpers to write into a config.worktree, let's make some existing methods be available for writing to a config file relative to a repository.

---
 config.c | 29 ++++++++++++++++++++++++++---
 config.h |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

index c5873f3a706..9c9eef16018 100644
@@ -2882,7 +2882,12 @@ int git_config_set_gently(const char *key, const char *value)
 
 void git_config_set(const char *key, const char *value)
 {
-	git_config_set_multivar(key, value, NULL, 0);
+	repo_config_set(the_repository, key, value);
+}
+
+void repo_config_set(struct repository *r, const char *key, const char *value)
+{
+	repo_config_set_multivar(r, key, value, NULL, 0);
 
 	trace2_cmd_set_config(key, value);
 }

@@ -3177,14 +3182,32 @@ void git_config_set_multivar_in_file(const char *config_filename,
 int git_config_set_multivar_gently(const char *key, const char *value,
 				   const char *value_pattern, unsigned flags)
 {
-	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
+	return repo_config_set_multivar_gently(the_repository, key, value,
+					       value_pattern, flags);
+}
+
+int repo_config_set_multivar_gently(struct repository *r, const char *key,
+				    const char *value,
+				    const char *value_pattern, unsigned flags)
+{
+	return git_config_set_multivar_in_file_gently(repo_git_path(r, "config"),
+						      key, value, value_pattern,
 						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
 			     const char *value_pattern, unsigned flags)
 {
-	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
+	repo_config_set_multivar(the_repository, key, value,
+				 value_pattern, flags);
+}
+
+void repo_config_set_multivar(struct repository *r, const char *key,
+			      const char *value, const char *value_pattern,
+			      unsigned flags)
+{
+	git_config_set_multivar_in_file(repo_git_path(r, "config"),
+					key, value, value_pattern,
 					flags);
 }
 

index f119de01309..5531fc018e3 100644
@@ -258,6 +258,11 @@ int git_config_set_gently(const char *, const char *);
  */
 void git_config_set(const char *, const char *);
 
+/**
+ * write config values to `.git/config`, takes a key/value pair as parameter.
+ */
+void repo_config_set(struct repository *, const char *, const char *);
+
 int git_config_parse_key(const char *, char **, size_t *);
 
 /*

@@ -281,6 +286,8 @@ int git_config_parse_key(const char *, char **, size_t *);
 
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
+int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
+void repo_config_set_multivar(struct repository *, const char *, const char *, const char *, unsigned);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**

gitgitgadget

From: Derrick Stolee <[email protected]>

Some features, such as the sparse-checkout builtin, require using the worktree config extension. It might seem simple to upgrade the repository format and add extensions.worktreeConfig, and that is what happens in the sparse-checkout builtin.

Transitioning from one config file to multiple has some strange side-effects. In particular, if the base repository is bare and the worktree is not, Git knows to treat the worktree as non-bare as a special case when not using worktree config. Once worktree config is enabled, Git stops that special case since the core.bare setting could apply at the worktree config level. This opens the door for bare worktrees.

To help resolve this transition, create upgrade_to_worktree_config() to navigate the intricacies of this operation. In particular, we need to look for core.bare=true within the base config file and move that setting into the core repository's config.worktree file.

To gain access to the core repository's config and config.worktree file, we reference a repository struct's 'commondir' member. If the repository was a submodule instead of a worktree, then this still applies correctly.

---
 worktree.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 12 ++++++++++++
 2 files changed, 59 insertions(+)

index 2c155b10150..e8ddbe2bfcc 100644
@@ -5,6 +5,7 @@
 #include "worktree.h"
 #include "dir.h"
 #include "wt-status.h"
+#include "config.h"
 
 void free_worktrees(struct worktree **worktrees)
 {

@@ -830,3 +831,49 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 	*wtpath = path;
 	return 0;
 }
+
+int upgrade_to_worktree_config(struct repository *r)
+{
+	int res;
+	int bare = 0;
+	struct config_set cs = { 0 };
+	char *base_config_file = xstrfmt("%s/config", r->commondir);
+	char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, base_config_file);
+
+	/*
+	 * If the base repository is bare, then we need to move core.bare=true
+	 * out of the base config file and into the base repository's
+	 * config.worktree file.
+	 */
+	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
+		if ((res = git_config_set_in_file_gently(base_worktree_file,
+							"core.bare", "true"))) {
+			error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
+			goto cleanup;
+		}
+
+		if ((res = git_config_set_in_file_gently(base_config_file,
+							"core.bare", NULL))) {
+			error(_("unable to unset core.bare=true in '%s'"), base_config_file);
+			goto cleanup;
+		}
+	}
+	if (upgrade_repository_format(r, 1) < 0) {
+		res = error(_("unable to upgrade repository format to enable worktreeConfig"));
+		goto cleanup;
+	}
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
+		error(_("failed to set extensions.worktreeConfig setting"));
+		goto cleanup;
+	}
+
+cleanup:
+	git_configset_clear(&cs);
+	free(base_config_file);
+	free(base_worktree_file);
+	trace2_printf("returning %d", res);
+	return res;
+}

index 8b7c408132d..170b6b1e1f5 100644
@@ -182,4 +182,16 @@ void strbuf_worktree_ref(const struct worktree *wt,
 			 struct strbuf *sb,
 			 const char *refname);
 
+/**
+ * Upgrade the config of the current repository and its base (if different
+ * from this repository) to use worktree-config. This might adjust config
+ * in both repositories, including:
+ *
+ * 1. Upgrading the repository format version to 1.
+ * 2. Adding extensions.worktreeConfig to the base config file.
+ * 3. Moving core.bare=true from the base config file to the base
+ *    repository's config.worktree file.
+ */
+int upgrade_to_worktree_config(struct repository *r);
+
 #endif

gitgitgadget

It would be a good idea to drop the final sentence since there is no such thing as a bare worktree (either conceptually or practically), and end the first sentence at "case": i.e. "... stops that special case."

I'm not sure how much this commit message is going to help someone who did not participate in the discussion which led to this patch series. I think the entire commit message could be written more concisely like this:

worktree: add helper to upgrade repository to per-worktree config

Changing a repository to use per-worktree configuration is a somewhat involved manual process, as described in the git-worktree documentation, but it's easy to get wrong if the steps are not followed precisely, which could lead to odd anomalies such as Git thinking that a worktree is "bare" (which conceptually makes no sense). Therefore, add a utility function to automate the process of switching to per-worktree configuration for modules which require such configuration. (In the future, it may make sense to also expose this convenience as a git-worktree command to automate the process for end-users, as well.)

To upgrade the repository to per-worktree configuration, it performs these steps:

* enable extensions.worktreeConfig in .git/config

* relocate core.bare from .git/config to .git/config.worktree (if key exists)

* relocate core.worktree from .git/config to .git/config.worktree (if key exists)

It also upgrades the repository format to 1 if necessary since that is a prerequisite of using extensions.

Bare worktrees don't exist, that is correct. But if one existed it would be a directory where you could operate as if it is a bare repo, but it has its own HEAD different from the base repo's HEAD. Not sure why one would want it.

I guess the question is: what happens if someone writes core.bare=true into their worktree config? A question to resolve another day, perhaps.

Good suggestions to add the necessary context here. Thanks.

That would be good. I originally erred on the side of least complicated but slower because this is not run very often.

'repo_config' is too generic, because I want the worktree config for the "original" repo. I chose to call that the "base" repo and its worktree config. Shared_config is a good name, though.

Yes, but specific to the original/root/bare repo. I'm open to preferences here, but "repo" isn't specific enough.

It took me a long time to actually understand the purpose of core.worktree, since it seems in conflict with the 'git worktree' feature. Indeed, it is special-cased the same way core.bare is, so this relocation is required.

This order can still cause some issues, specifically the worktree will still think it is bare or the core.worktree value is incorrect, but that is less serious than a broken base repo.

Leftover debugging. Thanks for catching.

Thanks,
-Stolee

I'm not following. I also still don't know what "base repo" is or where two HEADs would arise.

There seems to be some terminology confusion or conflict at play here. We're dealing with only a single repository and zero or more worktrees, so I'm still having trouble understanding your references to "original repo" and "base repo", which seem to indicate multiple repositories.

There's only a single repository, so this should be clear, however, there appears to be some terminology mismatch. I'll enumerate a few items in an effort to clarify how I'm using the terminology...

.git/ -- the repository residing within the main worktree

bare.git/ -- a bare repository

.git/config -- configuration shared by the repository and all worktrees

bare.git/config -- configuration shared by the repository and all worktrees

.git/config.worktree -- configuration specific to the main worktree

bare.git/config.worktree -- configuration specific to the bare repository

.git/worktrees/<id>/config -- configuration specific to worktree <id>

bare.git/worktrees/<id>/config -- configuration specific to worktree <id>

Indeed, the situation is unfortunately confusing in this area. core.worktree predates multiple-worktree support (i.e. git-worktree) by quite a long time and is a mechanism for allowing the repository (.git/) to exist at a distinct location from the worktree (by which I mean "main worktree" since there was no such thing as a "linked worktree" at a time). git-worktree generalized the concept by making multiple worktrees first-class citizens, but core.worktree and GIT_WORKTREE still need to be supported for backward compatibility even though they conflict (or can conflict) rather badly with multiple-worktrees.

Your use of "main worktree" is what I am meaning. I will adopt your terminology.

Thanks,
-Stolee

Not a suggestion to rename the function, but does it mean "we have been using a single configuration for all worktrees attached to the repository, but we are switching to use per-worktree configuration file"? I am wondering if the phrase "worktree_config" is clear enough for our future developers.

		if (git_config_set_in_file_gently(...)) {
			res = error(_("..."));
                        goto cleanup;
		}

Deep down in the git_config_set_... stack, it returns helpful error codes that I thought would be good to communicate upwards. cmd_config() uses these error codes, too, but that's a more obvious place where the user is expecting the error code to be related to the config operation.

If this communication is not helpful, then I will use the pattern you suggest. I will assume this is the case unless told otherwise.

I don't think we should add burden to the callers to check that the repo is not using worktree config. Thinking back to your rename idea this could be "ensure_using_worktree_config()" to make it clear that we will use the worktree config after the method, whether or not we were using it beforehand.

I think this current implementation is non-damaging if someone calls it after already using worktree config. The change being that a user can go and add core.bare=false to the common config file and break all worktrees again, and the current implementation will help heal that. It's probably better to let the user have that ability to mess things up and not try to fix something so broken.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

The previous change added upgrade_to_worktree_config() to assist creating a worktree-specific config for the first time. However, this requires every config writer to care about that upgrade before writing to the worktree-specific config. In addition, callers need to know how to generate the name of the config.worktree file and pass it to the config API.

To assist, create a new repo_config_set_worktree_gently() method in the config API that handles the upgrade_to_worktree_config() method in addition to assigning the value in the worktree-specific config. This will be consumed by an upcoming change.

---
 config.c | 10 ++++++++++
 config.h |  7 +++++++
 2 files changed, 17 insertions(+)

index 9c9eef16018..81f3a689c11 100644
@@ -21,6 +21,7 @@
 #include "dir.h"
 #include "color.h"
 #include "refs.h"
+#include "worktree.h"
 
 struct config_source {
 	struct config_source *prev;

@@ -2880,6 +2881,15 @@ int git_config_set_gently(const char *key, const char *value)
 	return git_config_set_multivar_gently(key, value, NULL, 0);
 }
 
+int repo_config_set_worktree_gently(struct repository *r,
+				    const char *key, const char *value)
+{
+	return upgrade_to_worktree_config(r) ||
+	       git_config_set_multivar_in_file_gently(
+			 repo_git_path(r, "config.worktree"),
+			 key, value, NULL, 0);
+}
+
 void git_config_set(const char *key, const char *value)
 {
 	repo_config_set(the_repository, key, value);

index 5531fc018e3..b05c51b3528 100644
@@ -253,6 +253,13 @@ void git_config_set_in_file(const char *, const char *, const char *);
 
 int git_config_set_gently(const char *, const char *);
 
+/**
+ * Write a config value into the config.worktree file for the current
+ * worktree. This will initialize extensions.worktreeConfig if necessary,
+ * which might trigger some changes to the root repository's config file.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
+
 /**
  * write config values to `.git/config`, takes a key/value pair as parameter.
  */

gitgitgadget

I still feel very uncomfortable about this function conflating two very different concerns (upgrading the repository to per-worktree config, and the simple setting of a config variable). Since I've already explained my discomfort and suggested alternatives several times during this discussion (most recently at [1]), I don't have all that much more to say. However, I do have a few comments...

First, I would have no problem if this function "did the right thing" where "the right thing" means correctly choosing between .git/config and .git/config.worktree depending upon whether or not extensions.worktreeConfig is set, and only that. It should not perform any sort of repository upgrade on its own. Doing it this way should satisfy your major concern of relieving callers from having to figure out the correct configuration file name.

Second, if you absolutely must have this function, as implemented, as part of the public API (despite my protests), please give it a name which is sufficiently different from the other "config setter" functions so that people won't be confused into thinking it's just another "setter" without realizing that calling it has repository-wide consequences.

Third, I don't have an objection if you want to make this function private (static) to builtin/sparse-checkout.c, thus omitting it from the public API. This way you can have its convenience where you want it, and we can delay finishing this discussion until such time that it becomes apparent that other modules want its convenience, as well, if that ever comes about.

[1]: https://lore.kernel.org/git/CAPig+cRombN-8g0t7Hs9qQypJoY41gK3+kvypH4D0G6EB4JgbQ@mail.gmail.com/

From: Derrick Stolee <[email protected]>

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.

---
 builtin/sparse-checkout.c          | 25 ++++++++-----------------
 sparse-index.c                     | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh | 16 +++++++++++++++-
 3 files changed, 26 insertions(+), 25 deletions(-)

index 34447f87cd8..ec2c9a146cc 100644
@@ -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;
 }

index a1d505d50e9..e93609999e0 100644
@@ -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;

index 272ba1b566b..b563ccfeb36 100755
@@ -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
 	)
 '
 

gitgitgadget

I'm having trouble understanding what this is trying to say. Did you mean "true" rather than "false" in the final sentence? Anyhow, I think this description is somewhat stale. A more succinct way to describe the issue is that git sparse-checkout init wasn't correctly upgrading the repo to per-worktree configuration, with the result that the core.bare=true setting of a bare repo bled into worktree-specific configuration, which caused a bit of havoc. This patch series fixes init to upgrade the repo properly.

It's good to see the "upgrade to per-worktree config" split out to a standalone single-purpose utility function. I left several review comments in that patch, the most important of which is that the implementation is incomplete (because it doesn't relocate core.worktree), thus it can leave the repo in an inconsistent and broken state. Several of the other review comments are actionable, as well.

I'm still concerned about and uncomfortable with the implementation of the new repo_config_set_worktree_gently(), but I've left ample comments about that elsewhere in this discussion, so I needn't go into it here.

Thanks for working on this.

Based on the description I went and fetched the patch series and tried it out.

This feels like a bandaid to me. In addition to fixating on core.bare (thus overlooking core.worktree), it also overlooks that people can use worktrees without using sparse-checkout. What if they do something like:

git clone --bare $URL myrepo
cd myrepo
git worktree add foo
git worktree add bar
git worktree add baz
... days/weeks later ...
cd foo
git config extensions.worktreeConfig true
git config status.showUntrackedFiles no # Or other config options ... hours/days later ..
cd ../bar
git status

At this point the user gets "fatal: this operation must be run in a work tree". And it's much, much worse if the original clone was not bare, but had core.worktree pointing somewhere else (because then the git status invocation will show the differences between the *other* worktree with the HEAD of *this* worktree). I think that "git worktree add" should check if either core.bare is false or core.worktree is set, and if so then set extensions.worktreeConfig and migrate the relevant config.

While there may be some concern about non-git clients not understanding extensions.worktreeConfig, I'd say that this type of situation is one where we are just flat broken without it. Without it, we're stuck in a situation that will: (a) confuse users ("Why does core.bare/core.worktree seem to get ignored or have incorrect values?"), (b) confuse non-git clients (are they really going to have the tricky logic to overrule core.bare/core.worktree when in another worktree?), (c) confuse git itself after subsequent configuration tweaks, and (d) (related to item c) lead to ever more complicated logic in core git to attempt to know when and how to overrule core.bare and core.worktree as additional concerns enter the picture (e.g. will someone contribute code to override core.bare/core.worktree when run from a worktree with extensions.worktreeConfig=true, just as someone originally wrote code to override core.bare/core.worktree when the extensions.worktreeConfig setting wasn't present?)

I also think git worktree add should handle additional configuration items related to sparse checkouts (as we've discussed elsewhere in the past), but that's going a bit outside the scope of this series; I only mention it so that we're aware the functionality added to git worktree add will be getting some additions in the future.

Your example indeed leads to a broken state because it doesn't follow the instructions given by git-worktree.txt for enabling extensions.worktreeConfig, which involves additional bookkeeping operations beyond merely setting that config variable. It is exactly this sort of situation which prompted me to suggest several times[1,2,3] in the conversation following my diagnosis of the problem, as well as in my reviews of this series, that we may want to add a git-worktree subcommand which does all the necessary bookkeeping to enable extensions.worktreeConfig rather than expecting users to handle it all manually. In [1], I called this hypothetical command git worktree manage --enable-worktree-config and in [4], I called it git worktree config --enable-per-worktree (not because I like either name, but because I couldn't think of anything better).

(I think you meant "...if either core.bare is _true_ or...".)

Similar to my response to Sean in [1] and to Stolee in [2], while this may help the situation for worktrees created _after_ extensions.worktreeConfig is enabled, it does _not_ help existing worktrees at all. For this reason, in my opinion, git worktree add is simply not the correct place to be addressing this problem, and it's why I suggested a separate command for enabling the feature and doing all the necessary bookkeeping. It's also why I suggested[2] that in the long run, we may want per-worktree config to be the default (and only) behavior rather than the current (legacy) behavior of all config being shared between worktrees.

Aside from that, I'm uncomfortable with the suggestion that git worktree add should be responsible for making these sort of dramatic changes (upgrading to version=1 and enabling extensions.worktreeConfig) to the repository automatically. That seems very much out of scope for what this command should be doing. On the other hand, I would have no problem with git worktree add protecting users by detecting whether core.bare=true or core.worktree is set in the shared .git/config file and aborting with an error if so, and giving a "HINT" telling the user to enable per-worktree config via the (hypothetical) git worktree config --enable-per-worktree command.

Regarding your feeling that this patch series is a "band-aid", while I agree with you that we ultimately need a better approach, such as the hypothetical git worktree config --enable-per-worktree (or eventually making per-worktree config be the default), that better solution does not need to be implemented today, and certainly shouldn't derail _this_ patch series which is aimed at fixing a very real bug which exists presently in git sparse-checkout init. This patch series does need a good number of improvements and fixes before it is ready -- as indicated by my v2 review comments[4,5,6], the most obvious of which is its missing handling of core.worktree -- but I do think this series is headed in the correct direction by focusing on fixing the immediate problem with git sparse-checkout init (and paving the way for an eventual more complete solution, such as git worktree config --enable-per-worktree ).

[1]: https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cQPUe9REf+wgVNjyak_nk3V361h-48rTFgk6TGC7vJgOA@mail.gmail.com/
[3]: https://lore.kernel.org/git/CAPig+cRombN-8g0t7Hs9qQypJoY41gK3+kvypH4D0G6EB4JgbQ@mail.gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cQrJ9yWjkc8VMu=uyx_qtrXdL3cNnxLVafoxOo6e-r9kw@mail.gmail.com/
[5]: https://lore.kernel.org/git/CAPig+cRi2SA6+poaemY8XR5ZoMweuztfiENpcRVOCnukg3Qa7w@mail.gmail.com/
[6]: https://lore.kernel.org/git/CAPig+cRuY40RNi4bC3CBfghLLqz74VUPRbaYJYEhmF78b0GfPQ@mail.gmail.com/#t

I vaguely recall some mention of this not long ago on the list but didn't follow the discussion at all. Do you have pointers or a summary?

Thinking upon it further, I take back what I said about it not helping existing worktrees.

Your proposal is _almost_ the same as my suggestion of eventually making per-worktree config the default. The difference is that you're only making it the default if core.bare=true or core.worktree is set. But do we need that distinction? If people are comfortable with that, then are they comfortable with simply flipping the switch and making per-worktree config the default today regardless of core.bare and core.worktree?

I'm not sure that I'm comfortable with it due to the potential of breaking older versions of git which don't understand extensions.worktreeConfig, as well as breaking third-party tools, but maybe other people feel differently?

Indeed. :-)

This is tempting, at least if we leave core.repositoryFormatVersion as 0 (see 11664196ac ("Revert "check_repository_format_gently(): refuse extensions for old repositories"", 2020-07-15)) when core.bare is false and core.worktree was unset. However, for that case:

* This is a case where operating on the primary worktree was not previously problematic for older git versions or third party tools.
* Interestingly, git <= 2.6.2 can continue to operate on the primary worktree (because it didn't know to error out on unknown extensions)
* git >= 2.19.0 could continue to operate on the primary worktree (because it understands the extension)
* git versions between that range would suddenly break, erroring out on the unknown extension (though those versions would start working again if we migrated core.bare and core.worktree but just didn't set extensions.worktreeConfig).

The distinction I made was particularly written with third party tools and older versions of git in mind, to allow them to continue to safely operate when possible. But let's flesh it out a little:

* core.bare = false AND core.worktree is unset (i.e. a typical non-bare clone), AND we try to add a worktree: we have _years_ of in-the-wild usage showing that old git versions and third party tools operate fine without migrating the config. Leave it in place and do not set extensions.worktreeConfig and do not upgrade core.repositoryFormatVersion.

* (core.bare = true OR core.worktree is set) AND we try to add a
worktree: all third party tools and all git versions (old and new) are broken anyway. Flip the switch (upgrade core.repositoryFormatVersion to 1, set extensions.worktreeConfig=true, and migrate core.bare/core.worktree for main repo and newly created worktree), which at least allows new git versions and new tools to work correctly, and will hopefully cause old tools to error out since this is a configuration they won't handle correctly.

Further:
* Toggling extensions.worktreeConfig=true for the first time is
rather trivial for users to try; for years they have been able to do so without making _any_ other changes and expect things to continue to work (assuming new enough git versions and third-party tools). They have likely disseminated this information to other users. I certainly did. If we are going to expect more of anyone toggling this option, we need lots of documentation and perhaps code changes to help shore up the path. I'd rather just allow folks to continue to do such toggling.
* Toggling either core.bare or core.worktree in an existing clone
requires significant additional manual work to make things consistent. Because it requires a lot more knowledge and work, I think the burden should be on these users to know about the ramifications with existing worktrees. (I've never heard of a user other than myself attempt to toggle these; I'm sure there are some, it just seems quite rare.)

I'll try to respond to this email when I can find a quiet block of time to really concentrate on what you're saying and reason through it more thoroughly; it will probably require several read-throughs.

I mentioned previously[1] that I needed to find a block of time to really think through the topic before I'd be able to respond to this email. So, today I spent some time trying to reason through the various cases under discussion, and I came back and re-read this email with the intention of trying to summarize my understanding of the situation and my understanding of the points you were making. However, you did such a good job of summarizing the various cases at the very end of [2] that it probably makes more sense for me to respond to that email instead.

[1]: https://lore.kernel.org/git/CAPig+cTFSDw-9Aq+=+r4sHSzTmG7s2T93Z0uqWTxHbKwGFaiYQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/CABPp-BHuO3B366uJuODMQo-y449p8cAMVn0g2MTcO5di3Xa7Zg@mail.gmail.com/

I had seen 11664196ac when researching one of my earlier responses, though it took more than one read to (hopefully) fully understand what it is saying (i.e. due to an oversight, it's too late to enforce the core.repositoryFormatVersion=1 requirement when extensions are used, as originally intended).

The significance of versions 2.6.2 and 2.19.0 is unclear to me. What context or criteria are you using to identify those versions as meaningful here?

We had been discussing how certain config settings "might break external tools OR old git versions", but hadn't brought up which tools or which git versions. I don't know which all external tools might be in play (though I mentioned some in use at $DAYJOB in another thread) but in this email I had just thought that I'd mention where the cutoff point was in terms of git versions which understood core.repositoryFormatVersion and extensions.worktreeConfig. If other folks also want to test how things behaved before or after these patches with "old git versions", those were the switchover points that are relevant and which I tested with.

These are instructions which neither Stolee nor I was aware of prior to your pointing it out. Not only had we often flipped that variable, we did so for many of our users (I know I did for some time prior to Stolee introducing sparse-checkout). With git sparse-checkout we propagated that to many more users, and now have many repositories out in the wild that have been set up for _years_ in violation of these instructions. So, even if Stolee and I are independently particularly bad about noticing the relevant documentation, we now have a situation where people can discover this misconfiguration just by looking around in their config. Once they notice it, they may well copy it elsewhere.

I'd suspect that Stolee and I are actually _more_ likely to be aware of relevant documentation than the average Git user, so if we missed it, I suspect many of them will. Especially now that we've amplified their opportunities for discovering repositories set up in contravention to that documentation.

So, I don't think relying on folks to read this particular piece of documentation is a reliable course of action...at least not without some changes to make it much more likely to be noticed.

How would users discover this new command and use it? Is it any more reliably discoverable than the above documentation?

Your suggestion sounds to me like "We know this command will break things, so we'll provide another command they can use to avoid the breakage, and hope they notice this new command and use it." I'm sure that's not your intent, and perhaps there's a way of making this suggestion robust, but to me it just sounds like it leads to inevitable breakage. I'd rather just fix the command that can break things.

Yes, indeed.

Looks like you've changed your opinion a bit and it'd be better for me to respond to these parts in your follow-up email.

For the microsoft repositories, sparse-checkouts are needed because a full checkout is unmanageable (millions of files to check out otherwise). For other repositories, full checkouts might technically be manageable, but are annoyingly slow and users may only want to work with sparse checkouts (and for some of these, due to various mono-repoization efforts, the repository is growing towards a size where manageability of full checkouts is decreasing).

The fact that git worktree add does a full checkout is quite painful...possibility to the point of making worktrees useless for some users. I think git worktree add should copy the sparsity of the worktree from which it was invoked.

Addressing potential questions/objections to this proposal:
* just requiring users to do a full checkout first is very
unfriendly: the checkout might not even fit in available disk space, and even if it does fit, this has the performance penalty of inflating and writing all files to disk only to delete a (vast?) majority of them immediately after. Users have shown a willingness to swallow a lot of pain trying to figure out how to avoid that performance penalty.
* full-checkout: If users do want a full checkout of the new
worktree despite running from a sparse-checkout, it's a single command away (git sparse-checkout disable or <sparsity-wrapper-script> --undo). And in that case, the invoked commands don't do huge amounts of unnecessary work.
* using --no-checkout as a proxy: This means no files checked out
and no index file. The lack of an index file makes it appear that everything was manually deleted (with the deletion staged). Also, if the project is using some kind of <sparsity-wrapper-script> (e.g. for determining dependencies between directories so that appropriate 'modules' can be specified and transformed into a list of directories passed to sparse-checkout), then the sparsity-wrapper-script isn't available to them to invoke. If users try to check out just the wrapper file, then an index will be created and have just one entry and we kind of cement the fact that all other files look like they were intended to be deleted. Also, even if the user runs git sparse-checkout init --cone, you don't actually don't transform this no-checkout into a sparse checkout because sparse-checkout doesn't want to undo your staged deletions. Despite the fact that I'm very familiar with all the implementation internals, it was not obvious to me all the necessary additional commands needed for users to get a sparse checkout while making use of --no-checkout. Users stand little chance of figuring the necessary command invocations out without a huge amount of effort (and they've given up and come to me before asking for help, and my first response turned out to be incomplete in various cases...).

I wasn't originally aware of the bookkeeping instructions either. In fact, for a good while, I wasn't even aware that Duy had implemented per-worktree configuration or that there was such a thing. I either must have been entirely off-list during the implementation or was not in a position to follow changes to the project at the time. I only became aware of per-worktree configuration at some point in the last two or three years when someone asked some question on the list related to the feature and I ended up diving into the documentation, the source code, and the patches themselves in order to understand it fully -- because I think I didn't understand it merely from reading the documentation which is rather sparse (no pun intended). And I had forgotten enough about it since then that I had to re-research it when Sean raised the issue on the list a few days back in relation to sparse-checkout.

The sparsity of documentation about per-worktree configuration certainly doesn't help, nor the fact that it's fairly near the end of git-worktree.txt, at which point people may have given up reading. We could make it a bit more prominent by mentioning it early in the command description, but it still involves enough fiddly bookkeeping that it likely will continue to be problematic.

Making per-worktree configuration the default does seem like the best long-term solution. Doing so should make all these problems go away. I don't know what Duy's plans were, nor whether he had some migration strategy planned.

Okay, I do recall reading a message in which you proposed this, though I didn't understand the reasoning for the suggestion since I wasn't following the discussion. The explanation you provide here makes the proposal understandable.

You've clearly put much more thought into this than I have (since I only just read this), so I'm not likely to have any meaningful input, but I'll write down a few thoughts/questions which popped into my head while reading what you wrote. Perhaps they've already been discussed elsewhere, so feel free to ignore (and they may not be worth responding to anyhow).

When you say "copy the sparsity of the worktree from which it was invoked", do you mean that literally, such that it special-cases it and only copies sparse-checkout information? An alternative would be to allow the user to specify -- via the shared configuration (.git/config) -- exactly which config keys get inherited/copied over by git worktree add. Such a solution would avoid special-casing sparse-checkout and could be useful in the future for other commands which might need such functionality, though this approach may be overengineered.

A more general approach might be for the new worktree to copy all the per-worktree configuration from the worktree in which the command was invoked, thus sparsity would be inherited "for free" along with other settings. This has the benefits of not requiring sparse-checkout special-cases in the code and it's easy to document ("the new worktree inherits/copies configuration settings from the worktree in which git worktree add was invoked") and easy to understand.

I also wondered if adding some sort of --sparse-checkout=... option to git worktree add would solve this particular dilemma, thus allowing the user to configure custom sparse-checkout for the worktree as it is being created. I also very briefly wondered if this should instead be a feature of the git sparse-checkout command itself, such as git sparse-checkout add-worktree, but I think that's probably a dead-end in terms of user discoverability, whereas git worktree add --sparse-checkout=... is more easily discoverable for people wanting to work with worktrees.

Further, it's not clear people even looked at git-worktree.txt at the time they learned about extensions.worktreeConfig. I believe I discovered and started using extensions.worktreeConfig from git config --help, which makes no mention of or even reference to the need for any extra steps. (I didn't see the original mailing list discussions around that setting either.) It never occurred to me in the ~3 years since to even look in git worktree --help for additional guidance around that config setting until this particular mailing list thread.

Ooh, this is a good point and I *really* like this simple solution. Thanks for pointing it out.

Do note, though, that it's more than just config.worktree -- I also want the ${GITDIR}/info/sparse-checkout file copied.

This might be a useful extra capability (we'd probably want to keep this flag in sync with git-clone's --sparse flag and whatever capabilities grow there), but I don't see it as a solution to this problem. I think the default needs to be copying the existing sparsity. Making users specify cone/non-cone mode and sparse-index/non-sparse-index and and several dozen directories by hand just doesn't sound reasonable to me. (We have a case with several hundred directories/modules, with various dependencies between them. Users can use a wrapper, ./sparsify --modules $MODULE_A $MODULE_B which figures out the several dozen relevant directories and calls sparse-checkout set with those, but of course that wrapper won't yet be available in the new worktree until after the new worktree has been added.)

An alternative that would be workable, though annoying, is giving the user a super-sparse checkout with only files in the toplevel directory present (i.e. what you'd get after git sparse-checkout init --cone or git clone --sparse ...), and then making them use the normal tools to manually specify the wanted sparsity (which probably requires switching back to the previous worktree to run some info commands to determine exactly what the sparsity was).

An increasingly unworkable alternative is the current behavior of defaulting to a full checkout in all cases (and forcing users to sparsify afterwards). A full checkout is fine if the user came from one (and probably preferable in such a case), but it's increasingly problematic for us even with our repo being nowhere near the size of the microsoft repos.

That's an interesting datapoint. At the very least, we probably should update Documentation/git-config.txt to mention the extra bookkeeping required when setting extensions.worktreeConfig=true.

I do wonder, though, if there are traps waiting for us with this all-inclusive approach. I don't know what sort of worktree-specific configuration people use, so I do worry a bit that this could be casting a too-wide net, and that it might in fact be better to only copy the sparse-checkout settings (as ugly as it is to special-case that -- but we need to special-case core.bare and core.worktree anyhow[1]).

[1]: https://lore.kernel.org/git/CAPig+cSUOknNC9GMyPvAqdBU0r1MVgvSpvgpSpXUmBm67HO7PQ@mail.gmail.com/

Thanks for pointing that out. I'm reasonably (or completely) ignorant of sparse-checkout since I've never used it nor read the documentation, and I didn't follow the earlier discussions.

Okay.

Sounds somewhat painful.

It feels unfortunate and a bit dirty to spread around this special-case knowledge about sparse-checkout to various parts of the system, but based upon the pain-points you describe, having a new worktree inherit the sparsity from the originating worktree does sound (given my limited knowledge of the topic) like it would ease the pain for users.

I could probably be persuaded either way (do users want to copy something and tweak it, or start with a clean slate?), and it might even make sense to have a flag for users to specify.

My hunch, at least with the developers I work with, is that they're more likely to think in terms of "I want another worktree like this one, except that I'm going to change..."

Also, another reason to prefer copying all of core.worktree (minus the always worktree-specific value of core.worktree, and core.bare), is because it's easy to explain in the documentation, and I think we'd be much less likely to obsolete user's knowledge over time. (I think additional sparse-checkout things, or new other features that also want to be copied over, are much more likely than the addition of keys that are always worktree-specific like core.worktree).

...

This might makes things worse, but it's far too late to avoid that:

$ git grep -il -e sparse-checkout -e skip_worktree
.gitignore
Documentation/RelNotes/2.19.0.txt
Documentation/RelNotes/2.25.0.txt
Documentation/RelNotes/2.26.0.txt
Documentation/RelNotes/2.27.0.txt
Documentation/RelNotes/2.28.0.txt
Documentation/RelNotes/2.34.0.txt
Documentation/RelNotes/2.6.0.txt
Documentation/config/checkout.txt
Documentation/config/core.txt
Documentation/git-add.txt
Documentation/git-checkout.txt
Documentation/git-clone.txt
Documentation/git-read-tree.txt
Documentation/git-restore.txt
Documentation/git-rm.txt
Documentation/git-sparse-checkout.txt
Documentation/git-worktree.txt
Documentation/gitrepository-layout.txt
Documentation/rev-list-options.txt
Documentation/technical/index-format.txt
Documentation/technical/parallel-checkout.txt
Documentation/technical/sparse-index.txt
Makefile
advice.c
apply.c
attr.c
builtin/add.c
builtin/check-ignore.c
builtin/checkout.c
builtin/clone.c
builtin/commit.c
builtin/grep.c
builtin/ls-files.c
builtin/mv.c
builtin/read-tree.c
builtin/rm.c
builtin/sparse-checkout.c
builtin/stash.c
builtin/update-index.c
cache-tree.c
cache.h
command-list.txt
contrib/completion/git-prompt.sh
diff-lib.c
diff.c
dir.c
dir.h
entry.c
git.c
list-objects-filter.c
list-objects-filter.h
merge-ort.c
merge-recursive.c
path.c
pathspec.c
pathspec.h
po/bg.po
po/ca.po
po/de.po
po/el.po
po/es.po
po/fr.po
po/git.pot
po/id.po
po/it.po
po/ko.po
po/pl.po
po/pt_PT.po
po/ru.po
po/sv.po
po/tr.po
po/vi.po
po/zh_CN.po
po/zh_TW.po
preload-index.c
read-cache.c
sparse-index.c
sparse-index.h
t/perf/p0005-status.sh
t/perf/p0006-read-tree-checkout.sh
t/perf/p0007-write-cache.sh
t/perf/p2000-sparse-operations.sh
t/perf/repos/inflate-repo.sh
t/perf/repos/many-files.sh
t/t0060-path-utils.sh
t/t1011-read-tree-sparse-checkout.sh
t/t1090-sparse-checkout-scope.sh
t/t1091-sparse-checkout-builtin.sh
t/t1092-sparse-checkout-compatibility.sh
t/t2018-checkout-branch.sh
t/t3507-cherry-pick-conflict.sh
t/t3602-rm-sparse-checkout.sh
t/t3705-add-sparse-checkout.sh
t/t5317-pack-objects-filter-objects.sh
t/t6112-rev-list-filters-objects.sh
t/t6428-merge-conflicts-sparse.sh
t/t6435-merge-sparse.sh
t/t7002-mv-sparse-checkout.sh
t/t7012-skip-worktree-writing.sh
t/t7063-status-untracked-cache.sh
t/t7418-submodule-sparse-gitmodules.sh
t/t7519-status-fsmonitor.sh
t/t7817-grep-sparse-checkout.sh
unpack-trees.c
wt-status.c

:-)

I also could probably be persuaded either way, and yes a flag is a possibility, though it would be nice if we could get along without it.

Another possible point in favor of copying all worktree-specific config to the new worktree is that if the user really does want to do some configuration specific to the new worktree, then that is going to require a certain amount of manual setup after creating the new worktree regardless of whether we copy all worktree-specific config or only a select subset (such as the sparse-checkout settings).

This series is based on a merge of en/sparse-checkout-set, en/worktree-chatty-to-stderr, and ds/sparse-checkout-malformed-pattern-fix.

This patch series includes a fix to the bug reported by Sean Allred [1] and diagnosed by Eric Sunshine [2].

The root cause is that 'git sparse-checkout init' writes to the worktree config without checking that core.bare might need to be set. This only matters when the base repository is bare, since creating the config.worktree file and enabling extensions.worktreeConfig will cause Git to treat the base repo's core.bare=false as important for this worktree.

This series fixes this, but also puts in place some helpers to prevent this from happening in the future. While here, some of the config paths are modified to take a repository struct.

* 'git sparse-checkout' will now modify the worktree config, if enabled. It will no longer auto-upgrade users into using worktree config.

* The new 'git worktree init-worktree-config' will upgrade users to using worktree config. It will relocate core.bare and core.worktree if necessary.

* 'git worktree add' will copy the sparse-checkout patterns from the current worktree to the new one. If worktree config is enabled, then the config settings from the current worktree are copied to the new worktree's config file.

Updates in v3
=============

* This is now rebased onto a merge of:

* en/sparse-checkout-set (for dependence on 'git sparse-checkout set

     --cone'),

* en/worktree-chatty-to-stderr (for an adjacent change in Documentation/git-worktree.txt), and
* ds/sparse-checkout-malformed-pattern-fix (since it needed a fixup! with the --worktree option in a test)

* The strategy is changed significantly: we simultaneously stop auto-upgrading to worktree config in 'git sparse-checkout set' while also providing a clear way for users to self-upgrade in 'git worktree init-worktree-config'. This upgrade moves core.bare and core.worktree, if they exist, and does not do anything if worktree config is already enabled.

* The sparse-checkout builtin will write to the worktree config, if it is enabled. The helper it uses now will fallback to the common config file if worktree config is not enabled.

* The 'git worktree add' command is updated to copy the sparse-checkout patterns and config from the current worktree into the new one.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/

Update in v2
============

* Eric correctly pointed out that I was writing core.bare incorrectly. It should move out of the core config and into the core repository's worktree config.
* Patch 3 is new, separating the "upgrade" logic out of config.c, but it is still called by the config helper to make it painless to write worktree config.

Thanks, -Stolee

Derrick Stolee (6):
setup: use a repository when upgrading format
config: make some helpers repo-aware
worktree: add 'init-worktree-config' subcommand
config: add repo_config_set_worktree_gently()
sparse-checkout: use repo_config_set_worktree_gently()
worktree: copy sparse-checkout patterns and config on add

 Documentation/git-worktree.txt           |  22 +++-
 builtin/sparse-checkout.c                |  25 ++---
 builtin/worktree.c                       | 123 +++++++++++++++++++++++
 config.c                                 |  50 ++++++++-
 config.h                                 |  15 +++
 list-objects-filter-options.c            |   2 +-
 repository.h                             |   2 +-
 setup.c                                  |   6 +-
 sparse-index.c                           |  10 +-
 t/t1091-sparse-checkout-builtin.sh       | 110 ++++++++++++++++++--
 t/t2407-worktree-init-worktree-config.sh |  68 +++++++++++++
 11 files changed, 385 insertions(+), 48 deletions(-)
 create mode 100755 t/t2407-worktree-init-worktree-config.sh

base-commit: 998dc12e841b4b17dd5a4700bb443fa215505e3d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v2:

 1:  889e69dc45d = 1:  749ba67d21e setup: use a repository when upgrading format
 2:  3e01356815a = 2:  61b96937016 config: make some helpers repo-aware
 3:  ed8e2a7b19d ! 3:  e2a0a458115 worktree: add upgrade_to_worktree_config()

@@ Metadata
Author: Derrick Stolee <[email protected]>

## Commit message ##
- worktree: add upgrade_to_worktree_config()
+ worktree: add 'init-worktree-config' subcommand

- Some features, such as the sparse-checkout builtin, require using the
+ Some features, such as the sparse-checkout builtin, currently use the
worktree config extension. It might seem simple to upgrade the
- repository format and add extensions.worktreeConfig, and that is what
- happens in the sparse-checkout builtin.
+ repository format and add extensions.worktreeConfig, which is what
+ happens in the sparse-checkout builtin. However, this is overly
+ simplistic and can cause issues in some cases. We will transition away
+ from making this upgrade automatically, but first we will make an easy
+ way for users to upgrade their repositories correctly.

Transitioning from one config file to multiple has some strange side-effects. In particular, if the base repository is bare and the worktree is not, Git knows to treat the worktree as non-bare as a special case when not using worktree config. Once worktree config is enabled, Git stops that special case since the core.bare setting could
- apply at the worktree config level. This opens the door for bare
- worktrees.
+ apply at the worktree config level.

- To help resolve this transition, create upgrade_to_worktree_config() to
- navigate the intricacies of this operation. In particular, we need to
- look for core.bare=true within the base config file and move that
- setting into the core repository's config.worktree file.
+ Similarly, the core.worktree config setting is a precursor to the 'git
+ worktree' feature, allowing config to point to a different worktree,
+ presumably temporarily. This is special-cased to be ignored in a
+ worktree, but that case is dropped when worktree config is enabled.
+
+ To help resolve this transition, create the 'git worktree
+ init-worktree-config' helper. This new subcommand does the following:
+
+ 1. Set core.repositoryFormatVersion to 1 in the common config file.
+ 2. Set extensions.worktreeConfig to true in the common config file.
+ 3. If core.bare is true in the common config file, then move that
+ setting to the main worktree's config file.
+ 4. Move the core.worktree config value to the main worktree's config
+ file.
+
+ If the repository is already configured to use worktree config, then
+ none of these steps happen. This preserves any state that the user might
+ have created on purpose.
+
+ Update the documentation to mention this subcommand as the proper way to
+ upgrade to worktree config files.

To gain access to the core repository's config and config.worktree file, we reference a repository struct's 'commondir' member. If the repository
@@ Commit message

- ## worktree.c ##
+ ## Documentation/git-worktree.txt ##
+@@ Documentation/git-worktree.txt: SYNOPSIS
+ --------
+ [verse]
+ 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>] ++'git worktree init-worktree-config'
+ 'git worktree list' [-v | --porcelain]
+ 'git worktree lock' [--reason <string>] <worktree>
+ 'git worktree move' <worktree> <new-path>
+@@ Documentation/git-worktree.txt: checked out in the new working tree, if it's not checked out anywhere
+ else, otherwise the command will refuse to create the working tree (unless
+ --force is used).
+
++init-worktree-config::
++
++Initialize config settings to enable worktree-specific config settings.
++This will set core.repositoryFormatversion=1 and enable
++extensions.worktreeConfig, which might cause some third-party tools from
++being able to operate on your repository. See CONFIGURATION FILE for more
++details.
++
+ list::
+
+ List details of each working tree. The main working tree is listed first,
+@@ Documentation/git-worktree.txt: already present in the config file, they will be applied to the main
+ working trees only.
+
+ In order to have configuration specific to working trees, you can turn
+-on the worktreeConfig extension, e.g.:
++on the worktreeConfig extension, using this command:
+
+ ------------
+-$ git config extensions.worktreeConfig true
++$ git worktree init-worktree-config
+ ------------
+
+ In this mode, specific configuration stays in the path pointed by `git
+@@ Documentation/git-worktree.txt: versions will refuse to access repositories with this extension. +
+ Note that in this file, the exception for core.bare and core.worktree
+ is gone. If they exist in $GIT_DIR/config, you must move
+-them to the config.worktree of the main working tree. You may also
+-take this opportunity to review and move other configuration that you
+-do not want to share to all working trees:
++them to the config.worktree of the main working tree. These keys are
++moved automatically when you use the git worktree init-worktree-config
++command.
++
++You may also take this opportunity to review and move other configuration
++that you do not want to share to all working trees:
+
+ - core.worktree and core.bare should never be shared
+
+
+ ## builtin/worktree.c ##
@@
- #include "worktree.h"
- #include "dir.h"
- #include "wt-status.h"
-+#include "config.h"

- void free_worktrees(struct worktree **worktrees)
- {
-@@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
- *wtpath = path;
- return 0;
+ static const char * const worktree_usage[] = {
+ N_("git worktree add [<options>] <path> [<commit-ish>]"),
++ N_("git worktree init-worktree-config"),
+ N_("git worktree list [<options>]"),
+ N_("git worktree lock [<options>] <path>"),
+ N_("git worktree move <worktree> <new-path>"),
+@@ builtin/worktree.c: static int repair(int ac, const char **av, const char *prefix)
+ return rc;
}
+
++static int move_config_setting(const char *key, const char *value,
++ const char *from_file, const char *to_file)
++{
++ if (git_config_set_in_file_gently(to_file, key, value))
++ return error(_("unable to set %s in '%s'"), key, to_file);
++ if (git_config_set_in_file_gently(from_file, key, NULL))
++ return error(_("unable to unset %s in '%s'"), key, from_file);
++ return 0;
++}
+
-+int upgrade_to_worktree_config(struct repository *r)
++static int init_worktree_config(int ac, const char **av, const char *prefix)
+{
-+ int res;
++ struct repository *r = the_repository;
++ struct option options[] = {
++ OPT_END()
++ };
++ int res = 0;
+ int bare = 0;
+ struct config_set cs = { 0 };
-+ char *base_config_file = xstrfmt("%s/config", r->commondir);
-+ char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
++ const char *core_worktree;
++ char *common_config_file = xstrfmt("%s/config", r->commondir);
++ char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
++
++ /* Report error on any arguments */
++ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
++ if (ac)
++ usage_with_options(worktree_usage, options);
+
+ git_configset_init(&cs);
-+ git_configset_add_file(&cs, base_config_file);
++ git_configset_add_file(&cs, common_config_file);
+
+ /*
-+ * If the base repository is bare, then we need to move core.bare=true
-+ * out of the base config file and into the base repository's
-+ * config.worktree file.
++ * If the format and extension are already enabled, then we can
++ * skip the upgrade process.
+ */
-+ if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
-+ if ((res = git_config_set_in_file_gently(base_worktree_file,
-+ "core.bare", "true"))) {
-+ error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
-+ goto cleanup;
-+ }
++ if (repository_format_worktree_config)
++ return 0;
+
-+ if ((res = git_config_set_in_file_gently(base_config_file,
-+ "core.bare", NULL))) {
-+ error(_("unable to unset core.bare=true in '%s'"), base_config_file);
-+ goto cleanup;
-+ }
-+ }
+ if (upgrade_repository_format(r, 1) < 0) {
+ res = error(_("unable to upgrade repository format to enable worktreeConfig"));
+ goto cleanup;
@@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
+ goto cleanup;
+ }
+
++ /*
++ * If core.bare is true in the common config file, then we need to
++ * move it to the base worktree's config file or it will break all
++ * worktrees. If it is false, then leave it in place because it
++ * _could_ be negating a global core.bare=true.
++ */
++ if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
++ if ((res = move_config_setting("core.bare", "true",
++ common_config_file,
++ main_worktree_file)))
++ goto cleanup;
++ }
++ /*
++ * If core.worktree is set, then the base worktree is located
++ * somewhere different than the parent of the common Git dir.
++ * Relocate that value to avoid breaking all worktrees with this
++ * upgrade to worktree config.
++ */
++ if (!git_configset_get_string_tmp(&cs, "core.worktree", &core_worktree)) {
++ if ((res = move_config_setting("core.worktree", core_worktree,
++ common_config_file,
++ main_worktree_file)))
++ goto cleanup;
++ }
++
+cleanup:
+ git_configset_clear(&cs);
-+ free(base_config_file);
-+ free(base_worktree_file);
-+ trace2_printf("returning %d", res);
++ free(common_config_file);
++ free(main_worktree_file);
+ return res;
+}
++
+ int cmd_worktree(int ac, const char **av, const char *prefix)
+ {
+ struct option options[] = {
+@@ builtin/worktree.c: int cmd_worktree(int ac, const char **av, const char *prefix)
+ prefix = "";
+ if (!strcmp(av[1], "add"))
+ return add(ac - 1, av + 1, prefix);
++ if (!strcmp(av[1], "init-worktree-config"))
++ return init_worktree_config(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "prune"))
+ return prune(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "list"))

- ## worktree.h ##
-@@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt,
- struct strbuf *sb,
- const char *refname);
-
-+/**
-+ * Upgrade the config of the current repository and its base (if different -+ * from this repository) to use worktree-config. This might adjust config -+ * in both repositories, including:
-+ *
-+ * 1. Upgrading the repository format version to 1.
-+ * 2. Adding extensions.worktreeConfig to the base config file. -+ * 3. Moving core.bare=true from the base config file to the base
-+ * repository's config.worktree file.
-+ */
-+int upgrade_to_worktree_config(struct repository *r);
-+
- #endif
+ ## t/t2407-worktree-init-worktree-config.sh (new) ##
+@@
++#!/bin/sh
++
++test_description='test git worktree init-worktree-config'
++
++. ./test-lib.sh
++
++test_expect_success setup '
++ git init base &&
++ test_commit -C base commit &&
++ git -C base worktree add --detach worktree
++'
++
++reset_config_when_finished () {
++ test_when_finished git -C base config --unset core.repositoryFormatVersion &&
++ test_when_finished git -C base config --unset extensions.worktreeConfig &&
++ rm -rf base/.git/config.worktree &&
++ rm -rf base/.git/worktrees/worktree/config.worktree
++}
++
++test_expect_success 'upgrades repo format and adds extension' '
++ reset_config_when_finished &&
++ git -C base worktree init-worktree-config >out 2>err &&
++ test_must_be_empty out &&
++ test_must_be_empty err &&
++ test_cmp_config -C base 1 core.repositoryFormatVersion &&
++ test_cmp_config -C base true extensions.worktreeConfig
++'
++
++test_expect_success 'relocates core.worktree' '
++ reset_config_when_finished &&
++ mkdir dir &&
++ git -C base config core.worktree ../../dir &&
++ git -C base worktree init-worktree-config >out 2>err &&
++ test_must_be_empty out &&
++ test_must_be_empty err &&
++ test_cmp_config -C base 1 core.repositoryFormatVersion &&
++ test_cmp_config -C base true extensions.worktreeConfig &&
++ test_cmp_config -C base ../../dir core.worktree &&
++ test_must_fail git -C worktree core.worktree
++'
++
++test_expect_success 'relocates core.bare' '
++ reset_config_when_finished &&
++ git -C base config core.bare true &&
++ git -C base worktree init-worktree-config >out 2>err &&
++ test_must_be_empty out &&
++ test_must_be_empty err &&
++ test_cmp_config -C base 1 core.repositoryFormatVersion &&
++ test_cmp_config -C base true extensions.worktreeConfig &&
++ test_cmp_config -C base true core.bare &&
++ test_must_fail git -C worktree core.bare
++'
++
++test_expect_success 'skips upgrade is already upgraded' '
++ reset_config_when_finished &&
++ git -C base worktree init-worktree-config &&
++ git -C base config core.bare true &&
++
++ # this should be a no-op, even though core.bare
++ # makes the worktree be broken.
++ git -C base worktree init-worktree-config >out 2>err &&
++ test_must_be_empty out &&
++ test_must_be_empty err &&
++ test_must_fail git -C base config --worktree core.bare &&
++ git -C base config core.bare
++'
++
++test_done
4: 22896e9bb04 ! 4: 45316cd01c9 config: add repo_config_set_worktree_gently()
@@ Metadata
## Commit message ##
config: add repo_config_set_worktree_gently()

- The previous change added upgrade_to_worktree_config() to assist
- creating a worktree-specific config for the first time. However, this
- requires every config writer to care about that upgrade before writing
- to the worktree-specific config. In addition, callers need to know how
- to generate the name of the config.worktree file and pass it to the
- config API.
+ Some config settings, such as those for sparse-checkout, are likely
+ intended to only apply to one worktree at a time. To make this write
+ easier, add a new config API method, repo_config_set_worktree_gently().

- To assist, create a new repo_config_set_worktree_gently() method in the
- config API that handles the upgrade_to_worktree_config() method in
- addition to assigning the value in the worktree-specific config. This
- will be consumed by an upcoming change.
+ This method will attempt to write to the worktree-specific config, but
+ will instead write to the common config file if worktree config is not
+ enabled. The next change will introduce a consumer of this method.

@@ config.c: int git_config_set_gently(const char *key, const char *value)
+int repo_config_set_worktree_gently(struct repository *r,
+ const char *key, const char *value)
+{
-+ return upgrade_to_worktree_config(r) ||
-+ git_config_set_multivar_in_file_gently(
-+ repo_git_path(r, "config.worktree"),
-+ key, value, NULL, 0);
++ /* Only use worktree-specific config if it is is already enabled. */
++ if (repository_format_worktree_config) {
++ char *file = repo_git_path(r, "config.worktree");
++ int ret = git_config_set_multivar_in_file_gently(
++ file, key, value, NULL, 0);
++ free(file);
++ return ret;
++ }
++ return repo_config_set_gently(r, key, value);
+}
+
void git_config_set(const char *key, const char *value)
{
repo_config_set(the_repository, key, value);
+@@ config.c: int repo_config_set_multivar_gently(struct repository *r, const char *key,
+ flags);
+ }
+
++int repo_config_set_gently(struct repository *r,
++ const char *key, const char *value)
++{
++ return repo_config_set_multivar_gently(r, key, value, NULL, 0);
++}
++
+ void git_config_set_multivar(const char *key, const char *value,
+ const char *value_pattern, unsigned flags)
+ {

## config.h ##
@@ config.h: void git_config_set_in_file(const char *, const char *, const char *);
@@ config.h: void git_config_set_in_file(const char *, const char *, const char *);
int git_config_set_gently(const char *, const char *);

+/**
-+ * Write a config value into the config.worktree file for the current
-+ * worktree. This will initialize extensions.worktreeConfig if necessary,
-+ * which might trigger some changes to the root repository's config file.
++ * Write a config value that should apply to the current worktree. If
++ * extensions.worktreeConfig is enabled, then the write will happen in the
++ * current worktree's config. Otherwise, write to the common config file.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
+
/**
* write config values to .git/config, takes a key/value pair as parameter.
*/
+@@ config.h: int git_config_set_multivar_gently(const char *, const char *, const char *, uns
+ void git_config_set_multivar(const char *, const char *, const char *, unsigned);
+ int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
+ void repo_config_set_multivar(struct repository *, const char *, const char *, const char *, unsigned); ++int repo_config_set_gently(struct repository *, const char *, const char *);
+ int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned); +
+ /**
5: 06457fafa78 ! 5: b200819c1bb sparse-checkout: use repo_config_set_worktree_gently()
@@ Commit message
sparse-checkout: use repo_config_set_worktree_gently()

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.
+ writing config values into the worktree.config file, if enabled.

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

- The fix is to have this assignment into config.worktree be handled by
- the repo_config_set_worktree_gently() helper.
+ 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.

@@ sparse-index.c: static int convert_to_sparse_rec(struct index_state *istate,
return res;

## t/t1091-sparse-checkout-builtin.sh ##
-@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'git sparse-checkout init' '
- check_files repo a
+@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'interaction with clone --no-checkout (unborn index)' '
'

-+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 /*
+ 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
+ ) &&
-+ git -C bare config --list --show-origin >actual &&
-+ grep "file:config.worktree core.bare=true" actual
++ 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 'git sparse-checkout list after init' '
- git -C repo sparse-checkout list >actual &&
- cat >expect <<-\EOF &&
++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' '
+@@ t/t1091-sparse-checkout-builtin.sh: 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 &&
@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'sparse-index enabled and disabled' '
test-tool -C repo read-cache --table >cache &&
! grep " tree " cache &&
@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'sparse-index enabled an
)
'

+@@ t/t1091-sparse-checkout-builtin.sh: 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 &&
-: ----------- > 6: fcece09546c worktree: copy sparse-checkout patterns and config on add

gitgitgadget

From: Derrick Stolee <[email protected]>

The upgrade_repository_format() helper previously was not aware of the possibility of multiple repositories. Add a 'struct repository *' parameter so it is possible to call it from a specific repository.

The implementation already referred to the_repository in one place, so that is an easy replacement. The use of git_config_set() is replaced with a call to repo_config_set().

---
 builtin/sparse-checkout.c     | 2 +-
 list-objects-filter-options.c | 2 +-
 repository.h                  | 2 +-
 setup.c                       | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

index 679c1070368..08f8df2648c 100644
@@ -361,7 +361,7 @@ static int set_config(enum sparse_checkout_mode mode)
 {
 	const char *config_path;
 
-	if (upgrade_repository_format(1) < 0)
+	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"));

index fd8d59f653a..6e21d12045e 100644
@@ -372,7 +372,7 @@ void partial_clone_register(
 			 */
 			return;
 	} else {
-		if (upgrade_repository_format(1) < 0)
+		if (upgrade_repository_format(the_repository, 1) < 0)
 			die(_("unable to upgrade repository format to support partial clone"));
 
 		/* Add promisor config for the remote */

index 98f95834706..d3fc1f7689d 100644
@@ -215,6 +215,6 @@ void prepare_repo_settings(struct repository *r);
  * Return 1 if upgrade repository format to target_version succeeded,
  * 0 if no upgrade is necessary, and -1 when upgrade is not possible.
  */
-int upgrade_repository_format(int target_version);
+int upgrade_repository_format(struct repository *, int target_version);
 
 #endif /* REPOSITORY_H */

index 347d7181ae9..90516664ce5 100644
@@ -595,14 +595,14 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 	return 0;
 }
 
-int upgrade_repository_format(int target_version)
+int upgrade_repository_format(struct repository *r, int target_version)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf err = STRBUF_INIT;
 	struct strbuf repo_version = STRBUF_INIT;
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 
-	strbuf_git_common_path(&sb, the_repository, "config");
+	strbuf_git_common_path(&sb, r, "config");
 	read_repository_format(&repo_fmt, sb.buf);
 	strbuf_release(&sb);
 

@@ -621,7 +621,7 @@ int upgrade_repository_format(int target_version)
 			     repo_fmt.unknown_extensions.items[0].string);
 
 	strbuf_addf(&repo_version, "%d", target_version);
-	git_config_set("core.repositoryformatversion", repo_version.buf);
+	repo_config_set(r, "core.repositoryformatversion", repo_version.buf);
 	strbuf_release(&repo_version);
 	return 1;
 }

gitgitgadget

From: Derrick Stolee <[email protected]>

As we prepare to add new config helpers to write into a config.worktree, let's make some existing methods be available for writing to a config file relative to a repository.

---
 config.c | 29 ++++++++++++++++++++++++++---
 config.h |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

index c5873f3a706..9c9eef16018 100644
@@ -2882,7 +2882,12 @@ int git_config_set_gently(const char *key, const char *value)
 
 void git_config_set(const char *key, const char *value)
 {
-	git_config_set_multivar(key, value, NULL, 0);
+	repo_config_set(the_repository, key, value);
+}
+
+void repo_config_set(struct repository *r, const char *key, const char *value)
+{
+	repo_config_set_multivar(r, key, value, NULL, 0);
 
 	trace2_cmd_set_config(key, value);
 }

@@ -3177,14 +3182,32 @@ void git_config_set_multivar_in_file(const char *config_filename,
 int git_config_set_multivar_gently(const char *key, const char *value,
 				   const char *value_pattern, unsigned flags)
 {
-	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
+	return repo_config_set_multivar_gently(the_repository, key, value,
+					       value_pattern, flags);
+}
+
+int repo_config_set_multivar_gently(struct repository *r, const char *key,
+				    const char *value,
+				    const char *value_pattern, unsigned flags)
+{
+	return git_config_set_multivar_in_file_gently(repo_git_path(r, "config"),
+						      key, value, value_pattern,
 						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
 			     const char *value_pattern, unsigned flags)
 {
-	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
+	repo_config_set_multivar(the_repository, key, value,
+				 value_pattern, flags);
+}
+
+void repo_config_set_multivar(struct repository *r, const char *key,
+			      const char *value, const char *value_pattern,
+			      unsigned flags)
+{
+	git_config_set_multivar_in_file(repo_git_path(r, "config"),
+					key, value, value_pattern,
 					flags);
 }
 

index f119de01309..5531fc018e3 100644
@@ -258,6 +258,11 @@ int git_config_set_gently(const char *, const char *);
  */
 void git_config_set(const char *, const char *);
 
+/**
+ * write config values to `.git/config`, takes a key/value pair as parameter.
+ */
+void repo_config_set(struct repository *, const char *, const char *);
+
 int git_config_parse_key(const char *, char **, size_t *);
 
 /*

@@ -281,6 +286,8 @@ int git_config_parse_key(const char *, char **, size_t *);
 
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
+int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
+void repo_config_set_multivar(struct repository *, const char *, const char *, const char *, unsigned);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**

gitgitgadget

From: Derrick Stolee <[email protected]>

Some features, such as the sparse-checkout builtin, currently use the worktree config extension. It might seem simple to upgrade the repository format and add extensions.worktreeConfig, which is what happens in the sparse-checkout builtin. However, this is overly simplistic and can cause issues in some cases. We will transition away from making this upgrade automatically, but first we will make an easy way for users to upgrade their repositories correctly.

Transitioning from one config file to multiple has some strange side-effects. In particular, if the base repository is bare and the worktree is not, Git knows to treat the worktree as non-bare as a special case when not using worktree config. Once worktree config is enabled, Git stops that special case since the core.bare setting could apply at the worktree config level.

Similarly, the core.worktree config setting is a precursor to the 'git worktree' feature, allowing config to point to a different worktree, presumably temporarily. This is special-cased to be ignored in a worktree, but that case is dropped when worktree config is enabled.

To help resolve this transition, create the 'git worktree init-worktree-config' helper. This new subcommand does the following:

1. Set core.repositoryFormatVersion to 1 in the common config file.
2. Set extensions.worktreeConfig to true in the common config file.
3. If core.bare is true in the common config file, then move that setting to the main worktree's config file.
4. Move the core.worktree config value to the main worktree's config file.

If the repository is already configured to use worktree config, then none of these steps happen. This preserves any state that the user might have created on purpose.

Update the documentation to mention this subcommand as the proper way to upgrade to worktree config files.

To gain access to the core repository's config and config.worktree file, we reference a repository struct's 'commondir' member. If the repository was a submodule instead of a worktree, then this still applies correctly.

---
 Documentation/git-worktree.txt           | 22 +++++--
 builtin/worktree.c                       | 82 ++++++++++++++++++++++++
 t/t2407-worktree-init-worktree-config.sh | 68 ++++++++++++++++++++
 3 files changed, 167 insertions(+), 5 deletions(-)
 create mode 100755 t/t2407-worktree-init-worktree-config.sh

index 9e862fbcf79..0f0642ac039 100644
@@ -10,6 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>]
+'git worktree init-worktree-config'
 'git worktree list' [-v | --porcelain]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>

@@ -92,6 +93,14 @@ checked out in the new working tree, if it's not checked out anywhere
 else, otherwise the command will refuse to create the working tree (unless
 `--force` is used).
 
+init-worktree-config::
+
+Initialize config settings to enable worktree-specific config settings.
+This will set `core.repositoryFormatversion=1` and enable
+`extensions.worktreeConfig`, which might cause some third-party tools from
+being able to operate on your repository. See CONFIGURATION FILE for more
+details.
+
 list::
 
 List details of each working tree.  The main working tree is listed first,

@@ -290,10 +299,10 @@ already present in the config file, they will be applied to the main
 working trees only.
 
 In order to have configuration specific to working trees, you can turn
-on the `worktreeConfig` extension, e.g.:
+on the `worktreeConfig` extension, using this command:
 
 ------------
-$ git config extensions.worktreeConfig true
+$ git worktree init-worktree-config
 ------------
 
 In this mode, specific configuration stays in the path pointed by `git

@@ -303,9 +312,12 @@ versions will refuse to access repositories with this extension.
 
 Note that in this file, the exception for `core.bare` and `core.worktree`
 is gone. If they exist in `$GIT_DIR/config`, you must move
-them to the `config.worktree` of the main working tree. You may also
-take this opportunity to review and move other configuration that you
-do not want to share to all working trees:
+them to the `config.worktree` of the main working tree. These keys are
+moved automatically when you use the `git worktree init-worktree-config`
+command.
+
+You may also take this opportunity to review and move other configuration
+that you do not want to share to all working trees:
 
  - `core.worktree` and `core.bare` should never be shared
 

gitgitgadget

Like my not-a-proper-review of [6/6], this also is not a proper review...

    struct config_set cs = { { 0 } };

In [1], I suggested that you should be using repository->gitdir rather than repository->commondir to access the .git/config file. Is the above paragraph saying that my suggestion was incorrect? Or is it incorrect only in the case of submodules? Or what is it saying?

Since you agree that "{r->commondir}/config.worktree" is the main worktree's config file, then "{r->commondir}/config" should be the repo-wide config file. If r->commondir and r->gitdir are different, then using r->gitdir would be wrong, as far as I understand it.

Indeed, tracing these two values when run in a worktree, I see:

gitdir: /home/stolee/_git/git/.git/worktrees/git-upstream
commondir: /home/stolee/_git/git/.git

So we definitely want commondir here.

Right, or move the 'if' to before the configset is initialized. The goto is simple enough.

Thanks,
-Stolee

Okay, it looks like I was misinterpreting how path.c:strbuf_worktree_gitdir() worked and how it was called, and was perhaps a bit confused by the minimal struct repository comments.

Moving the if -- in fact, all three ifs -- earlier is enticing, but you need to be careful not to leak common_config_file and main_worktree_file, as well. Something like this should work, I think:

   if (repository_format_worktree_config)
       return 0;
   if (upgrade_repository_format(r, 1) < 0)
       return error(...);
   if (git_config_set_gently(...))
       return error(...);

   common_config_file = xstrfmt(...);
   main_worktree_file = xstrfmt(...);
   git_configset_init(&cs);
   git_configset_add_file(&cs, common_config_file);

From: Derrick Stolee <[email protected]>

Some config settings, such as those for sparse-checkout, are likely intended to only apply to one worktree at a time. To make this write easier, add a new config API method, repo_config_set_worktree_gently().

This method will attempt to write to the worktree-specific config, but will instead write to the common config file if worktree config is not enabled. The next change will introduce a consumer of this method.

---
 config.c | 21 +++++++++++++++++++++
 config.h |  8 ++++++++
 2 files changed, 29 insertions(+)

index 9c9eef16018..f849aef253c 100644
@@ -21,6 +21,7 @@
 #include "dir.h"
 #include "color.h"
 #include "refs.h"
+#include "worktree.h"
 
 struct config_source {
 	struct config_source *prev;

@@ -2880,6 +2881,20 @@ int git_config_set_gently(const char *key, const char *value)
 	return git_config_set_multivar_gently(key, value, NULL, 0);
 }
 
+int repo_config_set_worktree_gently(struct repository *r,
+				    const char *key, const char *value)
+{
+	/* Only use worktree-specific config if it is is already enabled. */
+	if (repository_format_worktree_config) {
+		char *file = repo_git_path(r, "config.worktree");
+		int ret = git_config_set_multivar_in_file_gently(
+					file, key, value, NULL, 0);
+		free(file);
+		return ret;
+	}
+	return repo_config_set_gently(r, key, value);
+}
+
 void git_config_set(const char *key, const char *value)
 {
 	repo_config_set(the_repository, key, value);

@@ -3195,6 +3210,12 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,
 						      flags);
 }
 
+int repo_config_set_gently(struct repository *r,
+			   const char *key, const char *value)
+{
+	return repo_config_set_multivar_gently(r, key, value, NULL, 0);
+}
+
 void git_config_set_multivar(const char *key, const char *value,
 			     const char *value_pattern, unsigned flags)
 {

index 5531fc018e3..1386009fac8 100644
@@ -253,6 +253,13 @@ void git_config_set_in_file(const char *, const char *, const char *);
 
 int git_config_set_gently(const char *, const char *);
 
+/**
+ * Write a config value that should apply to the current worktree. If
+ * extensions.worktreeConfig is enabled, then the write will happen in the
+ * current worktree's config. Otherwise, write to the common config file.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
+
 /**
  * write config values to `.git/config`, takes a key/value pair as parameter.
  */

@@ -288,6 +295,7 @@ int git_config_set_multivar_gently(const char *, const char *, const char *, uns
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
 int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
 void repo_config_set_multivar(struct repository *, const char *, const char *, const char *, unsigned);
+int repo_config_set_gently(struct repository *, const char *, const char *);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

---
 builtin/sparse-checkout.c          | 25 +++++--------
 sparse-index.c                     | 10 ++----
 t/t1091-sparse-checkout-builtin.sh | 57 +++++++++++++++++++++++++-----
 3 files changed, 60 insertions(+), 32 deletions(-)

index 08f8df2648c..d0d6749593e 100644
@@ -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;
 }

index a1d505d50e9..e93609999e0 100644
@@ -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;