[PATCH 0/4] 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 <redacted>

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

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c     | 2 +-
 list-objects-filter-options.c | 2 +-
 repository.h                  | 2 +-
 setup.c                       | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index d0f5c4702be..34447f87cd8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -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"));

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index fd8d59f653a..6e21d12045e 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c

@@ -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 */

diff --git a/repository.h b/repository.h
index 98f95834706..d3fc1f7689d 100644
--- a/repository.h
+++ b/repository.h

@@ -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 */

diff --git a/setup.c b/setup.c
index 347d7181ae9..90516664ce5 100644
--- a/setup.c
+++ b/setup.c

@@ -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 <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 config.c | 29 ++++++++++++++++++++++++++---
 config.h |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index c5873f3a706..9c9eef16018 100644
--- a/config.c
+++ b/config.c

@@ -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);
 }
 

diff --git a/config.h b/config.h
index f119de01309..5531fc018e3 100644
--- a/config.h
+++ b/config.h

@@ -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 <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 config.c | 27 +++++++++++++++++++++++++++
 config.h |  6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/config.c b/config.c
index 9c9eef16018..67f3d5015ef 100644
--- a/config.c
+++ b/config.c

@@ -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);

diff --git a/config.h b/config.h
index 5531fc018e3..d8db420849e 100644
--- a/config.h
+++ b/config.h

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

From: Derrick Stolee <redacted>

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

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

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

Reported-by: Sean Allred <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c          | 25 ++++++++-----------------
 sparse-index.c                     | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh | 14 +++++++++++++-
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 34447f87cd8..ec2c9a146cc 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -356,26 +356,17 @@ enum sparse_checkout_mode {
 
 static int set_config(enum sparse_checkout_mode mode)
 {
-	const char *config_path;
-
-	if (upgrade_repository_format(the_repository, 1) < 0)
-		die(_("unable to upgrade repository format to enable worktreeConfig"));
-	if (git_config_set_gently("extensions.worktreeConfig", "true")) {
-		error(_("failed to set extensions.worktreeConfig setting"));
+	if (repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckout",
+					    mode ? "true" : "false") ||
+	    repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckoutCone",
+					    mode == MODE_CONE_PATTERNS ?
+						"true" : "false"))
 		return 1;
-	}
-
-	config_path = git_path("config.worktree");
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckout",
-				      mode ? "true" : NULL);
-
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckoutCone",
-				      mode == MODE_CONE_PATTERNS ? "true" : NULL);
 
 	if (mode == MODE_NO_PATTERNS)
-		set_sparse_index_config(the_repository, 0);
+		return set_sparse_index_config(the_repository, 0);
 
 	return 0;
 }

diff --git a/sparse-index.c b/sparse-index.c
index a1d505d50e9..e93609999e0 100644
--- a/sparse-index.c
+++ b/sparse-index.c

@@ -99,13 +99,9 @@ static int convert_to_sparse_rec(struct index_state *istate,
 
 int set_sparse_index_config(struct repository *repo, int enable)
 {
-	int res;
-	char *config_path = repo_git_path(repo, "config.worktree");
-	res = git_config_set_in_file_gently(config_path,
-					    "index.sparse",
-					    enable ? "true" : NULL);
-	free(config_path);
-
+	int res = repo_config_set_worktree_gently(repo,
+						  "index.sparse",
+						  enable ? "true" : "false");
 	prepare_repo_settings(repo);
 	repo->settings.sparse_index = enable;
 	return res;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..3ff125c5fd6 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

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

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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.

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/

On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:

From: Derrick Stolee <redacted>

...

+	/*
+	 * 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;
+	}

As mentioned by Eric, this portion isn't correct. It fixes _this_ worktree, but
any other existing worktrees would become broken.

The fix would be to detect if the core config file has core.bare=false and then
to move that setting into the base repo's config.worktree file.

I believe that if we do that change, then the rest of this patch series is valid.

What do others think?

Thanks,
-Stolee

On 12/20/2021 11:21 AM, Eric Sunshine wrote:

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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.

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.

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.
 

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.

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.
 

[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,
-Stolee

On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee [off-list ref] wrote:

On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:

quoted

+     /*
+      * 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))) {

As mentioned by Eric, this portion isn't correct. It fixes _this_ worktree, but
any other existing worktrees would become broken.

The fix would be to detect if the core config file has core.bare=false and then
to move that setting into the base repo's config.worktree file.

I believe that if we do that change, then the rest of this patch series is valid.

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.

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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.

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/6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/

I understand your desire to make this "setter" function as transparent
and simple for clients as possible, however, I think it does too much
by conflating two very distinct operations (one which changes the
nature of the repository itself, and one which simply sets a config
variable), and is far too magical. It doesn't help that the function
name gives no indication of just how magical it is, and it is easy to
imagine people calling this function thinking that it's just a simple
"config setter" like all the other similarly-named functions, without
realizing the impact it may have on the repository overall (i.e.
upgrading to version 1 and changing to per-worktree config).

I would feel much more comfortable for the new utility function to
have a single-purpose: namely, to upgrade the repository to a
per-worktree configuration mode (if not already upgraded) as outlined
in [4]. That's it. It shouldn't do anything other than that. This way,
callers which need per-worktree configuration must call the new
function explicitly to obtain the desired behavior, rather than
getting per-worktree configuration as a magical and implicit
side-effect of calling what looks like a plain old "config setter".
This should make it easier to reason about. Taking this approach also
means that you don't need to introduce a special-purpose "config
setter" just for worktrees; instead, you'd use the normal existing
"config setter" functions. For instance, if the new utility function
is named enable_per_worktree_config(), then `git sparse-checkout init`
might do something like this:

    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", ...);

(This, of course, assumes that repo_git_path() latches the changes
made by enable_per_worktree_config() so that it "does the right
thing", but it seems that existing code in `git sparse-checkout init`
is already expecting it to do so, so perhaps it does work that way.)

On Mon, Dec 20, 2021 at 7:01 PM Eric Sunshine [off-list ref] wrote:

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

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

On Mon, Dec 20, 2021 at 12:34 PM Derrick Stolee [off-list ref] wrote:

On 12/20/2021 11:21 AM, Eric Sunshine wrote:

quoted

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.

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 sent several pages worth of response to patch [3/4] because
(apparently) I don't know how to be laconic.

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

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

quoted

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.

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.

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

On 12/20/2021 7:01 PM, Eric Sunshine wrote:

On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee [off-list ref] wrote:

quoted

On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:

quoted

+     /*
+      * 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))) {

As mentioned by Eric, this portion isn't correct. It fixes _this_ worktree, but
any other existing worktrees would become broken.

The fix would be to detect if the core config file has core.bare=false and then
to move that setting into the base repo's config.worktree file.

I believe that if we do that change, then the rest of this patch series is valid.

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

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

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.

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

On 12/21/2021 12:53 AM, Eric Sunshine wrote:

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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.

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/6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/

quoted

Signed-off-by: Derrick Stolee <redacted>
---

diff --git a/config.c b/config.c

@@ -2880,6 +2880,33 @@ 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);
+}

diff --git a/config.h b/config.h

@@ -253,6 +253,12 @@ 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.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);

I understand your desire to make this "setter" function as transparent
and simple for clients as possible, however, I think it does too much
by conflating two very distinct operations (one which changes the
nature of the repository itself, and one which simply sets a config
variable), and is far too magical. It doesn't help that the function
name gives no indication of just how magical it is, and it is easy to
imagine people calling this function thinking that it's just a simple
"config setter" like all the other similarly-named functions, without
realizing the impact it may have on the repository overall (i.e.
upgrading to version 1 and changing to per-worktree config).

I would feel much more comfortable for the new utility function to
have a single-purpose: namely, to upgrade the repository to a
per-worktree configuration mode (if not already upgraded) as outlined
in [4]. That's it. It shouldn't do anything other than that. This way,
callers which need per-worktree configuration must call the new
function explicitly to obtain the desired behavior, rather than
getting per-worktree configuration as a magical and implicit
side-effect of calling what looks like a plain old "config setter".
This should make it easier to reason about. Taking this approach also
means that you don't need to introduce a special-purpose "config
setter" just for worktrees; instead, you'd use the normal existing
"config setter" functions. For instance, if the new utility function
is named enable_per_worktree_config(), then `git sparse-checkout init`
might do something like this:

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.
 

    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", ...);

(This, of course, assumes that repo_git_path() latches the changes
made by enable_per_worktree_config() so that it "does the right
thing", but it seems that existing code in `git sparse-checkout init`
is already expecting it to do so, so perhaps it does work that way.)

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

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.
      
          Signed-off-by: Derrick Stolee [off-list ref]
      
       ## 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 <redacted>

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

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c     | 2 +-
 list-objects-filter-options.c | 2 +-
 repository.h                  | 2 +-
 setup.c                       | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index d0f5c4702be..34447f87cd8 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -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"));

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index fd8d59f653a..6e21d12045e 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c

@@ -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 */

diff --git a/repository.h b/repository.h
index 98f95834706..d3fc1f7689d 100644
--- a/repository.h
+++ b/repository.h

@@ -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 */

diff --git a/setup.c b/setup.c
index 347d7181ae9..90516664ce5 100644
--- a/setup.c
+++ b/setup.c

@@ -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 <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 config.c | 29 ++++++++++++++++++++++++++---
 config.h |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index c5873f3a706..9c9eef16018 100644
--- a/config.c
+++ b/config.c

@@ -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);
 }
 

diff --git a/config.h b/config.h
index f119de01309..5531fc018e3 100644
--- a/config.h
+++ b/config.h

@@ -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 <redacted>

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.

Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 worktree.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 12 ++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/worktree.c b/worktree.c
index 2c155b10150..e8ddbe2bfcc 100644
--- a/worktree.c
+++ b/worktree.c

@@ -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;
+}

diff --git a/worktree.h b/worktree.h
index 8b7c408132d..170b6b1e1f5 100644
--- a/worktree.h
+++ b/worktree.h

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

From: Derrick Stolee <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 config.c | 10 ++++++++++
 config.h |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/config.c b/config.c
index 9c9eef16018..81f3a689c11 100644
--- a/config.c
+++ b/config.c

@@ -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);

diff --git a/config.h b/config.h
index 5531fc018e3..b05c51b3528 100644
--- a/config.h
+++ b/config.h

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

From: Derrick Stolee <redacted>

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

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

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

Reported-by: Sean Allred <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c          | 25 ++++++++-----------------
 sparse-index.c                     | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh | 16 +++++++++++++++-
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 34447f87cd8..ec2c9a146cc 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -356,26 +356,17 @@ enum sparse_checkout_mode {
 
 static int set_config(enum sparse_checkout_mode mode)
 {
-	const char *config_path;
-
-	if (upgrade_repository_format(the_repository, 1) < 0)
-		die(_("unable to upgrade repository format to enable worktreeConfig"));
-	if (git_config_set_gently("extensions.worktreeConfig", "true")) {
-		error(_("failed to set extensions.worktreeConfig setting"));
+	if (repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckout",
+					    mode ? "true" : "false") ||
+	    repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckoutCone",
+					    mode == MODE_CONE_PATTERNS ?
+						"true" : "false"))
 		return 1;
-	}
-
-	config_path = git_path("config.worktree");
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckout",
-				      mode ? "true" : NULL);
-
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckoutCone",
-				      mode == MODE_CONE_PATTERNS ? "true" : NULL);
 
 	if (mode == MODE_NO_PATTERNS)
-		set_sparse_index_config(the_repository, 0);
+		return set_sparse_index_config(the_repository, 0);
 
 	return 0;
 }

diff --git a/sparse-index.c b/sparse-index.c
index a1d505d50e9..e93609999e0 100644
--- a/sparse-index.c
+++ b/sparse-index.c

@@ -99,13 +99,9 @@ static int convert_to_sparse_rec(struct index_state *istate,
 
 int set_sparse_index_config(struct repository *repo, int enable)
 {
-	int res;
-	char *config_path = repo_git_path(repo, "config.worktree");
-	res = git_config_set_in_file_gently(config_path,
-					    "index.sparse",
-					    enable ? "true" : NULL);
-	free(config_path);
-
+	int res = repo_config_set_worktree_gently(repo,
+						  "index.sparse",
+						  enable ? "true" : "false");
 	prepare_repo_settings(repo);
 	repo->settings.sparse_index = enable;
 	return res;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..b563ccfeb36 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -71,6 +71,20 @@ test_expect_success 'git sparse-checkout init' '
 	check_files repo a
 '
 
+test_expect_success 'init in a worktree of a bare repo' '
+	test_when_finished rm -rf bare worktree &&
+	git clone --bare repo bare &&
+	git -C bare worktree add ../worktree &&
+	(
+		cd worktree &&
+		git sparse-checkout init &&
+		test_must_fail git config core.bare &&
+		git sparse-checkout set /*
+	) &&
+	git -C bare config --list --show-origin >actual &&
+	grep "file:config.worktree	core.bare=true" actual
+'
+
 test_expect_success 'git sparse-checkout list after init' '
 	git -C repo sparse-checkout list >actual &&
 	cat >expect <<-\EOF &&

@@ -219,7 +233,7 @@ test_expect_success 'sparse-index enabled and disabled' '
 		test-tool -C repo read-cache --table >cache &&
 		! grep " tree " cache &&
 		git -C repo config --list >config &&
-		! grep index.sparse config
+		test_cmp_config -C repo false index.sparse
 	)
 '
 

-- 
gitgitgadget

On Tue, Dec 21, 2021 at 8:46 AM Derrick Stolee [off-list ref] wrote:

On 12/21/2021 12:53 AM, Eric Sunshine wrote:

quoted

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

+/**
+ * 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 *);

I understand your desire to make this "setter" function as transparent
and simple for clients as possible, however, I think it does too much
by conflating two very distinct operations (one which changes the
nature of the repository itself, and one which simply sets a config
variable), and is far too magical. It doesn't help that the function
name gives no indication of just how magical it is, and it is easy to
imagine people calling this function thinking that it's just a simple
"config setter" like all the other similarly-named functions, without
realizing the impact it may have on the repository overall (i.e.
upgrading to version 1 and changing to per-worktree config).

I would feel much more comfortable for the new utility function to
have a single-purpose: namely, to upgrade the repository to a
per-worktree configuration mode (if not already upgraded) as outlined
in [4]. That's it. It shouldn't do anything other than that. This way,
callers which need per-worktree configuration must call the new
function explicitly to obtain the desired behavior, rather than
getting per-worktree configuration as a magical and implicit
side-effect of calling what looks like a plain old "config setter".
This should make it easier to reason about. Taking this approach also
means that you don't need to introduce a special-purpose "config
setter" just for worktrees; instead, you'd use the normal existing
"config setter" functions. For instance, if the new utility function
is named enable_per_worktree_config(), then `git sparse-checkout init`
might do something like this:

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.

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

quoted

    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", ...);

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.

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.

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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.

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

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.

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

This function is doing a lot of unnecessary work if per-worktree
configuration is already enabled. The very first thing it should be
doing is checking whether or not it actually needs to do that work,
and short-circuit if it doesn't. I would think that simply checking
whether `extensions.worktreeConfig` is true and returning early if it
is would be sufficient.

+       char *base_config_file = xstrfmt("%s/config", r->commondir);

If we look at path.c:strbuf_worktree_gitdir(), we see that this should
be using `r->gitdir`, not `r->commondir`.

+       char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

Per path.c:strbuf_worktree_gitdir(), this use of `r->commondir` is
correct. Good.

Can we use more meaningful variable names? It's not at all clear what
"base" means in this context (I don't think it has any analog in Git
terminology). Perhaps name these `shared_config` and `repo_config`,
respectively.

+       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.
+        */

Here, too, it's not clear what "base" means. I think you want to say
that it needs to "move `core.bare` from the shared config to the
repo-specific config".

+       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;
+               }
+       }

This seems unnecessarily complicated and overly specific, thus
potentially confusing. The requirements laid out in git-worktree.txt
say only to move the configuration key from .git/config to
.git/config.worktree; it doesn't add any qualifiers about the value
being "true". And, indeed, we should not care about the value; it's
immaterial to this operation. Instead, we should just treat it
opaquely and relocate the key and value from .git/config (if it
exists) to .git/config.worktree without interpretation.

The error messages are too specific, as well, by mentioning "true".
They could instead be:

    unable to set `core.bare` in '%s'

and

    unable to remove `core.bare` from '%s'

However, there is a much more _severe_ problem with this
implementation: it is incomplete. As documented in git-worktree.txt
(and mentioned several times during this discussion), this utility
function _must_ relocate both `core.bare` _and_ `core.worktree` (if
they exist) from .git/config to .git/config.worktree. This
implementation neglects to relocate `core.worktree`, which can leave
things in quite a broken state (just as neglecting to relocate
`core.bare` can).

+       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;
+       }

The order in which this function performs its operations can leave the
repository in a broken state if any of the steps fails. For instance,
if setting `extensions.worktreeConfig=true` fails _after_ you've
relocated `core.bare` (and `core.worktree`) to .git/config.worktree,
then those configuration values will no longer be "active" since the
config system won't consult .git/config.worktree without
`extensions.worktreeConfig` enabled.

To be resilient against this sort of problem, you should perform the
operations in this order:

(1) upgrade repository format to 1
(2) enable `extensions.worktreeConfig`
(3) relocate `core.bare` and `core.worktree`

+cleanup:
+       git_configset_clear(&cs);
+       free(base_config_file);
+       free(base_worktree_file);
+       trace2_printf("returning %d", res);

Is this leftover debugging or intentional?

Here, too, it's not clear what "base" means. Moreover, this seems to
be talking about multiple repositories, but we're only dealing with a
single repository and zero or more worktrees, so it's not clear what
this is trying to say.

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

As mentioned above, it's unnecessary and perhaps confusing to focus
only on "true" here; we should be treating the value opaquely.

Also, if you're talking about the specific config settings which this
relocates, then `core.worktree` should be mentioned too, not just
`core.bare`.

+ */
+int upgrade_to_worktree_config(struct repository *r);

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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.

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/

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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

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

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

+               git sparse-checkout set /*

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

    git sparse-checkout set "/*"

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

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

+'

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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.

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.

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.

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.

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.

On Wed, Dec 22, 2021 at 8:00 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

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.

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.

On Wed, Dec 22, 2021 at 5:54 PM Elijah Newren [off-list ref] wrote:

On Wed, Dec 22, 2021 at 8:00 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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

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

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

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

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?

On Mon, Dec 27, 2021 at 2:15 AM Eric Sunshine [off-list ref] wrote:

On Wed, Dec 22, 2021 at 5:54 PM Elijah Newren [off-list ref] wrote:

quoted

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.

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.

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?

On Sun, Dec 26, 2021 at 11:15 PM Eric Sunshine [off-list ref] wrote:

On Wed, Dec 22, 2021 at 5:54 PM Elijah Newren [off-list ref] wrote:

quoted

On Wed, Dec 22, 2021 at 8:00 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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

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

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.

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.

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

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.

quoted

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.

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

Yes, indeed.

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

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.

[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

quoted

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.

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?

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

On Sun, Dec 26, 2021 at 11:34 PM Eric Sunshine [off-list ref] wrote:

On Mon, Dec 27, 2021 at 2:15 AM Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Dec 22, 2021 at 5:54 PM Elijah Newren [off-list ref] wrote:

quoted

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.

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.

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.

Indeed.  :-)

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`?

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

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?

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

On Mon, Dec 27, 2021 at 2:35 PM Elijah Newren [off-list ref] wrote:

On Sun, Dec 26, 2021 at 11:15 PM Eric Sunshine [off-list ref] wrote:

quoted

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.

These are instructions which neither Stolee nor I was aware of prior
to your pointing it out.  [...]
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. [...]

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.

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.

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.

quoted

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?

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.

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.

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

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.

On Mon, Dec 27, 2021 at 3:16 PM Elijah Newren [off-list ref] wrote:

On Sun, Dec 26, 2021 at 11:34 PM Eric Sunshine [off-list ref] wrote:

quoted

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.

Indeed.  :-)

quoted

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`?

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:

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.

On 12/21/2021 7:45 PM, Eric Sunshine wrote:

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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.

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

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.
 

quoted

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.

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:

Good suggestions to add the necessary context here. Thanks.

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

quoted

Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---

diff --git a/worktree.c b/worktree.c

@@ -830,3 +831,49 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
+int upgrade_to_worktree_config(struct repository *r)
+{
+       int res;
+       int bare = 0;
+       struct config_set cs = { 0 };

This function is doing a lot of unnecessary work if per-worktree
configuration is already enabled. The very first thing it should be
doing is checking whether or not it actually needs to do that work,
and short-circuit if it doesn't. I would think that simply checking
whether `extensions.worktreeConfig` is true and returning early if it
is would be sufficient.

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

quoted

+       char *base_config_file = xstrfmt("%s/config", r->commondir);

If we look at path.c:strbuf_worktree_gitdir(), we see that this should
be using `r->gitdir`, not `r->commondir`.

quoted

+       char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

Per path.c:strbuf_worktree_gitdir(), this use of `r->commondir` is
correct. Good.

Can we use more meaningful variable names? It's not at all clear what
"base" means in this context (I don't think it has any analog in Git
terminology). Perhaps name these `shared_config` and `repo_config`,
respectively.

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

quoted

+       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.
+        */

Here, too, it's not clear what "base" means. I think you want to say
that it needs to "move `core.bare` from the shared config to the
repo-specific config".

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

quoted

+       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;
+               }
+       }

This seems unnecessarily complicated and overly specific, thus
potentially confusing. The requirements laid out in git-worktree.txt
say only to move the configuration key from .git/config to
.git/config.worktree; it doesn't add any qualifiers about the value
being "true". And, indeed, we should not care about the value; it's
immaterial to this operation. Instead, we should just treat it
opaquely and relocate the key and value from .git/config (if it
exists) to .git/config.worktree without interpretation.

The error messages are too specific, as well, by mentioning "true".
They could instead be:

    unable to set `core.bare` in '%s'

and

    unable to remove `core.bare` from '%s'

However, there is a much more _severe_ problem with this
implementation: it is incomplete. As documented in git-worktree.txt
(and mentioned several times during this discussion), this utility
function _must_ relocate both `core.bare` _and_ `core.worktree` (if
they exist) from .git/config to .git/config.worktree. This
implementation neglects to relocate `core.worktree`, which can leave
things in quite a broken state (just as neglecting to relocate
`core.bare` can).

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.

quoted

+       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;
+       }

The order in which this function performs its operations can leave the
repository in a broken state if any of the steps fails. For instance,
if setting `extensions.worktreeConfig=true` fails _after_ you've
relocated `core.bare` (and `core.worktree`) to .git/config.worktree,
then those configuration values will no longer be "active" since the
config system won't consult .git/config.worktree without
`extensions.worktreeConfig` enabled.

To be resilient against this sort of problem, you should perform the
operations in this order:

(1) upgrade repository format to 1
(2) enable `extensions.worktreeConfig`
(3) relocate `core.bare` and `core.worktree`

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.

quoted

+cleanup:
+       git_configset_clear(&cs);
+       free(base_config_file);
+       free(base_worktree_file);
+       trace2_printf("returning %d", res);

Is this leftover debugging or intentional?

Leftover debugging. Thanks for catching.

Thanks,
-Stolee

On Tue, Dec 28, 2021 at 10:03 AM Derrick Stolee [off-list ref] wrote:

On 12/21/2021 7:45 PM, Eric Sunshine wrote:

quoted

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

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'm not following. I also still don't know what "base repo" is or
where two HEADs would arise.

quoted

quoted

+       char *base_config_file = xstrfmt("%s/config", r->commondir);
+       char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

Per path.c:strbuf_worktree_gitdir(), this use of `r->commondir` is
correct. Good.

Can we use more meaningful variable names? It's not at all clear what
"base" means in this context (I don't think it has any analog in Git
terminology). Perhaps name these `shared_config` and `repo_config`,
respectively.

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

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.

quoted

quoted

+       /*
+        * 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.
+        */

Here, too, it's not clear what "base" means. I think you want to say
that it needs to "move `core.bare` from the shared config to the
repo-specific config".

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

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>

quoted

However, there is a much more _severe_ problem with this
implementation: it is incomplete. As documented in git-worktree.txt
(and mentioned several times during this discussion), this utility
function _must_ relocate both `core.bare` _and_ `core.worktree` (if
they exist) from .git/config to .git/config.worktree. This
implementation neglects to relocate `core.worktree`, which can leave
things in quite a broken state (just as neglecting to relocate
`core.bare` can).

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.

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.

On 12/28/2021 11:58 AM, Eric Sunshine wrote:

On Tue, Dec 28, 2021 at 10:03 AM Derrick Stolee [off-list ref] wrote:

quoted

On 12/21/2021 7:45 PM, Eric Sunshine wrote:

quoted

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

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'm not following. I also still don't know what "base repo" is or
where two HEADs would arise.

quoted

quoted

quoted

+       char *base_config_file = xstrfmt("%s/config", r->commondir);
+       char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

Per path.c:strbuf_worktree_gitdir(), this use of `r->commondir` is
correct. Good.

Can we use more meaningful variable names? It's not at all clear what
"base" means in this context (I don't think it has any analog in Git
terminology). Perhaps name these `shared_config` and `repo_config`,
respectively.

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

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.

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

Thanks,
-Stolee

On Mon, Dec 27, 2021 at 11:33 PM Eric Sunshine [off-list ref] wrote:

On Mon, Dec 27, 2021 at 2:35 PM Elijah Newren [off-list ref] wrote:

quoted

On Sun, Dec 26, 2021 at 11:15 PM Eric Sunshine [off-list ref] wrote:

quoted

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.

These are instructions which neither Stolee nor I was aware of prior
to your pointing it out.  [...]
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. [...]

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.

quoted

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.

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.

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.

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.

quoted

quoted

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?

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.

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.

quoted

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

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.

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.

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.

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.

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 [off-list ref]
      
       ## 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
          Helped-by: Eric Sunshine [off-list ref]
          Signed-off-by: Derrick Stolee [off-list ref]
      
     - ## 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.
      
          Signed-off-by: Derrick Stolee [off-list ref]
      
     @@ 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.
      
          Reported-by: Sean Allred [off-list ref]
          Helped-by: Eric Sunshine [off-list ref]
     @@ 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 <redacted>

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

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/sparse-checkout.c     | 2 +-
 list-objects-filter-options.c | 2 +-
 repository.h                  | 2 +-
 setup.c                       | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 679c1070368..08f8df2648c 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -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"));

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index fd8d59f653a..6e21d12045e 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c

@@ -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 */

diff --git a/repository.h b/repository.h
index 98f95834706..d3fc1f7689d 100644
--- a/repository.h
+++ b/repository.h

@@ -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 */

diff --git a/setup.c b/setup.c
index 347d7181ae9..90516664ce5 100644
--- a/setup.c
+++ b/setup.c

@@ -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 <redacted>

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.

Signed-off-by: Derrick Stolee <redacted>
---
 config.c | 29 ++++++++++++++++++++++++++---
 config.h |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index c5873f3a706..9c9eef16018 100644
--- a/config.c
+++ b/config.c

@@ -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);
 }
 

diff --git a/config.h b/config.h
index f119de01309..5531fc018e3 100644
--- a/config.h
+++ b/config.h

@@ -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 <redacted>

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.

Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 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

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..0f0642ac039 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt

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