patch, 4 partsSparse checkout: fix bug with worktree of bare repo

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

> On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
> > +     /*
> > +      * 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.

> 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

> On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee <stolee@gmail.com> wrote:
>> On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
>>> +     /*
>>> +      * 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

> 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 <dstolee@microsoft.com>
> ---
> 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 *);

> On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> 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/
> 
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> 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:

>     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 12/21/2021 12:53 AM, Eric Sunshine wrote:
> > On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +/**
> >> + * 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", ...);
>
> 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.

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

> On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> 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.

> On 12/20/2021 11:21 AM, Eric Sunshine wrote:
> > 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 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.

> 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 <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> 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 };

> +       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
> @@ -182,4 +182,16 @@ void strbuf_worktree_ref(const struct worktree *wt,
> +/**
> + * 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:

> On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> 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`.
> 
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> 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.

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

> On 12/21/2021 7:45 PM, Eric Sunshine wrote:
> > 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.

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

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

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

> On Tue, Dec 28, 2021 at 10:03 AM Derrick Stolee <stolee@gmail.com> wrote:
>> On 12/21/2021 7:45 PM, Eric Sunshine wrote:
>>> 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.
> 
>>>> +       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.

> +{
> +	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 (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

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +int upgrade_to_worktree_config(struct repository *r)
> 
> Not a suggestion to rename the function, but does it mean "we have
> been using a single configuration for all worktrees attached to the
> repository, but we are switching to use per-worktree configuration
> file"?  I am wondering if the phrase "worktree_config" is clear
> enough for our future developers.
> 
>> +{
>> +	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;
>> +		}
> 
> Hmph, I would have expected to see
> 
> 		if (git_config_set_in_file_gently(...)) {
> 			res = error(_("..."));
>                         goto cleanup;
> 		}
> 
> instead (obviously with "res" initialized to "0" to assume all will
> be well at the beginning).

> More importantly, are we prepared to see the repository 'r' that
> already uses per-worktree config layout and refrain from causing any
> damage to it, or are we all perfect developers that any caller that
> calls this on repository 'r' that does not need to be upgraded is a
> BUG()?

> 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 <dstolee@microsoft.com>
> ---
> 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.
>   */
> --

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

> +       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 &&

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

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

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

> On Wed, Dec 22, 2021 at 8:00 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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".

> 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 <newren@gmail.com> wrote:
> > 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.

>
> On Mon, Dec 27, 2021 at 2:15 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, Dec 22, 2021 at 5:54 PM Elijah Newren <newren@gmail.com> wrote:
> > > 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.

> On Sun, Dec 26, 2021 at 11:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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:

> On Sun, Dec 26, 2021 at 11:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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).

>
> On Mon, Dec 27, 2021 at 3:16 PM Elijah Newren <newren@gmail.com> wrote:
> > On Sun, Dec 26, 2021 at 11:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > 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.  :-)
>
> I mentioned previously[1] that I needed to find a block of time to
> really think through the topic before I'd be able to respond to this
> email. So, today I spent some time trying to reason through the
> various cases under discussion, and I came back and re-read this email
> with the intention of trying to summarize my understanding of the
> situation and my understanding of the points you were making. However,
> you did such a good job of summarizing the various cases at the very
> end of [2] that it probably makes more sense for me to respond to that
> email instead.
>
> [1]: https://lore.kernel.org/git/CAPig+cTFSDw-9Aq+=+r4sHSzTmG7s2T93Z0uqWTxHbKwGFaiYQ@mail.gmail.com/
> [2]: https://lore.kernel.org/git/CABPp-BHuO3B366uJuODMQo-y449p8cAMVn0g2MTcO5di3Xa7Zg@mail.gmail.com/
>
> > > 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 had seen 11664196ac when researching one of my earlier responses,
> though it took more than one read to (hopefully) fully understand what
> it is saying (i.e. due to an oversight, it's too late to enforce the
> `core.repositoryFormatVersion=1` requirement when extensions are used,
> as originally intended).
>
> > * This is a case where operating on the primary worktree was not
> > previously problematic for older git versions or third party tools.
> > * Interestingly, git <= 2.6.2 can continue to operate on the primary
> > worktree (because it didn't know to error out on unknown extensions)
> > * git >= 2.19.0 could continue to operate on the primary worktree
> > (because it understands the extension)
> > * git versions between that range would suddenly break, erroring out
> > on the unknown extension (though those versions would start working
> > again if we migrated core.bare and core.worktree but just didn't set
> > extensions.worktreeConfig).
>
> The significance of versions 2.6.2 and 2.19.0 is unclear to me. What
> context or criteria are you using to identify those versions as
> meaningful here?

>
> On Wed, Dec 22, 2021 at 5:54 PM Elijah Newren <newren@gmail.com> wrote:
> > On Wed, Dec 22, 2021 at 8:00 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > 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 Sun, Dec 26, 2021 at 11:15 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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 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.

>   * 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 Mon, Dec 27, 2021 at 2:35 PM Elijah Newren <newren@gmail.com> wrote:
> > On Sun, Dec 26, 2021 at 11:15 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > 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.
>
> > > 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 11:33 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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.

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

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

> > > 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.
>
> I do wonder, though, if there are traps waiting for us with this
> all-inclusive approach. I don't know what sort of worktree-specific
> configuration people use, so I do worry a bit that this could be
> casting a too-wide net, and that it might in fact be better to only
> copy the sparse-checkout settings (as ugly as it is to special-case
> that -- but we need to special-case `core.bare` and `core.worktree`
> anyhow[1]).
>
> [1]: https://lore.kernel.org/git/CAPig+cSUOknNC9GMyPvAqdBU0r1MVgvSpvgpSpXUmBm67HO7PQ@mail.gmail.com/

> > 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.
>
> It feels unfortunate and a bit dirty to spread around this
> special-case knowledge about sparse-checkout to various parts of the
> system,

> On Wed, Dec 29, 2021 at 10:41 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Tue, Dec 28, 2021 at 1:16 PM Elijah Newren <newren@gmail.com> wrote:
> > > On Mon, Dec 27, 2021 at 11:33 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > > 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.
> >
> > I do wonder, though, if there are traps waiting for us with this
> > all-inclusive approach. I don't know what sort of worktree-specific
> > configuration people use, so I do worry a bit that this could be
> > casting a too-wide net, and that it might in fact be better to only
> > copy the sparse-checkout settings (as ugly as it is to special-case
> > that -- but we need to special-case `core.bare` and `core.worktree`
> > anyhow[1]).
>
> I could probably be persuaded either way (do users want to copy
> something and tweak it, or start with a clean slate?), and it might
> even make sense to have a flag for users to specify.

> My hunch, at least with the developers I work with, is that they're
> more likely to think in terms of "I want another worktree like this
> one, except that I'm going to change..."
>
> Also, another reason to prefer copying all of core.worktree (minus the
> always worktree-specific value of core.worktree, and core.bare), is
> because it's easy to explain in the documentation, and I think we'd be
> much less likely to obsolete user's knowledge over time.  (I think
> additional sparse-checkout things, or new other features that also
> want to be copied over, are much more likely than the addition of keys
> that are always worktree-specific like core.worktree).

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -1031,6 +1032,85 @@ static int repair(int ac, const char **av, const char *prefix)
> +static int init_worktree_config(int ac, const char **av, const char *prefix)
> +{
> +       struct config_set cs = { 0 };

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

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -1031,6 +1032,85 @@ static int repair(int ac, const char **av, const char *prefix)
> +static int init_worktree_config(int ac, const char **av, const char *prefix)
> +{
> +       struct repository *r = the_repository;
> +       char *common_config_file = xstrfmt("%s/config", r->commondir);
> +       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

> +       git_configset_init(&cs);
> +       git_configset_add_file(&cs, common_config_file);
> +
> +       /*
> +        * If the format and extension are already enabled, then we can
> +        * skip the upgrade process.
> +        */
> +       if (repository_format_worktree_config)
> +               return 0;

> +       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(common_config_file);
> +       free(main_worktree_file);
> +       return res;
> +}

> On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [...]
>> 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.
> 
> In [1], I suggested that you should be using `repository->gitdir`
> rather than `repository->commondir` to access the `.git/config` file.
> Is the above paragraph saying that my suggestion was incorrect? Or is
> it incorrect only in the case of submodules? Or what is it saying?
> 
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -1031,6 +1032,85 @@ static int repair(int ac, const char **av, const char *prefix)
>> +static int init_worktree_config(int ac, const char **av, const char *prefix)
>> +{
>> +       struct repository *r = the_repository;
>> +       char *common_config_file = xstrfmt("%s/config", r->commondir);
>> +       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> 
> Specifically, in [1], I said that `common_config_file` should be using
> `r->gitdir` (and that the use of `r->commondir` was correct for
> `main_worktree_file`).

>> +       /*
>> +        * If the format and extension are already enabled, then we can
>> +        * skip the upgrade process.
>> +        */
>> +       if (repository_format_worktree_config)
>> +               return 0;
> 
> Rather than `return 0`, should this be `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;
>> +       }
> 
> ... as is done for these two cases?
> 
>> +cleanup:
>> +       git_configset_clear(&cs);
>> +       free(common_config_file);
>> +       free(main_worktree_file);
>> +       return res;

> On 12/30/2021 3:41 AM, Eric Sunshine wrote:
> > On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +static int init_worktree_config(int ac, const char **av, const char *prefix)
> >> +{
> >> +       struct repository *r = the_repository;
> >> +       char *common_config_file = xstrfmt("%s/config", r->commondir);
> >> +       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> >
> > Specifically, in [1], I said that `common_config_file` should be using
> > `r->gitdir` (and that the use of `r->commondir` was correct for
> > `main_worktree_file`).
>
> Since you agree that "{r->commondir}/config.worktree" is the main
> worktree's config file, then "{r->commondir}/config" should be the
> repo-wide config file. If r->commondir and r->gitdir are different,
> then using r->gitdir would be wrong, as far as I understand it.
>
> Indeed, tracing these two values when run in a worktree, I see:
>
>   gitdir: /home/stolee/_git/git/.git/worktrees/git-upstream
>   commondir: /home/stolee/_git/git/.git
>
> So we definitely want commondir here.

> >> +       if (repository_format_worktree_config)
> >> +               return 0;
> >
> > Rather than `return 0`, should this be `goto cleanup`...
>
> Right, or move the 'if' to before the configset is initialized. The
> goto is simple enough.

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

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

> When adding a new worktree, it is reasonable to expect that we want to
> use the current set of sparse-checkout settings for that new worktree.
> This is particularly important for repositories where the worktree would
> become too large to be useful. This is even more important when using
> partial clone as well, since we want to avoid downloading the missing
> blobs for files that should not be written to the new worktree.
>
> The only way to create such a worktree without this intermediate step of
> expanding the full worktree is to copy the sparse-checkout patterns and
> config settings during 'git worktree add'. Each worktree has its own
> sparse-checkout patterns, and the default behavior when the
> sparse-checkout file is missing is to include all paths at HEAD. Thus,
> we need to have patterns from somewhere, they might as well be the
> current worktree's patterns. These are then modified independently in
> the future.
>
> In addition to the sparse-checkout file, copy the worktree config file
> if worktree config is enabled and the file exists. This will copy over
> any important settings to ensure the new worktree behaves the same as
> the current one.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -336,6 +336,47 @@ static int add_worktree(const char *path, const char *refname,
> +       /*
> +        * If we are using worktree config, then copy all currenct config
> +        * values from the current worktree into the new one, that way the
> +        * new worktree behaves the same as this one.
> +        */

> +       if (repository_format_worktree_config) {
> +               char *from_file = git_pathdup("config.worktree");
> +               char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
> +                                       realpath.buf, name);
> +
> +               if (file_exists(from_file)) {
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666))
> +                               error(_("failed to copy worktree config from '%s' to '%s'"),
> +                                     from_file, to_file);
> +               }

> On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> When adding a new worktree, it is reasonable to expect that we want to
>> use the current set of sparse-checkout settings for that new worktree.
>> This is particularly important for repositories where the worktree would
>> become too large to be useful. This is even more important when using
>> partial clone as well, since we want to avoid downloading the missing
>> blobs for files that should not be written to the new worktree.
>>
>> The only way to create such a worktree without this intermediate step of
>> expanding the full worktree is to copy the sparse-checkout patterns and
>> config settings during 'git worktree add'. Each worktree has its own
>> sparse-checkout patterns, and the default behavior when the
>> sparse-checkout file is missing is to include all paths at HEAD. Thus,
>> we need to have patterns from somewhere, they might as well be the
>> current worktree's patterns. These are then modified independently in
>> the future.
>>
>> In addition to the sparse-checkout file, copy the worktree config file
>> if worktree config is enabled and the file exists. This will copy over
>> any important settings to ensure the new worktree behaves the same as
>> the current one.
> 
> This is not a proper review. I just happened to very quickly scan my
> eyes over this patch without even having looked at any of the others,
> nor have I read the v3 cover letter closely yet. Nevertheless, while
> skimming this patch, an issue jumped out at me...
> 
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -336,6 +336,47 @@ static int add_worktree(const char *path, const char *refname,
>> +       /*
>> +        * If we are using worktree config, then copy all currenct config
>> +        * values from the current worktree into the new one, that way the
>> +        * new worktree behaves the same as this one.
>> +        */
> 
> s/currenct/current/
> 
>> +       if (repository_format_worktree_config) {
>> +               char *from_file = git_pathdup("config.worktree");
>> +               char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
>> +                                       realpath.buf, name);
>> +
>> +               if (file_exists(from_file)) {
>> +                       if (safe_create_leading_directories(to_file) ||
>> +                           copy_file(to_file, from_file, 0666))
>> +                               error(_("failed to copy worktree config from '%s' to '%s'"),
>> +                                     from_file, to_file);
>> +               }
> 
> I presume that you lifted this idea from [1] in which I offhandedly
> mentioned that one possible way to implement Elijah's desire to copy
> sparse-checkout configuration when a new worktree is created would be
> to simply copy the existing worktree-specific configuration to the new
> worktree. Unfortunately, a direct implementation of that idea suffers
> the same problem which started this entire thread. Namely:
> 
>     % git clone --bare <url>/bare.git
>     % cd bare.git/
>     % git worktree init-worktree-config
>     % git worktree add -d ../foo
>     Preparing worktree (detached HEAD a0df8ce)
>     HEAD is now at a0df8ce gobbledygook
>     % cd ../foo/
>     % git sparse-checkout init
>     fatal: this operation must be run in a work tree
>     % git config --get --show-origin --show-scope core.bare
>     worktree file:.../bare.git/worktrees/foo/config.worktree true
> 
> The problem is that for a bare repository, after `git worktree
> init-worktree-config`, "bare.git/config.worktree" contains the
> repo-specific `core.bare=true` setting, so copying
> "bare.git/config.worktree" to
> "bare.git/worktrees/<id>/config.worktree" verbatim has undesired
> consequences.

> The obvious way to work around this problem is to (again) special-case
> `core.bare` and `core.worktree` to remove them when copying the
> worktree-specific configuration. Whether or not that is the best
> solution or even desirable is a different question. (I haven't thought
> it through enough to have an opinion.)

> > The obvious way to work around this problem is to (again) special-case
> > `core.bare` and `core.worktree` to remove them when copying the
> > worktree-specific configuration. Whether or not that is the best
> > solution or even desirable is a different question. (I haven't thought
> > it through enough to have an opinion.)
>
> It makes sense to special case these two settings since we want to
> allow creating a working worktree from a bare repo, even if it has
> worktree config stating that it is bare.

> On Wed, Dec 29, 2021 at 9:31 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 12/29/2021 1:37 AM, Eric Sunshine wrote:
>>> On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
> 
>>> The obvious way to work around this problem is to (again) special-case
>>> `core.bare` and `core.worktree` to remove them when copying the
>>> worktree-specific configuration. Whether or not that is the best
>>> solution or even desirable is a different question. (I haven't thought
>>> it through enough to have an opinion.)
>>
>> It makes sense to special case these two settings since we want to
>> allow creating a working worktree from a bare repo, even if it has
>> worktree config stating that it is bare.
> 
> Agreed.
> 
>> As far as the implementation goes, we could do the copy and then
>> unset those two values in the new file. That's an easy enough change.
> 
>> I'll wait for more feedback on the overall ideas (and names of things
>> like the init-worktree-config subcommand).
> 
> What value does the init-worktree-config subcommand provide; why
> shouldn't we just get rid of it?
> 
> I know Eric was strongly suggesting it, but he was thinking in terms
> of always doing that full switchover step, or never doing it.  Both
> extremes had the potential to cause user-visible bugs, and thus he
> suggested providing a command to allow users to pick their poison.  I
> provided a suggestion avoiding both extremes that doesn't have that
> pick-your-poison approach, so I don't see why forcing users into this
> extra step makes any sense.
> 
> But perhaps I missed something.  Is there a usecase for users to
> explicitly use this?

>
> On 12/29/2021 2:51 PM, Elijah Newren wrote:
> > On Wed, Dec 29, 2021 at 9:31 AM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> On 12/29/2021 1:37 AM, Eric Sunshine wrote:
> >>> On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
> >>> <gitgitgadget@gmail.com> wrote:
> >
> >>> The obvious way to work around this problem is to (again) special-case
> >>> `core.bare` and `core.worktree` to remove them when copying the
> >>> worktree-specific configuration. Whether or not that is the best
> >>> solution or even desirable is a different question. (I haven't thought
> >>> it through enough to have an opinion.)
> >>
> >> It makes sense to special case these two settings since we want to
> >> allow creating a working worktree from a bare repo, even if it has
> >> worktree config stating that it is bare.
> >
> > Agreed.
> >
> >> As far as the implementation goes, we could do the copy and then
> >> unset those two values in the new file. That's an easy enough change.
> >
> >> I'll wait for more feedback on the overall ideas (and names of things
> >> like the init-worktree-config subcommand).
> >
> > What value does the init-worktree-config subcommand provide; why
> > shouldn't we just get rid of it?
> >
> > I know Eric was strongly suggesting it, but he was thinking in terms
> > of always doing that full switchover step, or never doing it.  Both
> > extremes had the potential to cause user-visible bugs, and thus he
> > suggested providing a command to allow users to pick their poison.  I
> > provided a suggestion avoiding both extremes that doesn't have that
> > pick-your-poison approach, so I don't see why forcing users into this
> > extra step makes any sense.
> >
> > But perhaps I missed something.  Is there a usecase for users to
> > explicitly use this?
>
> I think the motivation is that worktree config is something that is
> harder to set up than to just run a 'git config' command, and we
> should guide users into a best practice for using it. The
> documentation becomes "run this command to enable it".

> On Wed, Dec 29, 2021 at 1:39 PM Derrick Stolee <stolee@gmail.com> wrote:
> > I think the motivation is that worktree config is something that is
> > harder to set up than to just run a 'git config' command, and we
> > should guide users into a best practice for using it. The
> > documentation becomes "run this command to enable it".
>
> Okay, but that's an answer to a different question -- namely, "if
> users want/need to explicitly set it up, why should we have a
> command?"  Your answer here is a very good answer to that question,
> but you've assumed the "if".  My question was on the "if": (Why) Do
> users need or want to explicitly set it up?
>
> Secondarily, if users want to set it up explicitly, is the work here
> really sufficient to help guide them?  In particular, I discovered and
> started using extensions.worktreeConfig without ever looking at the
> relevant portions of git-worktree.txt (the references in
> git-config.txt never mentioned them).  I also pushed this usage to
> others, including even to you with `git-sparse-checkout`, and no
> reviewer on this list ever caught it or informed me of the `proper`
> additional guidelines found in git-worktree.txt until this thread
> started.  So, relying on folks to read git-worktree.txt for this
> config item feels a bit weak to me.  Granted, your new command will be
> much more likely to be read since it appears near the top of
> git-worktree.txt, but I just don't think that's enough.  The
> references to extensions.worktreeConfig in git-config.txt should
> reference any special command or extended steps if we expect users to
> manually configure it (whether via explicit new subcommand or via also
> playing with other config settings).

> Anyway, if we think users want to set it up explicitly, and we address
> the discoverability problem above, then I'd vote for
> "independent-config" or "private-config" (or _maybe_
> "migrate-config").  Because:
>   * no sense repeating the word `worktree` in `git worktree
> init-worktree-config`.  It's redundant.
>   * The words "independent" or "private" suggest what it does and why
> users might want to use the new subcommand.
>   * It's not an "init":

>     * As per my recommendation elsewhere, this step just isn't needed
> for the vast majority of users (i.e. those with non-bare clones who
> leave core.worktree alone).
>     * ...and it's also not needed for other (core.bare=true or
> core.worktree set) users since `git worktree add` will automatically
> run this config migration for them

> On Wed, Dec 29, 2021 at 9:31 AM Derrick Stolee <stolee@gmail.com> wrote:
> > I'll wait for more feedback on the overall ideas (and names of things
> > like the init-worktree-config subcommand).
>
> What value does the init-worktree-config subcommand provide; why
> shouldn't we just get rid of it?
>
> I know Eric was strongly suggesting it, but he was thinking in terms
> of always doing that full switchover step, or never doing it.  Both
> extremes had the potential to cause user-visible bugs, and thus he
> suggested providing a command to allow users to pick their poison.  I
> provided a suggestion avoiding both extremes that doesn't have that
> pick-your-poison approach, so I don't see why forcing users into this
> extra step makes any sense.

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

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

>      @@ Commit message
>           Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>           Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>
>      - ## 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

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

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

> On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This series is based on a merge of en/sparse-checkout-set,
>> en/worktree-chatty-to-stderr, and ds/sparse-checkout-malformed-pattern-fix.
> 
> I think you mean es/worktree-chatty-to-stderr (not 'en/')

>> 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.
> 
> This sounds dangerous to me.

> ...
>>      +    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.
> 
> Yikes.
> 
>>      +     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
> 
> No, it's not even rare, let alone very rare.  I'd actually call it
> common.  Since 'sparse-checkout disable' does not delete the
> sparse-checkout file, and we've encouraged folks to use the
> sparse-checkout command (or a wrapper thereof) instead of direct
> editing of the sparse-checkout file (and indeed, the sparse-checkout
> command will overwrite the sparse-checkout file which further
> discourages users from feeling they own it), having the file left
> around after disabling is the common case.  So, the only question is,
> how often do users disable and re-enable sparse-checkout, and
> potentially only do so in some of their worktrees?  At my $DAYJOB,
> that's actually quite common.  I got multiple reports quite soon after
> introducing our `sparsify` tool about users doing something like this;
> this is what led me to learn of the extensions.worktreeConfig, and why
> I pointed it out to you on your first submission of
> git-sparse-checkout[1]
> (https://lore.kernel.org/git/CABPp-BFcH5hQqujjmc88L3qGx3QAYZ_chH6PXQXyp13ipfV6hQ@mail.gmail.com/)

> On 12/29/2021 4:39 AM, Elijah Newren wrote:
> > No, it's not even rare, let alone very rare.  I'd actually call it
> > common.  Since 'sparse-checkout disable' does not delete the
> > sparse-checkout file, and we've encouraged folks to use the
> > sparse-checkout command (or a wrapper thereof) instead of direct
> > editing of the sparse-checkout file (and indeed, the sparse-checkout
> > command will overwrite the sparse-checkout file which further
> > discourages users from feeling they own it), having the file left
> > around after disabling is the common case.  So, the only question is,
> > how often do users disable and re-enable sparse-checkout, and
> > potentially only do so in some of their worktrees?  At my $DAYJOB,
> > that's actually quite common.  I got multiple reports quite soon after
> > introducing our `sparsify` tool about users doing something like this;
> > this is what led me to learn of the extensions.worktreeConfig, and why
> > I pointed it out to you on your first submission of
> > git-sparse-checkout[1]
> > (https://lore.kernel.org/git/CABPp-BFcH5hQqujjmc88L3qGx3QAYZ_chH6PXQXyp13ipfV6hQ@mail.gmail.com/)
>
> Thank you for these comments and the detailed descriptions of things
> from your $DAYJOB. That's helpful context and I'm happy to switch back
> to enabling the extension in the sparse-checkout builtin. I might need
> to rearrange the code so there is an API in worktree.c instead of just
> the subcommand in builtin/worktree.c, but that's pretty minor. I'll
> keep Eric's earlier suggestion to have the upgrade be a separate call
> from the repo_config_set_worktree_gently().

> On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >      ++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.
>
> So, if users attempt to use `git worktree add` or `git sparse-checkout
> {init,set}` without first running this, they can break other
> worktrees.  And if they do run this new command, they potentially
> break third-party tools or older git versions.

> >      +    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
>
> No, it's not even rare, let alone very rare.  I'd actually call it
> common.  Since 'sparse-checkout disable' does not delete the
> sparse-checkout file, and we've encouraged folks to use the
> sparse-checkout command (or a wrapper thereof) instead of direct
> editing of the sparse-checkout file (and indeed, the sparse-checkout
> command will overwrite the sparse-checkout file which further
> discourages users from feeling they own it), having the file left
> around after disabling is the common case.  So, the only question is,
> how often do users disable and re-enable sparse-checkout, and
> potentially only do so in some of their worktrees?  At my $DAYJOB,
> that's actually quite common.  I got multiple reports quite soon after
> introducing our `sparsify` tool about users doing something like this;
> this is what led me to learn of the extensions.worktreeConfig, and why
> I pointed it out to you on your first submission of
> git-sparse-checkout[1]
> (https://lore.kernel.org/git/CABPp-BFcH5hQqujjmc88L3qGx3QAYZ_chH6PXQXyp13ipfV6hQ@mail.gmail.com/)

> So, here's the experience I expect from these patches at $DAYJOB:
>   (1) Several users per week hit the case of one worktree being
> sparsified when it wasn't supposed to be.
>   (2) These users have no idea how to figure out what they need to do
> to fix it.  The init-worktree-config is no more discoverable than the
> documentation on the official steps for enabling
> extensions.worktreeConfig (See
> https://lore.kernel.org/git/CABPp-BGKyDJV9DP+igmCC_Ad0jgvb4aOAYpXWCbx9hW8ShhDQg@mail.gmail.com/
> up through the paragraph, "Further, it's not even clear people would
> look at git-worktree.txt.)
>   (3) Even if they do discover it, and run it, it's an extra step they
> never needed to take before.  Why are we adding a "unbreak these other
> commands we want to run" step?
>   (4) Also, even if they do discover it, and run it, suddenly we are
> setting core.repositoryFormatVersion=1.  That scares me.  I have years
> of experience at $DAYJOB saying that the tooling we have works fine
> with extensions.worktreeConfig=true.  I have none with setting
> core.repositoryFormatVersion=1, but now we're effectively requiring it
> by your documentation.  I have no idea how
> jgit/egit/other-random-stuff interacts with that.  I'd be willing to
> do some tests with targetted users to try to learn more, but suddenly
> turning it on for everyone in cases that we know worked fine without
> it previously feels unsafe to me.  Maybe I'm over-worrying here, but
> see also commit 11664196ac ("Revert "check_repository_format_gently():
> refuse
> extensions for old repositories"", 2020-07-15) -- it just feels a bit
> late to recommend for users, especially when they'll see it as "oh, if
> you don't want this other bug we recently introduced you need to run
> this....".

> So, I'd like to reiterate my earlier suggestion which would avoid
> these regressions while also fixing the reported bug:
>   * If core.bare=true or core.worktree is set, then at `git worktree
> add` time, automatically run the logic you have here for
> init-worktree-config.  Having either of those config settings with
> multiple worktrees is currently broken in all git versions and likely
> in most all external tools.  As such, being aggressive in the new
> config settings to allow new versions of git to work seems totally
> safe to me -- we can't be any more broken than we already were.
>   * If core.bare=false and core.worktree is not set, then:
>     * `git sparse-checkout {init,set}` should set
> extensions.worktreeConfig if not already set, and always set the
> core.sparse* and index.sparse settings in worktree-specific files.
>     * `git worktree add`, if extensions.worktreeConfig is already set,
> will copy both the info/sparse-checkout file and the config.worktree
> settings (module core.bare and core.worktree, if present) to the new
> worktree

>>   * If core.bare=true or core.worktree is set, then at `git worktree
>> add` time, automatically run the logic you have here for
>> init-worktree-config.  Having either of those config settings with
>> multiple worktrees is currently broken in all git versions and likely
>> in most all external tools.  As such, being aggressive in the new
>> config settings to allow new versions of git to work seems totally
>> safe to me -- we can't be any more broken than we already were.

> Thanks for the clearly written enumeration of how you expect this to
> work. This summary pretty well (or entirely) captures the conclusions
> I arrived at, as well, after devoting a chunk of time today thinking
> through the cases. If I'm understanding everything correctly, the
> approach outlined here solves the bare-worktree problem in the least
> invasive and least dangerous way (for older Git versions and foreign
> tools). And we don't even need the `git worktree init-worktree-config`
> subcommand (though we need the underlying functionality).

>
> On 12/30/2021 2:40 AM, Eric Sunshine wrote:
> > On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:>
>
> Taking time to focus only on this outline here:
>
> >> So, I'd like to reiterate my earlier suggestion which would avoid
> >> these regressions while also fixing the reported bug:
>
> >>   * If core.bare=true or core.worktree is set, then at `git worktree
> >> add` time, automatically run the logic you have here for
> >> init-worktree-config.  Having either of those config settings with
> >> multiple worktrees is currently broken in all git versions and likely
> >> in most all external tools.  As such, being aggressive in the new
> >> config settings to allow new versions of git to work seems totally
> >> safe to me -- we can't be any more broken than we already were.
>
> I'm not sure I agree with the "currently broken in all git versions"
> because when extensions.worktreeConfig is not enabled, the core.bare
> and core.worktree settings are ignored by the worktrees. This upgrade
> during 'add' is the only thing I am not so sure about.

> >>   * If core.bare=false and core.worktree is not set, then:
>
> >>     * `git sparse-checkout {init,set}` should set
> >> extensions.worktreeConfig if not already set, and always set the
> >> core.sparse* and index.sparse settings in worktree-specific files.
>
> This should happen no matter the case of core.bare and core.worktree
> existing, right?

> >>     * `git worktree add`, if extensions.worktreeConfig is already set,
> >> will copy both the info/sparse-checkout file and the config.worktree
> >> settings (module core.bare and core.worktree, if present) to the new
> >> worktree
>
> and here, 'git worktree add' should always copy the info/sparse-checkout
> file (if core.sparseCheckout is enabled) and copy the config.worktree
> settings if extensions.worktreeConfig is enabled (and filter out
> core.bare and core.worktree in the process).

> > Thanks for the clearly written enumeration of how you expect this to
> > work. This summary pretty well (or entirely) captures the conclusions
> > I arrived at, as well, after devoting a chunk of time today thinking
> > through the cases. If I'm understanding everything correctly, the
> > approach outlined here solves the bare-worktree problem in the least
> > invasive and least dangerous way (for older Git versions and foreign
> > tools). And we don't even need the `git worktree init-worktree-config`
> > subcommand (though we need the underlying functionality).
>
> I'm happy to drop the subcommand in favor of some documentation in
> git-config.txt (Documentation/config/extensions.txt to be exact).

> On Thu, Dec 30, 2021 at 9:41 AM Derrick Stolee <stolee@gmail.com> wrote:
> > On 12/30/2021 2:40 AM, Eric Sunshine wrote:
> > > On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:>
> > >>   * If core.bare=true or core.worktree is set, then at `git worktree
> > >> add` time, automatically run the logic you have here for
> > >> init-worktree-config.  Having either of those config settings with
> > >> multiple worktrees is currently broken in all git versions and likely
> > >> in most all external tools.  As such, being aggressive in the new
> > >> config settings to allow new versions of git to work seems totally
> > >> safe to me -- we can't be any more broken than we already were.
> >
> > I'm not sure I agree with the "currently broken in all git versions"
> > because when extensions.worktreeConfig is not enabled, the core.bare
> > and core.worktree settings are ignored by the worktrees. This upgrade
> > during 'add' is the only thing I am not so sure about.
>
> Oh, you're right; I mis-spoke.  If someone has core.bare=true and has
> multiple worktrees, AND never attempts to use sparse-checkouts OR
> otherwise set extensions.worktreeConfig, then git still works due to
> git's special-case logic that will override core.bare in this
> configuration.  It's just setting them up for a ticking time bomb,
> waiting for them to either use an external tool that doesn't share
> that special case override-core.bare logic, or for the user to decide
> to set extensions.worktreeConfig directly or use sparse-checkouts.

> > >>   * If core.bare=false and core.worktree is not set, then:
> >
> > >>     * `git sparse-checkout {init,set}` should set
> > >> extensions.worktreeConfig if not already set, and always set the
> > >> core.sparse* and index.sparse settings in worktree-specific files.
> >
> > This should happen no matter the case of core.bare and core.worktree
> > existing, right?
>
> Hmm.  I think that's safe for people who cloned and used `git worktree
> add` with newer git versions, since `git worktree add` will have moved
> core.bare and core.worktree to the config.worktree file when those
> have non-default values.
>
> But, we might want to help out the folks who have existing repos with
> which they have used older git versions.  So, we could have `git
> sparse-checkout {init,set}` check for non-default values of
> core.bare/core.worktree in the shared config file, and, if found, exit
> with an error which point users at some relevant documentation (which
> may just suggest 'git worktree add temporary && git worktree remove
> temporary' as a workaround for those caught in such a state.)

>
> On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:>
> > On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >      ++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.
> >
> > So, if users attempt to use `git worktree add` or `git sparse-checkout
> > {init,set}` without first running this, they can break other
> > worktrees.  And if they do run this new command, they potentially
> > break third-party tools or older git versions.
>
> When you say "can break other worktrees", you don't necessarily mean
> that in general but rather in regard to sparse-checkout -- in
> particular, the sparse-checkout config settings and the
> `info/sparse-checkout file` -- correct? (Genuine question; I want to
> make sure that I'm actually understanding the issues under
> discussion.)

> > So, I'd like to reiterate my earlier suggestion which would avoid
> > these regressions while also fixing the reported bug:
> >   * If core.bare=true or core.worktree is set, then at `git worktree
> > add` time, automatically run the logic you have here for
> > init-worktree-config.  Having either of those config settings with
> > multiple worktrees is currently broken in all git versions and likely
> > in most all external tools.  As such, being aggressive in the new
> > config settings to allow new versions of git to work seems totally
> > safe to me -- we can't be any more broken than we already were.
> >   * If core.bare=false and core.worktree is not set, then:
> >     * `git sparse-checkout {init,set}` should set
> > extensions.worktreeConfig if not already set, and always set the
> > core.sparse* and index.sparse settings in worktree-specific files.
> >     * `git worktree add`, if extensions.worktreeConfig is already set,
> > will copy both the info/sparse-checkout file and the config.worktree
> > settings (module core.bare and core.worktree, if present) to the new
> > worktree
>
> Thanks for the clearly written enumeration of how you expect this to
> work. This summary pretty well (or entirely) captures the conclusions
> I arrived at, as well, after devoting a chunk of time today thinking
> through the cases. If I'm understanding everything correctly, the
> approach outlined here solves the bare-worktree problem in the least
> invasive and least dangerous way (for older Git versions and foreign
> tools). And we don't even need the `git worktree init-worktree-config`
> subcommand (though we need the underlying functionality).

>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Upgrading a repository to use extensions.worktreeConfig is non-trivial.
> There are several steps involved, including moving some config settings
> from the common config file to the main worktree's config.worktree file.
> The previous change updated the documentation with all of these details.
>
> Commands such as 'git sparse-checkout set' upgrade the repository to use
> extensions.worktreeConfig without following these steps, causing some
> user pain in some special cases.
>
> Create a helper method, init_worktree_config(), that will be used in a
> later change to fix this behavior within 'git sparse-checkout set'. The
> method is carefully documented in worktree.h.

> Note that we do _not_ upgrade the repository format version to 1 during
> this process. The worktree config extension must be considered by Git
> and third-party tools even if core.repositoryFormatVersion is 0 for
> historical reasons documented in 11664196ac ("Revert
> "check_repository_format_gently(): refuse extensions for old
> repositories"", 2020-07-15). This is a special case for this extension,
> and newer extensions (such as extensions.objectFormat) still need to
> upgrade the repository format version.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  worktree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  worktree.h | 19 +++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 6f598dcfcdf..dc4ead4c8fb 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)
>  {
> @@ -826,3 +827,72 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
>         *wtpath = path;
>         return 0;
>  }
> +
> +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 init_worktree_config(struct repository *r)
> +{
> +       int res = 0;
> +       int bare = 0;
> +       struct config_set cs = { { 0 } };
> +       const char *core_worktree;
> +       char *common_config_file = xstrfmt("%s/config", r->commondir);
> +       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> +
> +       /*
> +        * If the extension is already enabled, then we can skip the
> +        * upgrade process.
> +        */
> +       if (repository_format_worktree_config)
> +               return 0;
> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
> +               return error(_("failed to set extensions.worktreeConfig setting"));
> +
> +       git_configset_init(&cs);
> +       git_configset_add_file(&cs, common_config_file);
> +
> +       /*
> +        * 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;
> +       }
> +
> +       /*
> +        * Ensure that we use worktree config for the remaining lifetime
> +        * of the current process.
> +        */
> +       repository_format_worktree_config = 1;
> +
> +cleanup:
> +       git_configset_clear(&cs);
> +       free(common_config_file);
> +       free(main_worktree_file);
> +       return res;
> +}
> diff --git a/worktree.h b/worktree.h
> index 9e06fcbdf3d..5ea5fcc3647 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -183,4 +183,23 @@ void strbuf_worktree_ref(const struct worktree *wt,
>                          struct strbuf *sb,
>                          const char *refname);
>
> +/**
> + * Enable worktree config for the first time. This will make the following
> + * adjustments:
> + *
> + * 1. Add extensions.worktreeConfig=true in the common config file.
> + *
> + * 2. If the common config file has a core.worktree value or core.bare is
> + *    set to true, then those values are moved to the main worktree's
> + *    config.worktree file.

> + *
> + * If extensions.worktreeConfig is already true, then this method
> + * terminates early without any of the above steps. The existing config
> + * arrangement is assumed to be intentional.
> + *
> + * Returns 0 on success. Reports an error message and returns non-zero
> + * if any of these steps fail.
> + */
> +int init_worktree_config(struct repository *r);
> +
>  #endif
> --

> As documented in 11664196ac ("Revert "check_repository_format_gently():
> refuse extensions for old repositories"", 2020-07-15), this extension
> must be considered regardless of the repository format version for
> historical reasons.
> 

> On 26/01/22 01.42, Derrick Stolee via GitGitGadget wrote:
>> As documented in 11664196ac ("Revert "check_repository_format_gently():
>> refuse extensions for old repositories"", 2020-07-15), this extension
>> must be considered regardless of the repository format version for
>> historical reasons.
>>
> ...
>> +For historical reasons, `extensions.worktreeConfig` is respected
>> +regardless of the `core.repositoryFormatVersion` setting.
> 
> This implies `extensions.worktreeConfig` become integral part of every
> repository format version, from the past until now and to the future,
> right?

>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The extensions.worktreeConfig extension was added in 58b284a (worktree:
> add per-worktree config files, 2018-10-21) and was somewhat documented
> in Documentation/git-config.txt. However, the extensions.worktreeConfig
> value was not specified further in the list of possible config keys. The
> location of the config.worktree file is not specified, and there are
> some precautions that should be mentioned clearly, but are only
> mentioned in git-worktree.txt.
>
> Expand the documentation to help users discover the complexities of
> extensions.worktreeConfig by adding details and cross links in these
> locations (relative to Documentation/):
>
> - config/extensions.txt
> - git-config.txt
> - git-worktree.txt
>
> The updates focus on items such as
>
> * $GIT_DIR/config.worktree takes precedence over $GIT_COMMON_DIR/config.
>
> * The core.worktree and core.bare=true settings are incorrect to have in
>   the common config file when extensions.worktreeConfig is enabled.
>
> * The sparse-checkout settings core.sparseCheckout[Cone] are recommended
>   to be set in the worktree config.
>
> As documented in 11664196ac ("Revert "check_repository_format_gently():
> refuse extensions for old repositories"", 2020-07-15), this extension
> must be considered regardless of the repository format version for
> historical reasons.
>
> A future change will update references to extensions.worktreeConfig
> within git-sparse-checkout.txt, but a behavior change is needed before
> making those updates.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/config/extensions.txt | 31 +++++++++++++++++++++++++++++
>  Documentation/git-config.txt        |  8 ++++++--
>  Documentation/git-worktree.txt      | 11 +++++++---
>  3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
> index 4e23d73cdca..5999dcb2a1f 100644
> --- a/Documentation/config/extensions.txt
> +++ b/Documentation/config/extensions.txt
> @@ -6,3 +6,34 @@ extensions.objectFormat::
>  Note that this setting should only be set by linkgit:git-init[1] or
>  linkgit:git-clone[1].  Trying to change it after initialization will not
>  work and will produce hard-to-diagnose issues.
> +
> +extensions.worktreeConfig::
> +       If enabled, then worktrees will load config settings from the
> +       `$GIT_DIR/config.worktree` file in addition to the
> +       `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
> +       `$GIT_DIR` are the same for the main worktree, while other
> +       worktrees have `$GIT_DIR` equal to
> +       `$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
> +       `config.worktree` file will override settings from any other
> +       config files.
> ++
> +When enabling `extensions.worktreeConfig`, you must be careful to move
> +certain values from the common config file to the main worktree's
> +`config.worktree` file, if present:
> ++
> +* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to
> +  `$GIT_COMMON_DIR/config.worktree`.
> +* If `core.bare` is true, then it must be moved from `$GIT_COMMON_DIR/config`
> +  to `$GIT_COMMON_DIR/config.worktree`.
> ++
> +It may also be beneficial to adjust the locations of `core.sparseCheckout`
> +and `core.sparseCheckoutCone` depending on your desire for customizable
> +sparse-checkout settings for each worktree. By default, the `git
> +sparse-checkout` builtin enables `extensions.worktreeConfig`, assigns
> +these config values on a per-worktree basis, and uses the
> +`$GIT_DIR/info/sparse-checkout` file to specify the sparsity for each
> +worktree independently. See linkgit:git-sparse-checkout[1] for more
> +details.
> ++
> +For historical reasons, `extensions.worktreeConfig` is respected
> +regardless of the `core.repositoryFormatVersion` setting.
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 2285effb363..95cefd5e399 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -141,9 +141,13 @@ from all available files.
>  See also <<FILES>>.
>
>  --worktree::
> -       Similar to `--local` except that `.git/config.worktree` is
> +       Similar to `--local` except that `$GIT_DIR/config.worktree` is
>         read from or written to if `extensions.worktreeConfig` is
> -       present. If not it's the same as `--local`.
> +       enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
> +       is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
> +       form `.git/worktrees/<worktree-name>/` for other worktrees. See

> +       linkgit:git-worktree[1] to learn how to enable
> +       `extensions.worktreeConfig`.
>
>  -f <config-file>::
>  --file <config-file>::
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 9e862fbcf79..ea0ee9f8bb5 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -286,8 +286,8 @@ CONFIGURATION FILE
>  ------------------
>  By default, the repository `config` file is shared across all working
>  trees. If the config variables `core.bare` or `core.worktree` are
> -already present in the config file, they will be applied to the main
> -working trees only.
> +present in the common config file and `extensions.worktreeConfig` is
> +disabled, then 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.:
> @@ -307,11 +307,16 @@ 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:
>
> - - `core.worktree` and `core.bare` should never be shared
> + - `core.worktree` should never be shared.
> +
> + - `core.bare` should not be shared unless the value is `core.bare=false`.

>   - `core.sparseCheckout` is recommended per working tree, unless you
>     are sure you always use sparse checkout for all working trees.
>
> +See the documentation of `extensions.worktreeConfig` in
> +linkgit:git-config[1] for more details.
> +
>  DETAILS
>  -------
>  Each linked working tree has a private sub-directory in the repository's
> --
> gitgitgadget

>>  --worktree::
>> -       Similar to `--local` except that `.git/config.worktree` is
>> +       Similar to `--local` except that `$GIT_DIR/config.worktree` is
>>         read from or written to if `extensions.worktreeConfig` is
>> -       present. If not it's the same as `--local`.
>> +       enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
>> +       is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
>> +       form `.git/worktrees/<worktree-name>/` for other worktrees. See
> 
> is of the form `$GIT_DIR/worktrees/<worktree-name>/`; .git isn't even
> a directory in other worktrees.

>> +       linkgit:git-worktree[1] to learn how to enable
>> +       `extensions.worktreeConfig`.
>>
>>  -f <config-file>::
>>  --file <config-file>::
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> index 9e862fbcf79..ea0ee9f8bb5 100644
>> --- a/Documentation/git-worktree.txt
>> +++ b/Documentation/git-worktree.txt
>> @@ -286,8 +286,8 @@ CONFIGURATION FILE
>>  ------------------
>>  By default, the repository `config` file is shared across all working
>>  trees. If the config variables `core.bare` or `core.worktree` are
>> -already present in the config file, they will be applied to the main
>> -working trees only.
>> +present in the common config file and `extensions.worktreeConfig` is
>> +disabled, then they will be applied to the main working trees only.
> 
> "main working trees"?  Is that an accidental plural?

>>  In order to have configuration specific to working trees, you can turn
>>  on the `worktreeConfig` extension, e.g.:
>> @@ -307,11 +307,16 @@ 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:
>>
>> - - `core.worktree` and `core.bare` should never be shared
>> + - `core.worktree` should never be shared.
>> +
>> + - `core.bare` should not be shared unless the value is `core.bare=false`.
> 
> The double negative makes for harder parsing.  Perhaps
>     - `core.bare` should not be shared if the value is `core.bare=true`
> ?

>>   - `core.sparseCheckout` is recommended per working tree, unless you
>>     are sure you always use sparse checkout for all working trees.
>>
>> +See the documentation of `extensions.worktreeConfig` in
>> +linkgit:git-config[1] for more details.
>> +
>>  DETAILS
>>  -------
>>  Each linked working tree has a private sub-directory in the repository's
>> --
>> gitgitgadget
> 
> Thanks for documenting these details; I had some very minor comments
> but this is well written and very helpful.

>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the config.worktree file, if enabled. An
> earlier change added init_worktree_config() as a helper to initialize
> extensions.worktreeConfig if not already enabled.
>
> Let the sparse-checkout builtin use these helpers instead of attempting to
> initialize the worktree config on its own. This changes behavior of 'git
> sparse-checkout set' in a few important ways:
>
>  1. Git will no longer upgrade the repository format, since this is not
>     a requirement for understanding extensions.worktreeConfig.
>
>  2. If the main worktree is bare, then this command will not put the
>     worktree in a broken state.
>
> The main reason to use worktree-specific config for the sparse-checkout
> builtin was to avoid enabling sparse-checkout patterns in one and
> causing a loss of files in another. If a worktree does not have a
> sparse-checkout patterns file, then the sparse-checkout logic will not
> kick in on that worktree.
>
> Reported-by: Sean Allred <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-sparse-checkout.txt | 24 ++++++++++++++++-------
>  builtin/sparse-checkout.c             | 28 +++++++++++++--------------
>  sparse-index.c                        | 10 +++-------
>  t/t1091-sparse-checkout-builtin.sh    |  4 ++--
>  4 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index b81dbe06543..c6eae3ec7fd 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -31,12 +31,20 @@ COMMANDS
>         Describe the patterns in the sparse-checkout file.
>
>  'set'::
> -       Enable the necessary config settings
> -       (extensions.worktreeConfig, core.sparseCheckout,
> -       core.sparseCheckoutCone) if they are not already enabled, and
> -       write a set of patterns to the sparse-checkout file from the
> -       list of arguments following the 'set' subcommand. Update the
> -       working directory to match the new patterns.
> +       Enable the necessary sparse-checkout config settings
> +       (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if

> +       they are not already enabled, and write a set of patterns to the
> +       sparse-checkout file from the list of arguments following the
> +       'set' subcommand. Update the working directory to match the new
> +       patterns.
> ++
> +To ensure that adjusting the sparse-checkout settings within a worktree
> +does not alter the sparse-checkout settings in other worktrees, the 'set'
> +subcommand will upgrade your repository config to use worktree-specific
> +config if not already present. The sparsity defined by the arguments to
> +the 'set' subcommand are stored in the worktree-specific sparse-checkout
> +file. See linkgit:git-worktree[1] and the documentation of
> +`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
>  +
>  When the `--stdin` option is provided, the patterns are read from
>  standard in as a newline-delimited list instead of from the arguments.
> @@ -73,7 +81,9 @@ interact with your repository until it is disabled.
>         By default, these patterns are read from the command-line arguments,
>         but they can be read from stdin using the `--stdin` option. When
>         `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
> -       as directory names as in the 'set' subcommand.
> +       as directory names as in the 'set' subcommand. The sparsity defined
> +       by the arguments to the 'add' subcommand are added to the patterns
> +       in the worktree-specific sparse-checkout file.

>  'reapply'::
>         Reapply the sparsity pattern rules to paths in the working tree.
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 679c1070368..314c8d61f80 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -15,6 +15,7 @@
>  #include "wt-status.h"
>  #include "quote.h"
>  #include "sparse-index.h"
> +#include "worktree.h"
>
>  static const char *empty_base = "";
>
> @@ -359,26 +360,23 @@ enum sparse_checkout_mode {
>
>  static int set_config(enum sparse_checkout_mode mode)
>  {
> -       const char *config_path;
> -
> -       if (upgrade_repository_format(1) < 0)
> -               die(_("unable to upgrade repository format to enable worktreeConfig"));

> -       if (git_config_set_gently("extensions.worktreeConfig", "true")) {
> -               error(_("failed to set extensions.worktreeConfig setting"));
> +       /* Update to use worktree config, if not already. */
> +       if (init_worktree_config(the_repository)) {
> +               error(_("failed to initialize worktree config"));
>                 return 1;
>         }
>
> -       config_path = git_path("config.worktree");
> -       git_config_set_in_file_gently(config_path,
> -                                     "core.sparseCheckout",
> -                                     mode ? "true" : NULL);
> -
> -       git_config_set_in_file_gently(config_path,
> -                                     "core.sparseCheckoutCone",
> -                                     mode == MODE_CONE_PATTERNS ? "true" : NULL);
> +       if (repo_config_set_worktree_gently(the_repository,
> +                                           "core.sparseCheckout",
> +                                           mode ? "true" : "false") ||
> +           repo_config_set_worktree_gently(the_repository,
> +                                           "core.sparseCheckoutCone",
> +                                           mode == MODE_CONE_PATTERNS ?
> +                                               "true" : "false"))
> +               return 1;
>
>         if (mode == MODE_NO_PATTERNS)
> -               set_sparse_index_config(the_repository, 0);
> +               return set_sparse_index_config(the_repository, 0);
>
>         return 0;
>  }
> diff --git a/sparse-index.c b/sparse-index.c
> index a1d505d50e9..e93609999e0 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -99,13 +99,9 @@ static int convert_to_sparse_rec(struct index_state *istate,
>
>  int set_sparse_index_config(struct repository *repo, int enable)
>  {
> -       int res;
> -       char *config_path = repo_git_path(repo, "config.worktree");
> -       res = git_config_set_in_file_gently(config_path,
> -                                           "index.sparse",
> -                                           enable ? "true" : NULL);
> -       free(config_path);
> -
> +       int res = repo_config_set_worktree_gently(repo,
> +                                                 "index.sparse",
> +                                                 enable ? "true" : "false");
>         prepare_repo_settings(repo);
>         repo->settings.sparse_index = enable;
>         return res;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 42776984fe7..be6ea4ffe33 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -117,7 +117,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
>                 cd bad-patterns &&
>                 git sparse-checkout init &&
>                 git sparse-checkout add dir &&
> -               git config core.sparseCheckoutCone true &&
> +               git config --worktree core.sparseCheckoutCone true &&
>                 test_must_fail git sparse-checkout add dir 2>err &&
>                 grep "existing sparse-checkout patterns do not use cone mode" err
>         )
> @@ -256,7 +256,7 @@ test_expect_success 'sparse-index enabled and disabled' '
>                 test_cmp expect actual &&
>
>                 git -C repo config --list >config &&
> -               ! grep index.sparse config
> +               test_cmp_config -C repo false index.sparse
>         )
>  '
>
> --
> gitgitgadget

> On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>  'set'::
>> -       Enable the necessary config settings
>> -       (extensions.worktreeConfig, core.sparseCheckout,
>> -       core.sparseCheckoutCone) if they are not already enabled, and
>> -       write a set of patterns to the sparse-checkout file from the
>> -       list of arguments following the 'set' subcommand. Update the
>> -       working directory to match the new patterns.
>> +       Enable the necessary sparse-checkout config settings
>> +       (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
> 
> and possibly index.sparse as well, right?

>> +       they are not already enabled, and write a set of patterns to the
>> +       sparse-checkout file from the list of arguments following the
>> +       'set' subcommand. Update the working directory to match the new
>> +       patterns.
>> ++
>> +To ensure that adjusting the sparse-checkout settings within a worktree
>> +does not alter the sparse-checkout settings in other worktrees, the 'set'
>> +subcommand will upgrade your repository config to use worktree-specific
>> +config if not already present. The sparsity defined by the arguments to
>> +the 'set' subcommand are stored in the worktree-specific sparse-checkout
>> +file. See linkgit:git-worktree[1] and the documentation of
>> +`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
>>  +
>>  When the `--stdin` option is provided, the patterns are read from
>>  standard in as a newline-delimited list instead of from the arguments.
>> @@ -73,7 +81,9 @@ interact with your repository until it is disabled.
>>         By default, these patterns are read from the command-line arguments,
>>         but they can be read from stdin using the `--stdin` option. When
>>         `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
>> -       as directory names as in the 'set' subcommand.
>> +       as directory names as in the 'set' subcommand. The sparsity defined
>> +       by the arguments to the 'add' subcommand are added to the patterns
>> +       in the worktree-specific sparse-checkout file.
> 
> This sentence addition makes your series conflict with patch 4 of my
> en/present-despite-skipped series.
> 
> The sentence also seems somewhat redundant with the first sentence of
> the paragraph (not quoted here).  Perhaps consider just dropping it to
> make it easier for Junio to integrate?

>>  'reapply'::
>>         Reapply the sparsity pattern rules to paths in the working tree.
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 679c1070368..314c8d61f80 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -15,6 +15,7 @@
>>  #include "wt-status.h"
>>  #include "quote.h"
>>  #include "sparse-index.h"
>> +#include "worktree.h"
>>
>>  static const char *empty_base = "";
>>
>> @@ -359,26 +360,23 @@ enum sparse_checkout_mode {
>>
>>  static int set_config(enum sparse_checkout_mode mode)
>>  {
>> -       const char *config_path;
>> -
>> -       if (upgrade_repository_format(1) < 0)
>> -               die(_("unable to upgrade repository format to enable worktreeConfig"));
> 
> Wait, this got added in mid-2020, since v2.28.0 -- and I missed it but
> it hasn't bit us yet??
> 
> I'm so sorry.  Earlier when I objected to setting this, I was worried
> it was a new change and would introduce some new failures, but clearly
> it was already introduced...and it turns out we've been fine.  So, I
> was wrong to worry about this.
> 
> (Which is to say, if you want to add it back, I'm fine with it given
> this info.  If you'd rather still take it out, I'm fine with that
> too.)

>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> When adding a new worktree, it is reasonable to expect that we want to
> use the current set of sparse-checkout settings for that new worktree.
> This is particularly important for repositories where the worktree would
> become too large to be useful. This is even more important when using
> partial clone as well, since we want to avoid downloading the missing
> blobs for files that should not be written to the new worktree.
>
> The only way to create such a worktree without this intermediate step of
> expanding the full worktree is to copy the sparse-checkout patterns and
> config settings during 'git worktree add'. Each worktree has its own
> sparse-checkout patterns, and the default behavior when the
> sparse-checkout file is missing is to include all paths at HEAD. Thus,
> we need to have patterns from somewhere, they might as well be the
> current worktree's patterns. These are then modified independently in
> the future.
>
> In addition to the sparse-checkout file, copy the worktree config file
> if worktree config is enabled and the file exists. This will copy over
> any important settings to ensure the new worktree behaves the same as
> the current one. The only exception we must continue to make is that
> core.bare and core.worktree should become unset in the worktree's config
> file.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/worktree.c                 | 60 ++++++++++++++++++++++++++++++
>  t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++----
>  t/t2400-worktree-add.sh            | 46 ++++++++++++++++++++++-
>  3 files changed, 127 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 2838254f7f2..dc9cd6decc8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
>         strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
>         write_file(sb.buf, "../..");
>
> +       /*
> +        * If the current worktree has sparse-checkout enabled, then copy
> +        * the sparse-checkout patterns from the current worktree.
> +        */
> +       if (core_apply_sparse_checkout) {
> +               char *from_file = git_pathdup("info/sparse-checkout");
> +               char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
> +                                       realpath.buf, name);
> +
> +               if (file_exists(from_file)) {
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666))
> +                               error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
> +                                     from_file, to_file);
> +               }
> +
> +               free(from_file);
> +               free(to_file);
> +       }
> +
> +       /*
> +        * If we are using worktree config, then copy all current config
> +        * values from the current worktree into the new one, that way the
> +        * new worktree behaves the same as this one.
> +        */
> +       if (repository_format_worktree_config) {
> +               char *from_file = git_pathdup("config.worktree");
> +               char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
> +                                       realpath.buf, name);
> +
> +               if (file_exists(from_file)) {
> +                       struct config_set cs = { { 0 }};
> +                       const char *str_value;
> +                       int bool_value;
> +
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666))
> +                               die(_("failed to copy worktree config from '%s' to '%s'"),
> +                                   from_file, to_file);
> +
> +                       git_configset_init(&cs);
> +                       git_configset_add_file(&cs, from_file);
> +
> +                       if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
> +                           bool_value &&
> +                           git_config_set_multivar_in_file_gently(
> +                                       to_file, "core.bare", NULL, "true", 0))
> +                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
> +                       if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
> +                           git_config_set_in_file_gently(to_file,
> +                                                         "core.worktree", NULL))
> +                               error(_("failed to unset 'core.worktree' in '%s'"), to_file);
> +
> +                       git_configset_clear(&cs);
> +               }
> +
> +               free(from_file);
> +               free(to_file);
> +       }
> +
>         strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
>         strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
>         cp.git_cmd = 1;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index be6ea4ffe33..d929772be96 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -146,9 +146,9 @@ test_expect_success 'interaction with clone --no-checkout (unborn index)' '
>  '
>
>  test_expect_success 'set enables config' '
> -       git init empty-config &&
> +       git init worktree-config &&
>         (
> -               cd empty-config &&
> +               cd worktree-config &&
>                 test_commit test file &&
>                 test_path_is_missing .git/config.worktree &&
>                 git sparse-checkout set nothing &&
> @@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' '
>         check_files repo "a folder1 folder2"
>  '
>
> +test_expect_success 'worktree: add copies sparse-checkout patterns' '
> +       cat repo/.git/info/sparse-checkout >old &&
> +       test_when_finished cp old repo/.git/info/sparse-checkout &&
> +       test_when_finished git -C repo worktree remove ../worktree &&
> +       git -C repo sparse-checkout set "/*" &&

> +       git -C repo worktree add --quiet ../worktree 2>err &&
> +       test_must_be_empty err &&
> +       new=repo/.git/worktrees/worktree/info/sparse-checkout &&
> +       test_path_is_file $new &&
> +       test_cmp repo/.git/info/sparse-checkout $new &&
> +       git -C worktree sparse-checkout set --cone &&
> +       test_cmp_config -C worktree true core.sparseCheckoutCone &&
> +       test_must_fail git -C repo core.sparseCheckoutCone
> +'
> +
>  test_expect_success 'cone mode: match patterns' '
>         git -C repo config --worktree core.sparseCheckoutCone true &&
>         rm -rf repo/a repo/folder1 repo/folder2 &&
> @@ -520,13 +535,13 @@ test_expect_success 'interaction with submodules' '
>  '
>
>  test_expect_success 'different sparse-checkouts with worktrees' '
> +       git -C repo sparse-checkout set --cone deep folder1 &&
>         git -C repo worktree add --detach ../worktree &&
> -       check_files worktree "a deep folder1 folder2" &&
> -       git -C worktree sparse-checkout init --cone &&
> -       git -C repo sparse-checkout set folder1 &&
> -       git -C worktree sparse-checkout set deep/deeper1 &&
> -       check_files repo a folder1 &&
> -       check_files worktree a deep
> +       check_files worktree "a deep folder1" &&
> +       git -C repo sparse-checkout set --cone folder1 &&
> +       git -C worktree sparse-checkout set --cone deep/deeper1 &&
> +       check_files repo "a folder1" &&
> +       check_files worktree "a deep"
>  '
>
>  test_expect_success 'set using filename keeps file on-disk' '
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 37ad79470fb..3fb5b21b943 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -165,8 +165,50 @@ test_expect_success '"add" default branch of a bare repo' '
>         (
>                 git clone --bare . bare2 &&
>                 cd bare2 &&
> -               git worktree add ../there3 main
> -       )
> +               git worktree add ../there3 main &&
> +               cd ../there3 &&
> +               git status
> +       ) &&
> +       cat >expect <<-EOF &&
> +       init.t
> +       EOF
> +       ls there3 >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '"add" to bare repo with worktree config' '
> +       (
> +               git clone --bare . bare3 &&
> +               cd bare3 &&
> +               git config extensions.worktreeconfig true &&
> +               git config --worktree core.bare true &&
> +               git config --worktree core.worktree "$(pwd)" &&
> +               git config --worktree bogus.key value &&
> +               git config --unset core.bare &&
> +               git worktree add ../there4 main &&
> +               cd ../there4 &&
> +               git status &&
> +               git worktree add --detach ../there5 &&
> +               cd ../there5 &&
> +               git status
> +       ) &&
> +
> +       # the worktree has the arbitrary value copied.
> +       test_cmp_config -C there4 value bogus.key &&
> +       test_cmp_config -C there5 value bogus.key &&
> +
> +       # however, core.bare and core.worktree were removed.
> +       test_must_fail git -C there4 config core.bare &&
> +       test_must_fail git -C there4 config core.worktree &&
> +
> +       cat >expect <<-EOF &&
> +       init.t
> +       EOF
> +
> +       ls there4 >actual &&
> +       test_cmp expect actual &&
> +       ls there5 >actual &&
> +       test_cmp expect actual
>  '
>
>  test_expect_success 'checkout with grafts' '
> --
> gitgitgadget

> 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 or core.worktree are set in the
> common config file. This series fixes this, but also puts in place some
> helpers to prevent this from happening in the future.
>
> ATTENTION: I have significantly redesigned the series since previous
> versions, so most of this cover letter is new.
>
>  * Patch 1 updates documentation around extensions.worktreeConfig in a few
>    places to improve discoverability. Several cross links are added to make
>    it easy to find the related areas. (The documentation for the changes to
>    'git sparse-checkout' are delayed to patch 4.)
>
>  * Patch 2 introduces the init_worktree_config() helper which follows the
>    documented instructions to enable extensions.worktreeConfig as well as
>    move the core.bare and core.worktree config values. This update does not
>    modify core.repositoryFormatVersion, since this is not needed
>    specifically for extensions.worktreeConfig.
>
>  * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
>    can internally adjust a config value within a worktree, at least if
>    extensions.worktreeConfig is enabled. (It will write to the common config
>    file if the extension is not enabled.)
>
>  * Patch 4 modifies the sparse-checkout builtin to use
>    init_worktree_config() and repo_config_set_worktree_gently() in ways that
>    fix the reported bug. The behavior change here is that it will no longer
>    upgrade the repository format version, since that is not needed for
>    extensions.worktreeConfig.
>
>  * Patch 5 updates 'git worktree add' to copy the worktree config from the
>    current worktree to the new one (while unsetting core.bare=true and
>    core.worktree=*) along with copying the sparse-checkout patterns file.
>
> [1]
> https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
> [2]
> https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/
>
>
> Updates in v4
> =============
>
>  * Rebased to v2.35.0
>  * Fixed memory leak (was leaking repo_git_path() result)
>  * Added additional documentation updates so curious users can discover the
>    intricacies of extensions.worktreeConfig from multiple entry points.
>  * Significantly reduced the amount of changes to config.c.
>  * 'git sparse-checkout' no longer upgrades the repository format.
>  * Dropped the update to upgrade_repository_format(), since it is not
>    needed.
>  * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
>    helper method called by 'git sparse-checkout'
>  * Many others because of the significant changes required by the above
>    items.

> Thanks, -Stolee
>
> Derrick Stolee (5):
>   Documentation: add extensions.worktreeConfig details
>   worktree: create init_worktree_config()
>   config: add repo_config_set_worktree_gently()
>   sparse-checkout: set worktree-config correctly
>   worktree: copy sparse-checkout patterns and config on add
>
>  Documentation/config/extensions.txt   | 31 ++++++++++++
>  Documentation/git-config.txt          |  8 ++-
>  Documentation/git-sparse-checkout.txt | 24 ++++++---
>  Documentation/git-worktree.txt        | 11 +++--
>  builtin/sparse-checkout.c             | 28 +++++------
>  builtin/worktree.c                    | 60 +++++++++++++++++++++++
>  config.c                              | 35 ++++++++++++--
>  config.h                              |  8 +++
>  sparse-index.c                        | 10 ++--
>  t/t1091-sparse-checkout-builtin.sh    | 35 ++++++++++----
>  t/t2400-worktree-add.sh               | 46 +++++++++++++++++-
>  worktree.c                            | 70 +++++++++++++++++++++++++++
>  worktree.h                            | 19 ++++++++
>  13 files changed, 336 insertions(+), 49 deletions(-)
>
>
> base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1101
>
> Range-diff vs v3:
>
>  1:  749ba67d21e < -:  ----------- setup: use a repository when upgrading format
>  2:  61b96937016 < -:  ----------- config: make some helpers repo-aware
>  -:  ----------- > 1:  459e09dedd7 Documentation: add extensions.worktreeConfig details
>  3:  e2a0a458115 ! 2:  d262a76b448 worktree: add 'init-worktree-config' subcommand
>      @@ Metadata
>       Author: Derrick Stolee <dstolee@microsoft.com>
>
>        ## Commit message ##
>      -    worktree: add 'init-worktree-config' subcommand
>      +    worktree: create init_worktree_config()
>
>      -    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.
>      +    Upgrading a repository to use extensions.worktreeConfig is non-trivial.
>      +    There are several steps involved, including moving some config settings
>      +    from the common config file to the main worktree's config.worktree file.
>      +    The previous change updated the documentation with all of these details.
>
>      -    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.
>      +    Commands such as 'git sparse-checkout set' upgrade the repository to use
>      +    extensions.worktreeConfig without following these steps, causing some
>      +    user pain in some special cases.
>
>      -    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.
>      +    Create a helper method, init_worktree_config(), that will be used in a
>      +    later change to fix this behavior within 'git sparse-checkout set'. The
>      +    method is carefully documented in worktree.h.
>
>      -    To help resolve this transition, create the 'git worktree
>      -    init-worktree-config' helper. This new subcommand does the following:
>      +    Note that we do _not_ upgrade the repository format version to 1 during
>      +    this process. The worktree config extension must be considered by Git
>      +    and third-party tools even if core.repositoryFormatVersion is 0 for
>      +    historical reasons documented in 11664196ac ("Revert
>      +    "check_repository_format_gently(): refuse extensions for old
>      +    repositories"", 2020-07-15). This is a special case for this extension,
>      +    and newer extensions (such as extensions.objectFormat) still need to
>      +    upgrade the repository format version.
>
>      -     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 <sunshine@sunshineco.com>
>           Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>
>      - ## 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 ##
>      + ## worktree.c ##
>       @@
>      + #include "worktree.h"
>      + #include "dir.h"
>      + #include "wt-status.h"
>      ++#include "config.h"
>
>      - 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;
>      + 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 int move_config_setting(const char *key, const char *value,
>       +                        const char *from_file, const char *to_file)
>       +{
>      @@ builtin/worktree.c: static int repair(int ac, const char **av, const char *prefi
>       + return 0;
>       +}
>       +
>      -+static int init_worktree_config(int ac, const char **av, const char *prefix)
>      ++int init_worktree_config(struct repository *r)
>       +{
>      -+ struct repository *r = the_repository;
>      -+ struct option options[] = {
>      -+         OPT_END()
>      -+ };
>       + int res = 0;
>       + int bare = 0;
>      -+ struct config_set cs = { 0 };
>      ++ struct config_set cs = { { 0 } };
>       + 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, common_config_file);
>      -+
>       + /*
>      -+  * If the format and extension are already enabled, then we can
>      -+  * skip the upgrade process.
>      ++  * If the extension is already enabled, then we can skip the
>      ++  * upgrade process.
>       +  */
>       + if (repository_format_worktree_config)
>       +         return 0;
>      ++ if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
>      ++         return error(_("failed to set extensions.worktreeConfig setting"));
>       +
>      -+ 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;
>      -+ }
>      ++ git_configset_init(&cs);
>      ++ git_configset_add_file(&cs, common_config_file);
>       +
>       + /*
>       +  * If core.bare is true in the common config file, then we need to
>      @@ builtin/worktree.c: static int repair(int ac, const char **av, const char *prefi
>       +                 goto cleanup;
>       + }
>       +
>      ++ /*
>      ++  * Ensure that we use worktree config for the remaining lifetime
>      ++  * of the current process.
>      ++  */
>      ++ repository_format_worktree_config = 1;
>      ++
>       +cleanup:
>       + git_configset_clear(&cs);
>       + 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"))
>
>      - ## 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
>      + ## worktree.h ##
>      +@@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt,
>      +                   struct strbuf *sb,
>      +                   const char *refname);
>      +
>      ++/**
>      ++ * Enable worktree config for the first time. This will make the following
>      ++ * adjustments:
>      ++ *
>      ++ * 1. Add extensions.worktreeConfig=true in the common config file.
>      ++ *
>      ++ * 2. If the common config file has a core.worktree value or core.bare is
>      ++ *    set to true, then those values are moved to the main worktree's
>      ++ *    config.worktree file.
>      ++ *
>      ++ * If extensions.worktreeConfig is already true, then this method
>      ++ * terminates early without any of the above steps. The existing config
>      ++ * arrangement is assumed to be intentional.
>      ++ *
>      ++ * Returns 0 on success. Reports an error message and returns non-zero
>      ++ * if any of these steps fail.
>      ++ */
>      ++int init_worktree_config(struct repository *r);
>      ++
>      + #endif
>  4:  45316cd01c9 ! 3:  110d5e0546c config: add repo_config_set_worktree_gently()
>      @@ config.c: int git_config_set_gently(const char *key, const char *value)
>       +         free(file);
>       +         return ret;
>       + }
>      -+ return repo_config_set_gently(r, key, value);
>      ++ return repo_config_set_multivar_gently(r, key, value, NULL, 0);
>       +}
>       +
>        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);
>      +  git_config_set_multivar(key, value, NULL, 0);
>      +@@ config.c: 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,
>      +-                                               flags);
>      ++ 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)
>      ++{
>      ++ char *file = repo_git_path(r, "config");
>      ++ int res = git_config_set_multivar_in_file_gently(file,
>      ++                                                  key, value,
>      ++                                                  value_pattern,
>      ++                                                  flags);
>      ++ free(file);
>      ++ return res;
>      + }
>      +
>        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,
>      ++ git_config_set_multivar_in_file(git_path("config"),
>      ++                                 key, value, value_pattern,
>      +                                  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 *);
>        /**
>         * 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
>      +@@ config.h: 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 repo_config_set_gently(struct repository *, const char *, const char *);
>      ++int repo_config_set_multivar_gently(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);
>
>        /**
>  5:  b200819c1bb ! 4:  fbfaa17797c sparse-checkout: use repo_config_set_worktree_gently()
>      @@ Metadata
>       Author: Derrick Stolee <dstolee@microsoft.com>
>
>        ## Commit message ##
>      -    sparse-checkout: use repo_config_set_worktree_gently()
>      +    sparse-checkout: set worktree-config correctly
>
>           The previous change added repo_config_set_worktree_gently() to assist
>      -    writing config values into the worktree.config file, if enabled.
>      +    writing config values into the config.worktree file, if enabled. An
>      +    earlier change added init_worktree_config() as a helper to initialize
>      +    extensions.worktreeConfig if not already enabled.
>
>      -    Let the sparse-checkout builtin use this helper instead of attempting to
>      +    Let the sparse-checkout builtin use these helpers instead of attempting to
>           initialize the worktree config on its own. This changes behavior of 'git
>           sparse-checkout set' in a few important ways:
>
>      -     1. Git will no longer upgrade the repository format and add the
>      -        worktree config extension. The user should run 'git worktree
>      -        init-worktree-config' to enable this feature.
>      +     1. Git will no longer upgrade the repository format, since this is not
>      +        a requirement for understanding extensions.worktreeConfig.
>
>      -     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
>      +     2. If the main worktree is bare, then this command will not put the
>               worktree in a broken state.
>
>           The main reason to use worktree-specific config for the sparse-checkout
>      @@ Commit message
>           sparse-checkout patterns file, then the sparse-checkout logic will not
>           kick in on that worktree.
>
>      -    This new logic introduces a new user pattern that could lead to some
>      -    confusion. Suppose a user has not upgraded to worktree config and
>      -    follows these steps in order:
>      -
>      -     1. Enable sparse-checkout in a worktree.
>      -
>      -     2. Disable sparse-checkout in that worktree without deleting that
>      -        worktree's sparse-checkout file.
>      -
>      -     3. Enable sparse-checkout in another worktree.
>      -
>      -    After these steps, the first worktree will have sparse-checkout enabled
>      -    with whatever patterns exist. The worktree does not immediately have
>      -    those patterns applied, but a variety of Git commands would apply the
>      -    sparse-checkout patterns and update the worktree state to reflect those
>      -    patterns. This situation is likely very rare and the workaround is to
>      -    upgrade to worktree specific config on purpose. Users already in this
>      -    state used the sparse-checkout builtin with a version that upgraded to
>      -    worktree config, anyway.
>      -
>           Reported-by: Sean Allred <allred.sean@gmail.com>
>           Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>           Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>
>      + ## Documentation/git-sparse-checkout.txt ##
>      +@@ Documentation/git-sparse-checkout.txt: COMMANDS
>      +  Describe the patterns in the sparse-checkout file.
>      +
>      + 'set'::
>      +- Enable the necessary config settings
>      +- (extensions.worktreeConfig, core.sparseCheckout,
>      +- core.sparseCheckoutCone) if they are not already enabled, and
>      +- write a set of patterns to the sparse-checkout file from the
>      +- list of arguments following the 'set' subcommand. Update the
>      +- working directory to match the new patterns.
>      ++ Enable the necessary sparse-checkout config settings
>      ++ (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
>      ++ they are not already enabled, and write a set of patterns to the
>      ++ sparse-checkout file from the list of arguments following the
>      ++ 'set' subcommand. Update the working directory to match the new
>      ++ patterns.
>      +++
>      ++To ensure that adjusting the sparse-checkout settings within a worktree
>      ++does not alter the sparse-checkout settings in other worktrees, the 'set'
>      ++subcommand will upgrade your repository config to use worktree-specific
>      ++config if not already present. The sparsity defined by the arguments to
>      ++the 'set' subcommand are stored in the worktree-specific sparse-checkout
>      ++file. See linkgit:git-worktree[1] and the documentation of
>      ++`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
>      + +
>      + When the `--stdin` option is provided, the patterns are read from
>      + standard in as a newline-delimited list instead of from the arguments.
>      +@@ Documentation/git-sparse-checkout.txt: interact with your repository until it is disabled.
>      +  By default, these patterns are read from the command-line arguments,
>      +  but they can be read from stdin using the `--stdin` option. When
>      +  `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
>      +- as directory names as in the 'set' subcommand.
>      ++ as directory names as in the 'set' subcommand. The sparsity defined
>      ++ by the arguments to the 'add' subcommand are added to the patterns
>      ++ in the worktree-specific sparse-checkout file.
>      +
>      + 'reapply'::
>      +  Reapply the sparsity pattern rules to paths in the working tree.
>      +
>        ## builtin/sparse-checkout.c ##
>      +@@
>      + #include "wt-status.h"
>      + #include "quote.h"
>      + #include "sparse-index.h"
>      ++#include "worktree.h"
>      +
>      + static const char *empty_base = "";
>      +
>       @@ builtin/sparse-checkout.c: 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)
>      +- if (upgrade_repository_format(1) < 0)
>       -         die(_("unable to upgrade repository format to enable worktreeConfig"));
>       - if (git_config_set_gently("extensions.worktreeConfig", "true")) {
>       -         error(_("failed to set extensions.worktreeConfig setting"));
>      -+ 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"))
>      ++ /* Update to use worktree config, if not already. */
>      ++ if (init_worktree_config(the_repository)) {
>      ++         error(_("failed to initialize worktree config"));
>                 return 1;
>      -- }
>      --
>      +  }
>      +
>       - config_path = git_path("config.worktree");
>       - git_config_set_in_file_gently(config_path,
>       -                               "core.sparseCheckout",
>      @@ builtin/sparse-checkout.c: enum sparse_checkout_mode {
>       - git_config_set_in_file_gently(config_path,
>       -                               "core.sparseCheckoutCone",
>       -                               mode == MODE_CONE_PATTERNS ? "true" : NULL);
>      ++ if (repo_config_set_worktree_gently(the_repository,
>      ++                                     "core.sparseCheckout",
>      ++                                     mode ? "true" : "false") ||
>      ++     repo_config_set_worktree_gently(the_repository,
>      ++                                     "core.sparseCheckoutCone",
>      ++                                     mode == MODE_CONE_PATTERNS ?
>      ++                                         "true" : "false"))
>      ++         return 1;
>
>         if (mode == MODE_NO_PATTERNS)
>       -         set_sparse_index_config(the_repository, 0);
>      @@ 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 'interaction with clone --no-checkout (unborn index)' '
>      - '
>      -
>      - test_expect_success 'set enables config' '
>      -- git init empty-config &&
>      -+ git init initial-config &&
>      -  (
>      --         cd empty-config &&
>      -+         cd initial-config &&
>      -+         test_commit file file &&
>      -+         mkdir dir &&
>      -+         test_commit dir dir/file &&
>      -+         git worktree add --detach ../initial-worktree &&
>      -+         git sparse-checkout set --cone
>      -+ ) &&
>      -+ test_cmp_config -C initial-config true core.sparseCheckout &&
>      -+ test_cmp_config -C initial-worktree true core.sparseCheckout &&
>      -+ test_cmp_config -C initial-config true core.sparseCheckoutCone &&
>      -+ test_cmp_config -C initial-worktree true core.sparseCheckoutCone &&
>      -+
>      -+ # initial-config has a sparse-checkout file
>      -+ # that only contains files at root.
>      -+ ls initial-config >only-file &&
>      -+ cat >expect <<-EOF &&
>      -+ file
>      -+ EOF
>      -+ test_cmp expect only-file &&
>      -+
>      -+ # initial-worktree does not have its own sparse-checkout
>      -+ # file, so the repply does not modify the worktree at all.
>      -+ git -C initial-worktree sparse-checkout reapply &&
>      -+ ls initial-worktree >all &&
>      -+ cat >expect <<-EOF &&
>      -+ dir
>      -+ file
>      -+ EOF
>      -+ test_cmp expect all
>      -+'
>      -+
>      -+test_expect_success 'set enables worktree config, if enabled' '
>      -+ git init worktree-config &&
>      -+ (
>      -+         cd worktree-config &&
>      -          test_commit test file &&
>      --         test_path_is_missing .git/config.worktree &&
>      --         git sparse-checkout set nothing &&
>      --         test_path_is_file .git/config.worktree &&
>      --         test_cmp_config true core.sparseCheckout
>      -- )
>      -+         git worktree add --detach ../worktree-config2 &&
>      -+         git worktree init-worktree-config &&
>      -+         git sparse-checkout set --cone &&
>      -+         git config --worktree core.sparseCheckout &&
>      -+         git config --worktree core.sparseCheckoutCone
>      -+ ) &&
>      -+ test_cmp_config -C worktree-config true core.sparseCheckout &&
>      -+ test_must_fail git -C worktree-config2 core.sparseCheckout &&
>      -+ test_cmp_config -C worktree-config true core.sparseCheckoutCone &&
>      -+ test_must_fail git -C worktree-config2 core.sparseCheckoutCone
>      - '
>      -
>      - test_expect_success 'set sparse-checkout using builtin' '
>      -@@ 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 'switching to cone mode with non-cone mode patterns' '
>      +          cd bad-patterns &&
>      +          git sparse-checkout init &&
>      +          git sparse-checkout add dir &&
>      +-         git config core.sparseCheckoutCone true &&
>      ++         git config --worktree core.sparseCheckoutCone true &&
>      +          test_must_fail git sparse-checkout add dir 2>err &&
>      +          grep "existing sparse-checkout patterns do not use cone mode" err
>      +  )
>       @@ 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 &&
>      +          test_cmp expect actual &&
>      +
>                 git -C repo config --list >config &&
>       -         ! grep index.sparse config
>       +         test_cmp_config -C repo false index.sparse
>         )
>        '
>
>      -@@ 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 ! 5:  bb9e550ff3d worktree: copy sparse-checkout patterns and config on add
>      @@ Commit message
>           In addition to the sparse-checkout file, copy the worktree config file
>           if worktree config is enabled and the file exists. This will copy over
>           any important settings to ensure the new worktree behaves the same as
>      -    the current one.
>      +    the current one. The only exception we must continue to make is that
>      +    core.bare and core.worktree should become unset in the worktree's config
>      +    file.
>
>           Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>
>      @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
>       + }
>       +
>       + /*
>      -+  * If we are using worktree config, then copy all currenct config
>      ++  * If we are using worktree config, then copy all current config
>       +  * values from the current worktree into the new one, that way the
>       +  * new worktree behaves the same as this one.
>       +  */
>      @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
>       +                                 realpath.buf, name);
>       +
>       +         if (file_exists(from_file)) {
>      ++                 struct config_set cs = { { 0 }};
>      ++                 const char *str_value;
>      ++                 int bool_value;
>      ++
>       +                 if (safe_create_leading_directories(to_file) ||
>       +                     copy_file(to_file, from_file, 0666))
>      -+                         error(_("failed to copy worktree config from '%s' to '%s'"),
>      -+                               from_file, to_file);
>      ++                         die(_("failed to copy worktree config from '%s' to '%s'"),
>      ++                             from_file, to_file);
>      ++
>      ++                 git_configset_init(&cs);
>      ++                 git_configset_add_file(&cs, from_file);
>      ++
>      ++                 if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
>      ++                     bool_value &&
>      ++                     git_config_set_multivar_in_file_gently(
>      ++                                 to_file, "core.bare", NULL, "true", 0))
>      ++                         error(_("failed to unset 'core.bare' in '%s'"), to_file);
>      ++                 if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
>      ++                     git_config_set_in_file_gently(to_file,
>      ++                                                   "core.worktree", NULL))
>      ++                         error(_("failed to unset 'core.worktree' in '%s'"), to_file);
>      ++
>      ++                 git_configset_clear(&cs);
>       +         }
>       +
>       +         free(from_file);
>      @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
>         cp.git_cmd = 1;
>
>        ## t/t1091-sparse-checkout-builtin.sh ##
>      -@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'set enables config' '
>      +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'interaction with clone --no-checkout (unborn index)' '
>        '
>
>      - test_expect_success 'set enables worktree config, if enabled' '
>      -+ git init worktree-patterns &&
>      -+ (
>      -+         cd worktree-patterns &&
>      -+         test_commit test file &&
>      -+         mkdir dir dir2 &&
>      -+         test_commit dir dir/file &&
>      -+         test_commit dir2 dir2/file &&
>      -+
>      -+         # By initializing the worktree config here...
>      -+         git worktree init-worktree-config &&
>      -+
>      -+         # This set command places config values in worktree-
>      -+         # specific config...
>      -+         git sparse-checkout set --cone dir &&
>      -+
>      -+         # Which must be copied, along with the sparse-checkout
>      -+         # patterns, here.
>      -+         git worktree add --detach ../worktree-patterns2
>      -+ ) &&
>      -+ test_cmp_config -C worktree-patterns true core.sparseCheckout &&
>      -+ test_cmp_config -C worktree-patterns2 true core.sparseCheckout &&
>      -+ test_cmp_config -C worktree-patterns true core.sparseCheckoutCone &&
>      -+ test_cmp_config -C worktree-patterns2 true core.sparseCheckoutCone &&
>      -+ test_cmp worktree-patterns/.git/info/sparse-checkout \
>      -+          worktree-patterns/.git/worktrees/worktree-patterns2/info/sparse-checkout &&
>      -+
>      -+ ls worktree-patterns >expect &&
>      -+ ls worktree-patterns2 >actual &&
>      -+ test_cmp expect actual &&
>      -+
>      -+ # Double check that the copy works from a non-main worktree.
>      -+ (
>      -+         cd worktree-patterns2 &&
>      -+         git sparse-checkout set dir2 &&
>      -+         git worktree add --detach ../worktree-patterns3
>      -+ ) &&
>      -+ test_cmp_config -C worktree-patterns3 true core.sparseCheckout &&
>      -+ test_cmp_config -C worktree-patterns3 true core.sparseCheckoutCone &&
>      -+ test_cmp worktree-patterns/.git/worktrees/worktree-patterns2/info/sparse-checkout \
>      -+          worktree-patterns/.git/worktrees/worktree-patterns3/info/sparse-checkout &&
>      -+
>      -+ ls worktree-patterns2 >expect &&
>      -+ ls worktree-patterns3 >actual &&
>      -+ test_cmp expect actual
>      + test_expect_success 'set enables config' '
>      +- git init empty-config &&
>      ++ git init worktree-config &&
>      +  (
>      +-         cd empty-config &&
>      ++         cd worktree-config &&
>      +          test_commit test file &&
>      +          test_path_is_missing .git/config.worktree &&
>      +          git sparse-checkout set nothing &&
>      +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout' '
>      +  check_files repo "a folder1 folder2"
>      + '
>      +
>      ++test_expect_success 'worktree: add copies sparse-checkout patterns' '
>      ++ cat repo/.git/info/sparse-checkout >old &&
>      ++ test_when_finished cp old repo/.git/info/sparse-checkout &&
>      ++ test_when_finished git -C repo worktree remove ../worktree &&
>      ++ git -C repo sparse-checkout set "/*" &&
>      ++ git -C repo worktree add --quiet ../worktree 2>err &&
>      ++ test_must_be_empty err &&
>      ++ new=repo/.git/worktrees/worktree/info/sparse-checkout &&
>      ++ test_path_is_file $new &&
>      ++ test_cmp repo/.git/info/sparse-checkout $new &&
>      ++ git -C worktree sparse-checkout set --cone &&
>      ++ test_cmp_config -C worktree true core.sparseCheckoutCone &&
>      ++ test_must_fail git -C repo core.sparseCheckoutCone
>       +'
>       +
>      -+test_expect_success 'worktree add copies sparse-checkout patterns' '
>      -  git init worktree-config &&
>      -  (
>      -          cd worktree-config &&
>      + test_expect_success 'cone mode: match patterns' '
>      +  git -C repo config --worktree core.sparseCheckoutCone true &&
>      +  rm -rf repo/a repo/folder1 repo/folder2 &&
>       @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'interaction with submodules' '
>      + '
>
>        test_expect_success 'different sparse-checkouts with worktrees' '
>      ++ git -C repo sparse-checkout set --cone deep folder1 &&
>         git -C repo worktree add --detach ../worktree &&
>       - check_files worktree "a deep folder1 folder2" &&
>      -+ check_files worktree "a folder1" &&
>      -  git -C worktree sparse-checkout init --cone &&
>      +- git -C worktree sparse-checkout init --cone &&
>       - git -C repo sparse-checkout set folder1 &&
>      -+ git -C repo sparse-checkout set folder1 folder2 &&
>      -  git -C worktree sparse-checkout set deep/deeper1 &&
>      +- git -C worktree sparse-checkout set deep/deeper1 &&
>       - check_files repo a folder1 &&
>      -+ check_files repo a folder1 folder2 &&
>      -  check_files worktree a deep
>      +- check_files worktree a deep
>      ++ check_files worktree "a deep folder1" &&
>      ++ git -C repo sparse-checkout set --cone folder1 &&
>      ++ git -C worktree sparse-checkout set --cone deep/deeper1 &&
>      ++ check_files repo "a folder1" &&
>      ++ check_files worktree "a deep"
>      + '
>      +
>      + test_expect_success 'set using filename keeps file on-disk' '
>      +
>      + ## t/t2400-worktree-add.sh ##
>      +@@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare repo' '
>      +  (
>      +          git clone --bare . bare2 &&
>      +          cd bare2 &&
>      +-         git worktree add ../there3 main
>      +- )
>      ++         git worktree add ../there3 main &&
>      ++         cd ../there3 &&
>      ++         git status
>      ++ ) &&
>      ++ cat >expect <<-EOF &&
>      ++ init.t
>      ++ EOF
>      ++ ls there3 >actual &&
>      ++ test_cmp expect actual
>      ++'
>      ++
>      ++test_expect_success '"add" to bare repo with worktree config' '
>      ++ (
>      ++         git clone --bare . bare3 &&
>      ++         cd bare3 &&
>      ++         git config extensions.worktreeconfig true &&
>      ++         git config --worktree core.bare true &&
>      ++         git config --worktree core.worktree "$(pwd)" &&
>      ++         git config --worktree bogus.key value &&
>      ++         git config --unset core.bare &&
>      ++         git worktree add ../there4 main &&
>      ++         cd ../there4 &&
>      ++         git status &&
>      ++         git worktree add --detach ../there5 &&
>      ++         cd ../there5 &&
>      ++         git status
>      ++ ) &&
>      ++
>      ++ # the worktree has the arbitrary value copied.
>      ++ test_cmp_config -C there4 value bogus.key &&
>      ++ test_cmp_config -C there5 value bogus.key &&
>      ++
>      ++ # however, core.bare and core.worktree were removed.
>      ++ test_must_fail git -C there4 config core.bare &&
>      ++ test_must_fail git -C there4 config core.worktree &&
>      ++
>      ++ cat >expect <<-EOF &&
>      ++ init.t
>      ++ EOF
>      ++
>      ++ ls there4 >actual &&
>      ++ test_cmp expect actual &&
>      ++ ls there5 >actual &&
>      ++ test_cmp expect actual
>        '
>
>      + test_expect_success 'checkout with grafts' '
>
> --
> gitgitgadget

> On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This series is now based on v2.35.0 since that contains all of the necessary
>> topics.
> 
> Heads up for the maintainer: has a minor textual conflict with
> en/present-despite-skipped (this series modifies the end of a
> paragraph that the other series modifies the beginning of).

>> 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 or core.worktree are set in the
>> common config file. This series fixes this, but also puts in place some
>> helpers to prevent this from happening in the future.
> This series has become pretty solid.  I had only very minor comments
> on the patches.  Thanks for working on this.

> [...]
> Expand the documentation to help users discover the complexities of
> extensions.worktreeConfig by adding details and cross links in these
> locations (relative to Documentation/):
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

> diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
> @@ -6,3 +6,34 @@ extensions.objectFormat::
> +extensions.worktreeConfig::
> +       If enabled, then worktrees will load config settings from the
> +       `$GIT_DIR/config.worktree` file in addition to the
> +       `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
> +       `$GIT_DIR` are the same for the main worktree, while other
> +       worktrees have `$GIT_DIR` equal to
> +       `$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
> +       `config.worktree` file will override settings from any other
> +       config files.

> [...]
> Create a helper method, init_worktree_config(), that will be used in a
> later change to fix this behavior within 'git sparse-checkout set'. The
> method is carefully documented in worktree.h.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -826,3 +827,72 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
> +int init_worktree_config(struct repository *r)
> +{
> +       int res = 0;
> +       int bare = 0;
> +       struct config_set cs = { { 0 } };
> +       const char *core_worktree;
> +       char *common_config_file = xstrfmt("%s/config", r->commondir);
> +       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> +
> +       /*
> +        * If the extension is already enabled, then we can skip the
> +        * upgrade process.
> +        */
> +       if (repository_format_worktree_config)
> +               return 0;
> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
> +               return error(_("failed to set extensions.worktreeConfig setting"));

> +
> +       git_configset_init(&cs);
> +       git_configset_add_file(&cs, common_config_file);
> +
> +       /*
> +        * 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;
> +       }
> +
> +       /*
> +        * Ensure that we use worktree config for the remaining lifetime
> +        * of the current process.
> +        */
> +       repository_format_worktree_config = 1;
> +
> +cleanup:
> +       git_configset_clear(&cs);
> +       free(common_config_file);
> +       free(main_worktree_file);
> +       return res;
> +}

> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the config.worktree file, if enabled. An
> earlier change added init_worktree_config() as a helper to initialize
> extensions.worktreeConfig if not already enabled.
>
> Let the sparse-checkout builtin use these helpers instead of attempting to
> initialize the worktree config on its own. This changes behavior of 'git
> sparse-checkout set' in a few important ways:
>
>  1. Git will no longer upgrade the repository format, since this is not
>     a requirement for understanding extensions.worktreeConfig.
>
>  2. If the main worktree is bare, then this command will not put the
>     worktree in a broken state.

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

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> When adding a new worktree, it is reasonable to expect that we want to
> use the current set of sparse-checkout settings for that new worktree.
> This is particularly important for repositories where the worktree would
> become too large to be useful. This is even more important when using
> partial clone as well, since we want to avoid downloading the missing
> blobs for files that should not be written to the new worktree.
> 
> The only way to create such a worktree without this intermediate step of
> expanding the full worktree is to copy the sparse-checkout patterns and
> config settings during 'git worktree add'. Each worktree has its own
> sparse-checkout patterns, and the default behavior when the
> sparse-checkout file is missing is to include all paths at HEAD. Thus,
> we need to have patterns from somewhere, they might as well be the
> current worktree's patterns. These are then modified independently in
> the future.
> 
> In addition to the sparse-checkout file, copy the worktree config file
> if worktree config is enabled and the file exists. This will copy over
> any important settings to ensure the new worktree behaves the same as
> the current one. The only exception we must continue to make is that
> core.bare and core.worktree should become unset in the worktree's config
> file.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/worktree.c                 | 60 ++++++++++++++++++++++++++++++
>  t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++----
>  t/t2400-worktree-add.sh            | 46 ++++++++++++++++++++++-
>  3 files changed, 127 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 2838254f7f2..dc9cd6decc8 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
>  	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
>  	write_file(sb.buf, "../..");
>  
> +	/*
> +	 * If the current worktree has sparse-checkout enabled, then copy
> +	 * the sparse-checkout patterns from the current worktree.
> +	 */
> +	if (core_apply_sparse_checkout) {
> +		char *from_file = git_pathdup("info/sparse-checkout");
> +		char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
> +					realpath.buf, name);
> +
> +		if (file_exists(from_file)) {
> +			if (safe_create_leading_directories(to_file) ||
> +			    copy_file(to_file, from_file, 0666))
> +				error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
> +				      from_file, to_file);
> +		}
> +
> +		free(from_file);
> +		free(to_file);
> +	}
> +
> +	/*
> +	 * If we are using worktree config, then copy all current config
> +	 * values from the current worktree into the new one, that way the
> +	 * new worktree behaves the same as this one.
> +	 */
> +	if (repository_format_worktree_config) {
> +		char *from_file = git_pathdup("config.worktree");
> +		char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
> +					realpath.buf, name);
> +
> +		if (file_exists(from_file)) {
> +			struct config_set cs = { { 0 }};
> +			const char *str_value;
> +			int bool_value;
> +
> +			if (safe_create_leading_directories(to_file) ||
> +			    copy_file(to_file, from_file, 0666))
> +				die(_("failed to copy worktree config from '%s' to '%s'"),
> +				    from_file, to_file);
> +
> +			git_configset_init(&cs);
> +			git_configset_add_file(&cs, from_file);
> +
> +			if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
> +			    bool_value &&
> +			    git_config_set_multivar_in_file_gently(
> +					to_file, "core.bare", NULL, "true", 0))
> +				error(_("failed to unset 'core.bare' in '%s'"), to_file);
> +			if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
> +			    git_config_set_in_file_gently(to_file,
> +							  "core.worktree", NULL))
> +				error(_("failed to unset 'core.worktree' in '%s'"), to_file);
> +

> On 2/6/2022 5:36 AM, Jean-Noël AVILA wrote:
>> On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:
> 
> Hi Jean-Noël. Thanks for your attention to the translatable messages
> here:
> 
>>> error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
>>>        from_file, to_file);
> 
>>> die(_("failed to copy worktree config from '%s' to '%s'"),
>>>      from_file, to_file);
> 
>>> error(_("failed to unset 'core.bare' in '%s'"), to_file);
> 
>>> error(_("failed to unset 'core.worktree' in '%s'"), to_file);
> 
>> In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?
> 
> I would argue that "unable to set" is not appropriate for any of these
> messages. Perhaps the "failed to copy" messages might be able to use
> "unable to set", but the information that the config setting is coming
> from settings the user controlled is valuable.
> 
> The "failed to unset" means "we are trying to _remove_ this setting
> from the config file", so "unable to set" does not seem to work here.
> 
> I'm open to revisiting this if you disagree.
> 
> Thanks,
> -Stolee
> 

> Le 07/02/2022 à 15:10, Derrick Stolee a écrit :
>> On 2/6/2022 5:36 AM, Jean-Noël AVILA wrote:
>>> On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:
>>
>> Hi Jean-Noël. Thanks for your attention to the translatable messages
>> here:
>>
>>>> error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
>>>>        from_file, to_file);
>>
>>>> die(_("failed to copy worktree config from '%s' to '%s'"),
>>>>      from_file, to_file);
>>
>>>> error(_("failed to unset 'core.bare' in '%s'"), to_file);
>>
>>>> error(_("failed to unset 'core.worktree' in '%s'"), to_file);
>>
>>> In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?
>>
>> I would argue that "unable to set" is not appropriate for any of these
>> messages. Perhaps the "failed to copy" messages might be able to use
>> "unable to set", but the information that the config setting is coming
>> from settings the user controlled is valuable.
>>
>> The "failed to unset" means "we are trying to _remove_ this setting
>> from the config file", so "unable to set" does not seem to work here.
>>
>> I'm open to revisiting this if you disagree.
>>
>> Thanks,
>> -Stolee
>>
> 
> Hi Derrick,
> 
> Sorry for not being more precise. The first two errors were not the subject of this remark.
> 
> For the last two, this is quite surprising that the same function failing (git_config_set_in_file_gently) can lead to different error messages.
> 
> In any case, I would argue at least for  shifting to :
> 
>     error(_("failed to unset '%s' in '%s'"), 'core.bare", to_file);
> and
>     error(_("failed to unset '%s' in '%s'"), "core.worktree", to_file);
> 
> in order to factorize the message and get the option name out of the way.

> When adding a new worktree, it is reasonable to expect that we want to
> use the current set of sparse-checkout settings for that new worktree.
> This is particularly important for repositories where the worktree would
> become too large to be useful. This is even more important when using
> partial clone as well, since we want to avoid downloading the missing
> blobs for files that should not be written to the new worktree.
>
> The only way to create such a worktree without this intermediate step of
> expanding the full worktree is to copy the sparse-checkout patterns and
> config settings during 'git worktree add'. Each worktree has its own
> sparse-checkout patterns, and the default behavior when the
> sparse-checkout file is missing is to include all paths at HEAD. Thus,
> we need to have patterns from somewhere, they might as well be the
> current worktree's patterns. These are then modified independently in
> the future.
>
> In addition to the sparse-checkout file, copy the worktree config file
> if worktree config is enabled and the file exists. This will copy over
> any important settings to ensure the new worktree behaves the same as
> the current one. The only exception we must continue to make is that
> core.bare and core.worktree should become unset in the worktree's config
> file.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
> +       /*
> +        * If the current worktree has sparse-checkout enabled, then copy
> +        * the sparse-checkout patterns from the current worktree.
> +        */
> +       if (core_apply_sparse_checkout) {
> +               char *from_file = git_pathdup("info/sparse-checkout");
> +               char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
> +                                       realpath.buf, name);

> +               if (file_exists(from_file)) {
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666))
> +                               error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
> +                                     from_file, to_file);
> +               }
> +
> +               free(from_file);
> +               free(to_file);
> +       }
> +
> +       /*
> +        * If we are using worktree config, then copy all current config
> +        * values from the current worktree into the new one, that way the
> +        * new worktree behaves the same as this one.
> +        */

> +                       const char *str_value;
> +                       int bool_value;
> +
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666))
> +                               die(_("failed to copy worktree config from '%s' to '%s'"),
> +                                   from_file, to_file);

> +                       git_configset_init(&cs);
> +                       git_configset_add_file(&cs, from_file);
> +
> +                       if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
> +                           bool_value &&

> +                           git_config_set_in_file_gently(to_file,
> +                                                         "core.worktree", NULL))
> +                               error(_("failed to unset 'core.worktree' in '%s'"), to_file);
> +
> +                       git_configset_clear(&cs);
> +               }
> +
> +               free(from_file);
> +               free(to_file);
> +       }
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' '
> +test_expect_success 'worktree: add copies sparse-checkout patterns' '
> +       cat repo/.git/info/sparse-checkout >old &&
> +       test_when_finished cp old repo/.git/info/sparse-checkout &&
> +       test_when_finished git -C repo worktree remove ../worktree &&
> +       git -C repo sparse-checkout set --no-cone "/*" &&
> +       git -C repo worktree add --quiet ../worktree 2>err &&
> +       test_must_be_empty err &&
> +       new=repo/.git/worktrees/worktree/info/sparse-checkout &&

> +       test_path_is_file $new &&
> +       test_cmp repo/.git/info/sparse-checkout $new &&
> +       git -C worktree sparse-checkout set --cone &&
> +       test_cmp_config -C worktree true core.sparseCheckoutCone &&
> +       test_must_fail git -C repo core.sparseCheckoutCone
> +'
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 37ad79470fb..3fb5b21b943 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -165,8 +165,50 @@ test_expect_success '"add" default branch of a bare repo' '
>         (
>                 git clone --bare . bare2 &&
>                 cd bare2 &&
> -               git worktree add ../there3 main
> -       )
> +               git worktree add ../there3 main &&
> +               cd ../there3 &&
> +               git status
> +       ) &&

> +       cat >expect <<-EOF &&
> +       init.t
> +       EOF
> +       ls there3 >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '"add" to bare repo with worktree config' '
> +       (
> +               git clone --bare . bare3 &&
> +               cd bare3 &&
> +               git config extensions.worktreeconfig true &&
> +               git config --worktree core.bare true &&
> +               git config --worktree core.worktree "$(pwd)" &&

> +               git worktree add ../there4 main &&
> +               cd ../there4 &&
> +               git status &&
> +               git worktree add --detach ../there5 &&
> +               cd ../there5 &&
> +               git status
> +       ) &&

> +       # the worktree has the arbitrary value copied.
> +       test_cmp_config -C there4 value bogus.key &&
> +       test_cmp_config -C there5 value bogus.key &&
> +
> +       # however, core.bare and core.worktree were removed.
> +       test_must_fail git -C there4 config core.bare &&
> +       test_must_fail git -C there4 config core.worktree &&

> +       cat >expect <<-EOF &&
> +       init.t
> +       EOF
> +
> +       ls there4 >actual &&
> +       test_cmp expect actual &&
> +       ls there5 >actual &&
> +       test_cmp expect actual
>  '

> On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > +       new=repo/.git/worktrees/worktree/info/sparse-checkout &&
>
> For robustness, this should be using:
>
>     new=$(git rev-parse --git-path info/sparse-checkout)
>
> to retrieve ".git/worktrees/<id>/info/sparse-checkout" rather than
> hard-coding "worktree" for "<id>".

> On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +       /*
>> +        * If we are using worktree config, then copy all current config
>> +        * values from the current worktree into the new one, that way the
>> +        * new worktree behaves the same as this one.
>> +        */
> 
> As an aid for future readers, it might be helpful to extend this
> content to explain _why_ the new worktree should behave the same as
> the current one. Reproducing the important bits from the commit
> message here in the comment could make it more useful.

>> +                           git_config_set_multivar_in_file_gently(
>> +                                       to_file, "core.bare", NULL, "true", 0))
>> +                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
>> +                       if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
> 
> In patch [2/5] you used git_configset_get_string_tmp() to retrieve
> this setting, but here you're using git_configset_get_value(). Is
> there a reason for the inconsistency?

>> +               git worktree add ../there3 main &&
>> +               cd ../there3 &&
>> +               git status
>> +       ) &&
> 
> Is this some debugging code you forgot to remove or was `git status`
> failing due to the bug(s) fixed by this patch series? I'm guessing the
> latter since you also use `git status` in more tests below. Anyhow,
> it's not very clear what the `git-status` is meant to be testing. An
> in-code comment _might_ help. Even better, perhaps, would be to add a
> new single-purpose test or a well-named function which explicitly
> checks the conditions you want to test (i.e. that git-config doesn't
> report core.bare as true or core.worktree as having a value).

> On 2/6/2022 6:30 AM, Eric Sunshine wrote:
> > On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +               git config --worktree bogus.key value &&
> >> +               git config --unset core.bare &&
> >
> > Why is this being unset? (Genuine question. Am I missing something obvious?)
>
> I'm moving it out of the common config file. Earlier commands
> enabled it in the config.worktree file for this working tree.

> On Mon, Feb 7, 2022 at 9:30 AM Derrick Stolee <stolee@gmail.com> wrote:
>> On 2/6/2022 6:30 AM, Eric Sunshine wrote:
>>> On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
>>>> +               git config --worktree bogus.key value &&
>>>> +               git config --unset core.bare &&
>>>
>>> Why is this being unset? (Genuine question. Am I missing something obvious?)
>>
>> I'm moving it out of the common config file. Earlier commands
>> enabled it in the config.worktree file for this working tree.
> 
> But won't the `git worktree add` commands which immediately follow
> this bit automatically drop `core.bare=true` from the common config
> file? Or am I misthinking on this? Or are you just trying to be
> explicit here with the manual removal?

>
> This series is now based on v2.35.0 since that contains all of the necessary
> topics.
>
> 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 or core.worktree are set in the
> common config file. This series fixes this, but also puts in place some
> helpers to prevent this from happening in the future.
>
> ATTENTION: I have significantly redesigned the series since previous
> versions, so most of this cover letter is new.
>
>  * Patch 1 updates documentation around extensions.worktreeConfig in a few
>    places to improve discoverability. Several cross links are added to make
>    it easy to find the related areas. (The documentation for the changes to
>    'git sparse-checkout' are delayed to patch 4.)
>
>  * Patch 2 introduces the init_worktree_config() helper which follows the
>    documented instructions to enable extensions.worktreeConfig as well as
>    move the core.bare and core.worktree config values. This update does not
>    modify core.repositoryFormatVersion, since this is not needed
>    specifically for extensions.worktreeConfig.
>
>  * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
>    can internally adjust a config value within a worktree, at least if
>    extensions.worktreeConfig is enabled. (It will write to the common config
>    file if the extension is not enabled.)
>
>  * Patch 4 modifies the sparse-checkout builtin to use
>    init_worktree_config() and repo_config_set_worktree_gently() in ways that
>    fix the reported bug. The behavior change here is that it will no longer
>    upgrade the repository format version, since that is not needed for
>    extensions.worktreeConfig.
>
>  * Patch 5 updates 'git worktree add' to copy the worktree config from the
>    current worktree to the new one (while unsetting core.bare=true and
>    core.worktree=*) along with copying the sparse-checkout patterns file.
>
> [1]
> https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
> [2]
> https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/
>
>
> Updates in v5
> =============
>
>  * Cleaned up documentation as per Elijah's suggestions.
>  * Removed unnecessary conflicting change in git-sparse-checkout.txt
>  * Fixed an ambiguous comment about moving config values.

> Updates in v4
> =============
>
>  * Rebased to v2.35.0
>  * Fixed memory leak (was leaking repo_git_path() result)
>  * Added additional documentation updates so curious users can discover the
>    intricacies of extensions.worktreeConfig from multiple entry points.
>  * Significantly reduced the amount of changes to config.c.
>  * 'git sparse-checkout' no longer upgrades the repository format.
>  * Dropped the update to upgrade_repository_format(), since it is not
>    needed.
>  * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
>    helper method called by 'git sparse-checkout'
>  * Many others because of the significant changes required by the above
>    items.
>
> Thanks, -Stolee
>
> Derrick Stolee (5):
>   Documentation: add extensions.worktreeConfig details
>   worktree: create init_worktree_config()
>   config: add repo_config_set_worktree_gently()
>   sparse-checkout: set worktree-config correctly
>   worktree: copy sparse-checkout patterns and config on add
>
>  Documentation/config/extensions.txt   | 31 ++++++++++++
>  Documentation/git-config.txt          |  8 ++-
>  Documentation/git-sparse-checkout.txt | 16 ++++--
>  Documentation/git-worktree.txt        | 11 +++--
>  builtin/sparse-checkout.c             | 28 +++++------
>  builtin/worktree.c                    | 60 +++++++++++++++++++++++
>  config.c                              | 35 ++++++++++++--
>  config.h                              |  8 +++
>  sparse-index.c                        | 10 ++--
>  t/t1091-sparse-checkout-builtin.sh    | 35 ++++++++++----
>  t/t2400-worktree-add.sh               | 46 +++++++++++++++++-
>  worktree.c                            | 70 +++++++++++++++++++++++++++
>  worktree.h                            | 21 ++++++++
>  13 files changed, 333 insertions(+), 46 deletions(-)
>
>
> base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1101
>
> Range-diff vs v4:
>
>  1:  459e09dedd7 ! 1:  1bd5f26271c Documentation: add extensions.worktreeConfig details
>      @@ Commit message
>           within git-sparse-checkout.txt, but a behavior change is needed before
>           making those updates.
>
>      +    Helped-by: Elijah Newren <newren@gmail.com>
>           Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>
>        ## Documentation/config/extensions.txt ##
>      @@ Documentation/git-config.txt: from all available files.
>       - present. If not it's the same as `--local`.
>       + enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
>       + is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
>      -+ form `.git/worktrees/<worktree-name>/` for other worktrees. See
>      ++ form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See
>       + linkgit:git-worktree[1] to learn how to enable
>       + `extensions.worktreeConfig`.
>
>      @@ Documentation/git-worktree.txt: CONFIGURATION FILE
>       -already present in the config file, they will be applied to the main
>       -working trees only.
>       +present in the common config file and `extensions.worktreeConfig` is
>      -+disabled, then they will be applied to the main working trees only.
>      ++disabled, then they will be applied to the main working tree only.
>
>        In order to have configuration specific to working trees, you can turn
>        on the `worktreeConfig` extension, e.g.:
>      @@ Documentation/git-worktree.txt: them to the `config.worktree` of the main workin
>       - - `core.worktree` and `core.bare` should never be shared
>       + - `core.worktree` should never be shared.
>       +
>      -+ - `core.bare` should not be shared unless the value is `core.bare=false`.
>      ++ - `core.bare` should not be shared if the value is `core.bare=true`.
>
>         - `core.sparseCheckout` is recommended per working tree, unless you
>           are sure you always use sparse checkout for all working trees.
>  2:  d262a76b448 ! 2:  2a2c350112e worktree: create init_worktree_config()
>      @@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt,
>       + *
>       + * 1. Add extensions.worktreeConfig=true in the common config file.
>       + *
>      -+ * 2. If the common config file has a core.worktree value or core.bare is
>      -+ *    set to true, then those values are moved to the main worktree's
>      -+ *    config.worktree file.
>      ++ * 2. If the common config file has a core.worktree value, then that value
>      ++ *    is moved to the main worktree's config.worktree file.
>      ++ *
>      ++ * 3. If the common config file has a core.bare enabled, then that value
>      ++ *    is moved to the main worktree's config.worktree file.
>       + *
>       + * If extensions.worktreeConfig is already true, then this method
>       + * terminates early without any of the above steps. The existing config
>  3:  110d5e0546c = 3:  802b28a9510 config: add repo_config_set_worktree_gently()
>  4:  fbfaa17797c ! 4:  08b89d17ccf sparse-checkout: set worktree-config correctly
>      @@ Documentation/git-sparse-checkout.txt: COMMANDS
>       - (extensions.worktreeConfig, core.sparseCheckout,
>       - core.sparseCheckoutCone) if they are not already enabled, and
>       - write a set of patterns to the sparse-checkout file from the
>      -- list of arguments following the 'set' subcommand. Update the
>      -- working directory to match the new patterns.
>       + Enable the necessary sparse-checkout config settings
>      -+ (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
>      -+ they are not already enabled, and write a set of patterns to the
>      -+ sparse-checkout file from the list of arguments following the
>      -+ 'set' subcommand. Update the working directory to match the new
>      -+ patterns.
>      -++
>      ++ (`core.sparseCheckout`, `core.sparseCheckoutCone`, and
>      ++ `index.sparse`) if they are not already set to the desired values,
>      ++ and write a set of patterns to the sparse-checkout file from the
>      +  list of arguments following the 'set' subcommand. Update the
>      +  working directory to match the new patterns.
>      + +
>       +To ensure that adjusting the sparse-checkout settings within a worktree
>       +does not alter the sparse-checkout settings in other worktrees, the 'set'
>       +subcommand will upgrade your repository config to use worktree-specific
>      @@ Documentation/git-sparse-checkout.txt: COMMANDS
>       +the 'set' subcommand are stored in the worktree-specific sparse-checkout
>       +file. See linkgit:git-worktree[1] and the documentation of
>       +`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
>      - +
>      +++
>        When the `--stdin` option is provided, the patterns are read from
>        standard in as a newline-delimited list instead of from the arguments.
>      -@@ Documentation/git-sparse-checkout.txt: interact with your repository until it is disabled.
>      -  By default, these patterns are read from the command-line arguments,
>      -  but they can be read from stdin using the `--stdin` option. When
>      -  `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
>      -- as directory names as in the 'set' subcommand.
>      -+ as directory names as in the 'set' subcommand. The sparsity defined
>      -+ by the arguments to the 'add' subcommand are added to the patterns
>      -+ in the worktree-specific sparse-checkout file.
>      -
>      - 'reapply'::
>      -  Reapply the sparsity pattern rules to paths in the working tree.
>      + +
>
>        ## builtin/sparse-checkout.c ##
>       @@
>  5:  bb9e550ff3d ! 5:  85779dfaed3 worktree: copy sparse-checkout patterns and config on add
>      @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout'
>       + cat repo/.git/info/sparse-checkout >old &&
>       + test_when_finished cp old repo/.git/info/sparse-checkout &&
>       + test_when_finished git -C repo worktree remove ../worktree &&
>      -+ git -C repo sparse-checkout set "/*" &&
>      ++ git -C repo sparse-checkout set --no-cone "/*" &&
>       + git -C repo worktree add --quiet ../worktree 2>err &&
>       + test_must_be_empty err &&
>       + new=repo/.git/worktrees/worktree/info/sparse-checkout &&
>
> --
> gitgitgadget

> diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
> index 4e23d73cdca..bccaec7a963 100644
> --- a/Documentation/config/extensions.txt
> +++ b/Documentation/config/extensions.txt
> @@ -6,3 +6,34 @@ extensions.objectFormat::
>  Note that this setting should only be set by linkgit:git-init[1] or
>  linkgit:git-clone[1].  Trying to change it after initialization will not
>  work and will produce hard-to-diagnose issues.
> +
> +extensions.worktreeConfig::
> +	If enabled, then worktrees will load config settings from the
> +	`$GIT_DIR/config.worktree` file in addition to the
> +	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
> +	`$GIT_DIR` are the same for the main working tree, while other
> +	working trees have `$GIT_DIR` equal to
> +	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
>> index 4e23d73cdca..bccaec7a963 100644
>> --- a/Documentation/config/extensions.txt
>> +++ b/Documentation/config/extensions.txt
>> @@ -6,3 +6,34 @@ extensions.objectFormat::
>>  Note that this setting should only be set by linkgit:git-init[1] or
>>  linkgit:git-clone[1].  Trying to change it after initialization will not
>>  work and will produce hard-to-diagnose issues.
>> +
>> +extensions.worktreeConfig::
>> +	If enabled, then worktrees will load config settings from the
>> +	`$GIT_DIR/config.worktree` file in addition to the
>> +	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
>> +	`$GIT_DIR` are the same for the main working tree, while other
>> +	working trees have `$GIT_DIR` equal to
>> +	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the
> 
> The mixed use of "worktree" and "working tree" in this paragraph
> might confuse readers into thinking that the paragraph is being
> careful to make distinction between the two.  All references to
> "working tree" in the above paragraph should actually be "worktree",
> I would think.

> 	Side note: "working tree" is in the glossary-content.txt,
> 	but "worktree", which is one "working tree" + repository
> 	metadata (i.e. ".git/") that may be partially shared with
> 	other "worktree"s of a single repository, is not defined.
> 
> This is a tangent, but I wonder why we chose to use a different
> filename (i.e. not "config" but "config.worktree") for this.  If we
> were redoing multi-worktree support from scratch, we would not reuse
> the $GIT_DIR used by the primary worktree as $GIT_COMMON_DIR, so
> that all worktrees would share a single $GIT_COMMON_DIR and
> $GIT_COMMON_DIR/config that has stuff that is shared among all the
> worktrees, while per worktree stuff is in $GIT_DIR/config even for
> the primary worktree.  But that is all water under the bridge now.

> Other than the terminology gotcha, looked sensible.  Migrating
> automatically and/or noticing a suspicious setting may be needed to
> help end users, but that would not be within the scope of this step.
> 
> Attached is a "how about this?" glossary update suggestion.  Most of
> the existing mention of "working tree" are fine as-is because they
> only care about what is in the "working tree", but some should be
> changed to "worktree" to stress the fact that they care not just the
> "working tree" part but also the repository metadata part that is
> associated with that single "working tree".  The first hunk says
> worktree but it is clear that it is interested only in the "working
> tree" files.
> 
>  Documentation/glossary-content.txt | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
> index c077971335..d816512c6a 100644
> --- c/Documentation/glossary-content.txt
> +++ w/Documentation/glossary-content.txt
> @@ -312,7 +312,7 @@ Pathspecs are used on the command line of "git ls-files", "git
>  ls-tree", "git add", "git grep", "git diff", "git checkout",
>  and many other commands to
>  limit the scope of operations to some subset of the tree or
> -worktree.  See the documentation of each command for whether
> +working tree.  See the documentation of each command for whether
>  paths are relative to the current directory or toplevel.  The
>  pathspec syntax is as follows:
>  +
> @@ -446,7 +446,7 @@ exclude;;
>  	interface than the <<def_plumbing,plumbing>>.
>  
>  [[def_per_worktree_ref]]per-worktree ref::
> -	Refs that are per-<<def_working_tree,worktree>>, rather than
> +	Refs that are per-<<def_worktree,worktree>>, rather than
>  	global.  This is presently only <<def_HEAD,HEAD>> and any refs
>  	that start with `refs/bisect/`, but might later include other
>  	unusual refs.
> @@ -669,3 +669,12 @@ The most notable example is `HEAD`.
>  	The tree of actual checked out files.  The working tree normally
>  	contains the contents of the <<def_HEAD,HEAD>> commit's tree,
>  	plus any local changes that you have made but not yet committed.
> +
> +[[def_work_tree]]worktree::
> +	A repository can have zero (i.e. bare repository) or one or
> +	more worktrees attached to it. One "worktree" consists of a
> +	"working tree" and repository metadata, most of which are
> +	shared among other worktrees of a single repository, and
> +	some of which are maintained separately per worktree
> +	(e.g. the index, HEAD, per-worktree refs and per-worktree
> +	configuration file)

>> +[[def_work_tree]]worktree::
>> +	A repository can have zero (i.e. bare repository) or one or
>> +	more worktrees attached to it. One "worktree" consists of a
>> +	"working tree" and repository metadata, most of which are
>> +	shared among other worktrees of a single repository, and
>> +	some of which are maintained separately per worktree
>> +	(e.g. the index, HEAD, per-worktree refs and per-worktree
>> +	configuration file)
>
> I like this addition, except that I don't understand the "per-worktree
> refs" (other than HEAD). Are there other thins used by features such
> as merge and rebase that would appear as worktree-specific? Of course,
> some state for these operations is stored per-worktree, I just didn't
> know if any were actually "refs".

> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> +[[def_work_tree]]worktree::
>>> +	A repository can have zero (i.e. bare repository) or one or
>>> +	more worktrees attached to it. One "worktree" consists of a
>>> +	"working tree" and repository metadata, most of which are
>>> +	shared among other worktrees of a single repository, and
>>> +	some of which are maintained separately per worktree
>>> +	(e.g. the index, HEAD, per-worktree refs and per-worktree
>>> +	configuration file)
>>
>> I like this addition, except that I don't understand the "per-worktree
>> refs" (other than HEAD). Are there other thins used by features such
>> as merge and rebase that would appear as worktree-specific? Of course,
>> some state for these operations is stored per-worktree, I just didn't
>> know if any were actually "refs".
> 
> "per-worktree ref" is an entry in the glossary.
> 
>     [[def_per_worktree_ref]]per-worktree ref::
>             Refs that are per-<<def_working_tree,worktree>>, rather than
>             global.  This is presently only <<def_HEAD,HEAD>> and any refs
>             that start with `refs/bisect/`, but might later include other
>             unusual refs.
> 
> And those other things are also listed as "pseudoref".
> 
>     [[def_pseudoref]]pseudoref::
>             Pseudorefs are a class of files under `$GIT_DIR` which behave
>             like refs for the purposes of rev-parse, but which are treated
>             specially by git...
> 
> I think the motivation of special casing refs/bisect/ is to allow
> use of a separate worktree for bisecting without affecting other
> development or another bisection.  The HEAD is singled out in the
> description, but MERGE_HEAD and others (pseudoref) that are declared
> here to be files under '$GIT_DIR', when we migrate fully to other
> backend that may not want to have files under '$GIT_DIR' to
> represent them, ought to become per-worktree, for the same reason as
> HEAD should be per-worktree, i.e. it allows worktrees to be
> independent from each other and have their checkout at different
> commits, growing history of different branches in parallel.

>
> Derrick Stolee <stolee@gmail.com> writes:
>
> >> +[[def_work_tree]]worktree::
> >> +    A repository can have zero (i.e. bare repository) or one or
> >> +    more worktrees attached to it. One "worktree" consists of a
> >> +    "working tree" and repository metadata, most of which are
> >> +    shared among other worktrees of a single repository, and
> >> +    some of which are maintained separately per worktree
> >> +    (e.g. the index, HEAD, per-worktree refs and per-worktree
> >> +    configuration file)
> >
> > I like this addition, except that I don't understand the "per-worktree
> > refs" (other than HEAD). Are there other thins used by features such
> > as merge and rebase that would appear as worktree-specific? Of course,
> > some state for these operations is stored per-worktree, I just didn't
> > know if any were actually "refs".
>
> "per-worktree ref" is an entry in the glossary.
>
>     [[def_per_worktree_ref]]per-worktree ref::
>             Refs that are per-<<def_working_tree,worktree>>, rather than
>             global.  This is presently only <<def_HEAD,HEAD>> and any refs
>             that start with `refs/bisect/`, but might later include other
>             unusual refs.
>
> And those other things are also listed as "pseudoref".
>
>     [[def_pseudoref]]pseudoref::
>             Pseudorefs are a class of files under `$GIT_DIR` which behave
>             like refs for the purposes of rev-parse, but which are treated
>             specially by git...
>
> I think the motivation of special casing refs/bisect/ is to allow
> use of a separate worktree for bisecting without affecting other
> development or another bisection.  The HEAD is singled out in the
> description, but MERGE_HEAD and others (pseudoref) that are declared
> here to be files under '$GIT_DIR', when we migrate fully to other
> backend that may not want to have files under '$GIT_DIR' to
> represent them, ought to become per-worktree, for the same reason as
> HEAD should be per-worktree, i.e. it allows worktrees to be
> independent from each other and have their checkout at different
> commits, growing history of different branches in parallel.

> But testing just now, I think the pseudorefs are already per-worktree.
> I just did a merge in a secondary worktree, and then observed from the
> primary worktree that a .git/worktrees/<id>/MERGE_HEAD was created,
> not a .git/MERGE_HEAD.  (Maybe the glossary could just spell out that
> these are under $GIT_DIR and _not_ $GIT_COMMON_DIR to avoid potential
> confusion?)

> On 2/8/2022 5:20 PM, Junio C Hamano wrote:
> > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> +extensions.worktreeConfig::
> >> +    If enabled, then worktrees will load config settings from the
> >> +    `$GIT_DIR/config.worktree` file in addition to the
> >> +    `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
> >> +    `$GIT_DIR` are the same for the main working tree, while other
> >> +    working trees have `$GIT_DIR` equal to
> >> +    `$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the
> >
> > The mixed use of "worktree" and "working tree" in this paragraph
> > might confuse readers into thinking that the paragraph is being
> > careful to make distinction between the two.  All references to
> > "working tree" in the above paragraph should actually be "worktree",
> > I would think.
>
> I generally agree. This was changed in the most-recent re-roll
> based on a request by Eric [1]. I'm happy to take whichever
> version the two of you settle on.
>
> [1] https://lore.kernel.org/git/CAPig+cS-3CxxyPGcy_vkeN_WYTRo1b-ZhJNdPy8ARZSNKkF1Xg@mail.gmail.com/

>> I generally agree. This was changed in the most-recent re-roll
>> based on a request by Eric [1]. I'm happy to take whichever
>> version the two of you settle on.
>>
>> [1] https://lore.kernel.org/git/CAPig+cS-3CxxyPGcy_vkeN_WYTRo1b-ZhJNdPy8ARZSNKkF1Xg@mail.gmail.com/
>
> "request" is perhaps too strong a word considering that I led in with:
>
>     A few minor comments, which can be addressed later or not
>     at all, and likely are not worth holding up the series...
>
> I mentioned "worktree vs. working tree" only to point out the
> terminology inconsistency being introduced by the new patch; the same
> sort of inconsistency which had bothered Michael Haggerty enough to do
> something about it in bc483285b7 (Documentation/git-worktree:
> consistently use term "linked working tree", 2015-07-20).

> I, personally, prefer the term "worktree" for both convenience and
> because it better encapsulates the overall "thing" which is
> manipulated by the git-worktree command unlike the term "working tree"
> which, as Junio points out, has (perhaps) a more narrow meaning. As
> such, I would not be opposed to a patch series which changes "working
> tree" to "worktree" in documentation where appropriate, but that's
> outside the scope of this series.

>
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
> > index 4e23d73cdca..bccaec7a963 100644
> > --- a/Documentation/config/extensions.txt
> > +++ b/Documentation/config/extensions.txt
> > @@ -6,3 +6,34 @@ extensions.objectFormat::
> >  Note that this setting should only be set by linkgit:git-init[1] or
> >  linkgit:git-clone[1].  Trying to change it after initialization will not
> >  work and will produce hard-to-diagnose issues.
> > +
> > +extensions.worktreeConfig::
> > +     If enabled, then worktrees will load config settings from the
> > +     `$GIT_DIR/config.worktree` file in addition to the
> > +     `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
> > +     `$GIT_DIR` are the same for the main working tree, while other
> > +     working trees have `$GIT_DIR` equal to
> > +     `$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the
>
> The mixed use of "worktree" and "working tree" in this paragraph
> might confuse readers into thinking that the paragraph is being
> careful to make distinction between the two.  All references to
> "working tree" in the above paragraph should actually be "worktree",
> I would think.
>
>         Side note: "working tree" is in the glossary-content.txt,
>         but "worktree", which is one "working tree" + repository
>         metadata (i.e. ".git/") that may be partially shared with
>         other "worktree"s of a single repository, is not defined.
>
> This is a tangent, but I wonder why we chose to use a different
> filename (i.e. not "config" but "config.worktree") for this.  If we
> were redoing multi-worktree support from scratch, we would not reuse
> the $GIT_DIR used by the primary worktree as $GIT_COMMON_DIR, so
> that all worktrees would share a single $GIT_COMMON_DIR and
> $GIT_COMMON_DIR/config that has stuff that is shared among all the
> worktrees, while per worktree stuff is in $GIT_DIR/config even for
> the primary worktree.  But that is all water under the bridge now.
>
> Other than the terminology gotcha, looked sensible.  Migrating
> automatically and/or noticing a suspicious setting may be needed to
> help end users, but that would not be within the scope of this step.
>
> Attached is a "how about this?" glossary update suggestion.  Most of
> the existing mention of "working tree" are fine as-is because they
> only care about what is in the "working tree", but some should be
> changed to "worktree" to stress the fact that they care not just the
> "working tree" part but also the repository metadata part that is
> associated with that single "working tree".  The first hunk says
> worktree but it is clear that it is interested only in the "working
> tree" files.
>
>  Documentation/glossary-content.txt | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
> index c077971335..d816512c6a 100644
> --- c/Documentation/glossary-content.txt
> +++ w/Documentation/glossary-content.txt
> @@ -312,7 +312,7 @@ Pathspecs are used on the command line of "git ls-files", "git
>  ls-tree", "git add", "git grep", "git diff", "git checkout",
>  and many other commands to
>  limit the scope of operations to some subset of the tree or
> -worktree.  See the documentation of each command for whether
> +working tree.  See the documentation of each command for whether
>  paths are relative to the current directory or toplevel.  The
>  pathspec syntax is as follows:
>  +
> @@ -446,7 +446,7 @@ exclude;;
>         interface than the <<def_plumbing,plumbing>>.
>
>  [[def_per_worktree_ref]]per-worktree ref::
> -       Refs that are per-<<def_working_tree,worktree>>, rather than
> +       Refs that are per-<<def_worktree,worktree>>, rather than
>         global.  This is presently only <<def_HEAD,HEAD>> and any refs
>         that start with `refs/bisect/`, but might later include other
>         unusual refs.
> @@ -669,3 +669,12 @@ The most notable example is `HEAD`.
>         The tree of actual checked out files.  The working tree normally
>         contains the contents of the <<def_HEAD,HEAD>> commit's tree,
>         plus any local changes that you have made but not yet committed.
> +
> +[[def_work_tree]]worktree::
> +       A repository can have zero (i.e. bare repository) or one or
> +       more worktrees attached to it. One "worktree" consists of a
> +       "working tree" and repository metadata, most of which are
> +       shared among other worktrees of a single repository, and
> +       some of which are maintained separately per worktree
> +       (e.g. the index, HEAD, per-worktree refs and per-worktree
> +       configuration file)

>>  [[def_per_worktree_ref]]per-worktree ref::
>> -       Refs that are per-<<def_working_tree,worktree>>, rather than
>> +       Refs that are per-<<def_worktree,worktree>>, rather than
>>         global.  This is presently only <<def_HEAD,HEAD>> and any refs
>>         that start with `refs/bisect/`, but might later include other
>>         unusual refs.
>> @@ -669,3 +669,12 @@ The most notable example is `HEAD`.
>>         The tree of actual checked out files.  The working tree normally
>>         contains the contents of the <<def_HEAD,HEAD>> commit's tree,
>>         plus any local changes that you have made but not yet committed.
>> +
>> +[[def_work_tree]]worktree::
>> +       A repository can have zero (i.e. bare repository) or one or
>> +       more worktrees attached to it. One "worktree" consists of a
>> +       "working tree" and repository metadata, most of which are
>> +       shared among other worktrees of a single repository, and
>> +       some of which are maintained separately per worktree
>> +       (e.g. the index, HEAD, per-worktree refs and per-worktree
>> +       configuration file)
>
> We could also add pseudorefs to the list of things maintained
> separately in the final parenthetical comment, but otherwise looks
> good.

> +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 init_worktree_config(struct repository *r)
> +{
> +	int res = 0;
> +	int bare = 0;
> +	struct config_set cs = { { 0 } };
> +	const char *core_worktree;
> +	char *common_config_file;
> +	char *main_worktree_file;
> +
> +	/*
> +	 * If the extension is already enabled, then we can skip the
> +	 * upgrade process.
> +	 */
> +	if (repository_format_worktree_config)
> +		return 0;

> +	common_config_file = xstrfmt("%s/config", r->commondir);
> +	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> +
> +	git_configset_init(&cs);
> +	git_configset_add_file(&cs, common_config_file);
> +
> +	/*
> +	 * If core.bare is true in the common config file, then we need to
> +	 * move it to the main 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 (!git_configset_get_value(&cs, "core.worktree", &core_worktree)) {
> +		if ((res = move_config_setting("core.worktree", core_worktree,
> +					       common_config_file,
> +					       main_worktree_file)))
> +			goto cleanup;
> +	}
> +
> +	/*
> +	 * Ensure that we use worktree config for the remaining lifetime
> +	 * of the current process.
> +	 */
> +	repository_format_worktree_config = 1;
> +
> +cleanup:
> +	git_configset_clear(&cs);
> +	free(common_config_file);
> +	free(main_worktree_file);
> +	return res;
> +}
> diff --git a/worktree.h b/worktree.h
> index 9e06fcbdf3d..e9e839926b0 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -183,4 +183,25 @@ void strbuf_worktree_ref(const struct worktree *wt,
>  			 struct strbuf *sb,
>  			 const char *refname);
>  
> +/**
> + * Enable worktree config for the first time. This will make the following
> + * adjustments:
> + *
> + * 1. Add extensions.worktreeConfig=true in the common config file.
> + *
> + * 2. If the common config file has a core.worktree value, then that value
> + *    is moved to the main worktree's config.worktree file.
> + *
> + * 3. If the common config file has a core.bare enabled, then that value
> + *    is moved to the main worktree's config.worktree file.
> + *
> + * If extensions.worktreeConfig is already true, then this method
> + * terminates early without any of the above steps. The existing config
> + * arrangement is assumed to be intentional.
> + *
> + * Returns 0 on success. Reports an error message and returns non-zero
> + * if any of these steps fail.
> + */
> +int init_worktree_config(struct repository *r);
> +
>  #endif

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +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;
>> +}
> 
> Interesting.
> 
> The verb "move" in its name made me expect a "get (and remove)
> whatever value(s) defined out of the old file, and set them
> identically in the new file" sequence, but that is not what is done
> here.  "set to this new single value in the new file and unset from
> the old one".

> I can see the need to say "move it only when its value is X",
> so having the caller to extract the value before deciding to call
> the function (hence not "moving from old") does make sense, but then
> the function is misnamed---it is not "moving", it is doing something
> else.

>> +	common_config_file = xstrfmt("%s/config", r->commondir);
>> +	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
>> +
>> +	git_configset_init(&cs);
>> +	git_configset_add_file(&cs, common_config_file);
>> +
>> +	/*
>> +	 * If core.bare is true in the common config file, then we need to
>> +	 * move it to the main 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.
>> +	 */
> 
> Is the assumption that the secondary worktrees are never bare, but
> the primary one could be (iow, adding worktrees to a bare repository
> would leave the original bare repository as the primary "worktree"
> that does not have "working tree")?  I am trying to see what downsides
> it tries to avoid by not moving the core.bare==false setting.  Shouldn't
> core.bare be set to false when "worktree add" creates a new one anyway,
> if the secondaries are never bare?

>> +	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 main worktree is located
>> +	 * somewhere different than the parent of the common Git dir.
> 
> OK.  We do not want to share the working tree for the primary worktree
> among secondary worktrees.  For the primary, common and uncommon are
> the same, so it may not matter, but mention of "common Git dir" here
> may confuse readers?  Unless overridden by the config, the parent of
> the git dir is the root of the working tree, no?

>> +	 * Relocate that value to avoid breaking all worktrees with this
>> +	 * upgrade to worktree config.
>> +	 */
> 
> And if it is not set, then working tree of each worktree is the
> parent of the per-worktree Git dir, so they will automatically
> become separate, which makes sense.

> On 2/8/2022 5:09 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> +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;
>>> +}
>> 
>> Interesting.
>> 
>> The verb "move" in its name made me expect a "get (and remove)
>> whatever value(s) defined out of the old file, and set them
>> identically in the new file" sequence, but that is not what is done
>> here.  "set to this new single value in the new file and unset from
>> the old one".
>
> I think this "copy into the worktree-specific config, then remove
> from the common file" is an important sequence of events in case a
> concurrent process comes in and reads the two config files in the
> intermediate state and does not see the config value anywhere.
>
> But perhaps that's not actually what you are concerned about,
> because you're saying that the 'value' being provided does not
> actually guarantee that we are moving the setting.

>> I can see the need to say "move it only when its value is X",
>> so having the caller to extract the value before deciding to call
>> the function (hence not "moving from old") does make sense, but then
>> the function is misnamed---it is not "moving", it is doing something
>> else.

>> Is the assumption that the secondary worktrees are never bare, but
>> the primary one could be (iow, adding worktrees to a bare repository
>> would leave the original bare repository as the primary "worktree"
>> that does not have "working tree")?  I am trying to see what downsides
>> it tries to avoid by not moving the core.bare==false setting.  Shouldn't
>> core.bare be set to false when "worktree add" creates a new one anyway,
>> if the secondaries are never bare?
>
> Secondary worktrees cannot be bare. If Git interprets the worktree config
> to have core.bare=true in a secondary worktree, it errors out.
>
> You seem to be suggesting that we should explicitly write core.bare=false
> into each of the worktree-specific config files. Is that right? This move
> is effectively the same, since 'false' is the default.

> Here, the verbal gymnastics are somewhat necessary because secondary
> worktrees have a .git _file_, not a git directory, so using "common
> Git dir" is a way to explicitly reference the Git dir. And the
> strangeness here is exactly that core.worktree can change this working
> tree to be something other than the parent of the (common) Git dir.

>
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +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;
> > +}
>
> Interesting.
>
> The verb "move" in its name made me expect a "get (and remove)
> whatever value(s) defined out of the old file, and set them
> identically in the new file" sequence, but that is not what is done
> here.  "set to this new single value in the new file and unset from
> the old one".
>
> I can see the need to say "move it only when its value is X",
> so having the caller to extract the value before deciding to call
> the function (hence not "moving from old") does make sense, but then
> the function is misnamed---it is not "moving", it is doing something
> else.
>
> > +int init_worktree_config(struct repository *r)
> > +{
> > +     int res = 0;
> > +     int bare = 0;
> > +     struct config_set cs = { { 0 } };
> > +     const char *core_worktree;
> > +     char *common_config_file;
> > +     char *main_worktree_file;
> > +
> > +     /*
> > +      * If the extension is already enabled, then we can skip the
> > +      * upgrade process.
> > +      */
> > +     if (repository_format_worktree_config)
> > +             return 0;
>
> OK.
>
> > +     if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
> > +             return error(_("failed to set extensions.worktreeConfig setting"));
>
> OK.
>
> > +     common_config_file = xstrfmt("%s/config", r->commondir);
> > +     main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> > +
> > +     git_configset_init(&cs);
> > +     git_configset_add_file(&cs, common_config_file);
> > +
> > +     /*
> > +      * If core.bare is true in the common config file, then we need to
> > +      * move it to the main 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.
> > +      */
>
> Is the assumption that the secondary worktrees are never bare, but
> the primary one could be (iow, adding worktrees to a bare repository
> would leave the original bare repository as the primary "worktree"
> that does not have "working tree")?

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Some config settings, such as those for sparse-checkout, are likely
> intended to only apply to one worktree at a time. To make this write
> easier, add a new config API method, repo_config_set_worktree_gently().
>
> This method will attempt to write to the worktree-specific config, but
> will instead write to the common config file if worktree config is not
> enabled.  The next change will introduce a consumer of this method.

> +int repo_config_set_worktree_gently(struct repository *r,
> +				    const char *key, const char *value)
> +{
> +	/* Only use worktree-specific config if it is is already enabled. */
> +	if (repository_format_worktree_config) {
> +		char *file = repo_git_path(r, "config.worktree");
> +		int ret = git_config_set_multivar_in_file_gently(
> +					file, key, value, NULL, 0);
> +		free(file);
> +		return ret;
> +	}
> +	return repo_config_set_multivar_gently(r, key, value, NULL, 0);
> +}

> @@ -3181,14 +3196,28 @@ 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,
> -						      flags);
> +	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)
> +{
> +	char *file = repo_git_path(r, "config");
> +	int res = git_config_set_multivar_in_file_gently(file,
> +							 key, value,
> +							 value_pattern,
> +							 flags);
> +	free(file);
> +	return res;
>  }

>  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,
> +	git_config_set_multivar_in_file(git_path("config"),
> +					key, value, value_pattern,
>  					flags);
>  }

> diff --git a/config.h b/config.h
> index f119de01309..1d98ad269bd 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 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.
>   */
> @@ -281,6 +288,7 @@ 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);
>  int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
>  
>  /**

>> @@ -3181,14 +3196,28 @@ 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,
>> -						      flags);
>> +	return repo_config_set_multivar_gently(the_repository, key, value,
>> +					       value_pattern, flags);
>> +}
> 
> Is this an unrelated "morally no-op" change?

>>  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,
>> +	git_config_set_multivar_in_file(git_path("config"),
>> +					key, value, value_pattern,
>>  					flags);
>>  }
> 
> Is this an unrelated "morally no-op" change?
> 
> It might have value to make caller more explicit by reducing the use
> of "I give NULL, you use 'config' for me", but that doesn't sound
> related to the addition of set_per_worktree_config_gently() helper.

> On 2/8/2022 5:18 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> ...
>>> @@ -3181,14 +3196,28 @@ 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,
>>> -						      flags);
>>> +	return repo_config_set_multivar_gently(the_repository, key, value,
>>> +					       value_pattern, flags);
>>> +}
>> 
>> Is this an unrelated "morally no-op" change?
>
> This one is to match the pattern of how "git_*" methods should
> depend on their "repo_*" counterparts (with "the_repository" inserted
> properly). So, it's part of the standard process for creating these
> "repo_*" variants.

> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 2/8/2022 5:18 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> ...
>>>> @@ -3181,14 +3196,28 @@ 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,
>>>> -						      flags);
>>>> +	return repo_config_set_multivar_gently(the_repository, key, value,
>>>> +					       value_pattern, flags);
>>>> +}
>>>
>>> Is this an unrelated "morally no-op" change?
>>
>> This one is to match the pattern of how "git_*" methods should
>> depend on their "repo_*" counterparts (with "the_repository" inserted
>> properly). So, it's part of the standard process for creating these
>> "repo_*" variants.
> 
> If only one of repo_config_set_multivar_gently() and
> git_config_set_multivar_gently() existed and we were completing the
> pair, then I would understand the explanation, but the title says
> that it is adding repo_config_set_worktree_gently(), which is not,
> and that is where the "unrelated" comes from.
> 
> It needs to be a separate preparatory step to add
> repo_config_set_multivar_gently() before we add
> repo_config_set_worktree_gently(), perhaps?

> A bit higher level question is if the public part of "config-set"
> API functions should gain an "easy" (in the sense of curl_easy_* set
> of API functions) API to allow the callers to say "I do not care to
> find out if per-worktree configuration is in use, or this particular
> variable is meant to be per-worktree, just set it to this value".
> 
> On this question, I am of two minds.  As certain variables (like
> core.sparseCheckout) should always be per-worktree just like certain
> refs (like HEAD) should always be per-worktree, I can understand the
> viewpoint that the callers _ought_ to know and explicitly say that
> they want to get/set in the per-worktree configuration file, but at
> the same time, I would think the callers should not have to care.
> So, I dunno.

> When adding a new worktree, it is reasonable to expect that we want to
> use the current set of sparse-checkout settings for that new worktree.
> This is particularly important for repositories where the worktree would
> become too large to be useful. This is even more important when using
> partial clone as well, since we want to avoid downloading the missing
> blobs for files that should not be written to the new worktree.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -335,6 +335,69 @@ static int add_worktree(const char *path, const char *refname,
> +       /*
> +        * If the current worktree has sparse-checkout enabled, then copy
> +        * the sparse-checkout patterns from the current worktree.
> +        */
> +       if (core_apply_sparse_checkout) {
> +               char *from_file = git_pathdup("info/sparse-checkout");
> +               char *to_file = xstrfmt("%s/info/sparse-checkout",
> +                                       sb_repo.buf);
> +
> +               if (file_exists(from_file)) {
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666))
> +                               error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
> +                                     from_file, to_file);
> +               }
> +
> +               free(from_file);
> +               free(to_file);
> +       }
> +
> +       /*
> +        * If we are using worktree config, then copy all current config
> +        * values from the current worktree into the new one, that way the
> +        * new worktree behaves the same as this one.
> +        */
> +       if (repository_format_worktree_config) {
> +               char *from_file = git_pathdup("config.worktree");
> +               char *to_file = xstrfmt("%s/config.worktree",
> +                                       sb_repo.buf);
> +
> +               if (file_exists(from_file)) {
> +                       struct config_set cs = { { 0 } };
> +                       const char *core_worktree;
> +                       int bare;
> +
> +                       if (safe_create_leading_directories(to_file) ||
> +                           copy_file(to_file, from_file, 0666)) {
> +                               error(_("failed to copy worktree config from '%s' to '%s'"),
> +                                     from_file, to_file);
> +                               goto worktree_copy_cleanup;
> +                       }
> +
> +                       git_configset_init(&cs);
> +                       git_configset_add_file(&cs, from_file);
> +
> +                       if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
> +                           bare &&
> +                           git_config_set_multivar_in_file_gently(
> +                                       to_file, "core.bare", NULL, "true", 0))
> +                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
> +                       if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
> +                           git_config_set_in_file_gently(to_file,
> +                                                         "core.worktree", NULL))
> +                               error(_("failed to unset 'core.worktree' in '%s'"), to_file);
> +
> +                       git_configset_clear(&cs);
> +               }
> +
> +worktree_copy_cleanup:
> +               free(from_file);
> +               free(to_file);
> +       }

>
> This series is now based on v2.35.0 since that contains all of the necessary
> topics.
>
> 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 or core.worktree are set in the
> common config file. This series fixes this, but also puts in place some
> helpers to prevent this from happening in the future.
>
> ATTENTION: I have significantly redesigned the series since previous
> versions, so most of this cover letter is new.
>
>  * Patch 1 updates documentation around extensions.worktreeConfig in a few
>    places to improve discoverability. Several cross links are added to make
>    it easy to find the related areas. (The documentation for the changes to
>    'git sparse-checkout' are delayed to patch 4.)
>
>  * Patch 2 introduces the init_worktree_config() helper which follows the
>    documented instructions to enable extensions.worktreeConfig as well as
>    move the core.bare and core.worktree config values. This update does not
>    modify core.repositoryFormatVersion, since this is not needed
>    specifically for extensions.worktreeConfig.
>
>  * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
>    can internally adjust a config value within a worktree, at least if
>    extensions.worktreeConfig is enabled. (It will write to the common config
>    file if the extension is not enabled.)
>
>  * Patch 4 modifies the sparse-checkout builtin to use
>    init_worktree_config() and repo_config_set_worktree_gently() in ways that
>    fix the reported bug. The behavior change here is that it will no longer
>    upgrade the repository format version, since that is not needed for
>    extensions.worktreeConfig.
>
>  * Patch 5 updates 'git worktree add' to copy the worktree config from the
>    current worktree to the new one (while unsetting core.bare=true and
>    core.worktree=*) along with copying the sparse-checkout patterns file.
>
> [1]
> https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
> [2]
> https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/
>
>
> Updates in v6
> =============
>
>  * Updated documentation to use "working tree" over "worktree" and "" over
>    ""

> On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Updates in v6
> >  * Updated documentation to use "working tree" over "worktree" and "" over
> >    ""
>
> Not sure what "" over "" means.

> >  * Delay some allocations to avoid leaking memory in error conditions.
> >  * Use "main worktree" over "base worktree" in comments.
> >  *
>
> Was the empty bullet point meant to cover the new patch 6?

>
> On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote:
> > On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > Updates in v6
> > >  * Updated documentation to use "working tree" over "worktree" and "" over
> > >    ""
> >
> > Not sure what "" over "" means.
>
> Probably related to my review comment[1] about spelling it
> "$GIT_DIR/worktrees/<id>/" rather than
> "$GIT_DIR/worktrees/<worktree-name>/".
>
> > >  * Delay some allocations to avoid leaking memory in error conditions.
> > >  * Use "main worktree" over "base worktree" in comments.
> > >  *
> >
> > Was the empty bullet point meant to cover the new patch 6?
>
> The "Updates in v6" section was botched a bit. If you look closely,
> the remaining bullet points actually ended up in the "Updates in v5"
> section. The complete "Updates in v6" section should have been
> (approximately):
>
>  * Updated documentation to use "working tree" over "worktree" and
>    "<id>" over "<worktree-name>"
>  * Delay some allocations to avoid leaking memory in error conditions.
>  * Use "main worktree" over "base worktree" in comments.
>  * Removed use of git_configset_get_string_tmp() and added a patch that
>    removes its public declaration.
>  * Fragile variables are replaced with better ones.
>  * Variable names and code style improved.
>  * Several test cleanups in patch 5.
>
> [1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24

> On Mon, Feb 7, 2022 at 9:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote:
> > > Was the empty bullet point meant to cover the new patch 6?
> >
> > The "Updates in v6" section was botched a bit. If you look closely,
> > the remaining bullet points actually ended up in the "Updates in v5"
> > section. The complete "Updates in v6" section should have been
> > (approximately):
> >
> >  * Updated documentation to use "working tree" over "worktree" and
> >    "<id>" over "<worktree-name>"
> >  * Delay some allocations to avoid leaking memory in error conditions.
> >  * Use "main worktree" over "base worktree" in comments.
> >  * Removed use of git_configset_get_string_tmp() and added a patch that
> >    removes its public declaration.
> >  * Fragile variables are replaced with better ones.
> >  * Variable names and code style improved.
> >  * Several test cleanups in patch 5.
>
> So, you clearly also read the patches in this round.  Do they also
> look good to you?   :-)

> On Tue, Feb 8, 2022 at 12:18 AM Elijah Newren <newren@gmail.com> wrote:
> > So, you clearly also read the patches in this round.  Do they also
> > look good to you?   :-)
>
> I have not yet looked at either the patches or the range-diff, and I
> only scanned my eye quickly over the cover letter, but the empty
> bullet point made me stop and look a bit more carefully at that part
> (and only that part) of the cover letter. Not sure yet when I'll have
> time to carefully read this round.

> On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote:
>> On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> Updates in v6
>>>  * Updated documentation to use "working tree" over "worktree" and "" over
>>>    ""
>>
>> Not sure what "" over "" means.
> 
> Probably related to my review comment[1] about spelling it
> "$GIT_DIR/worktrees/<id>/" rather than
> "$GIT_DIR/worktrees/<worktree-name>/".
> 
>>>  * Delay some allocations to avoid leaking memory in error conditions.
>>>  * Use "main worktree" over "base worktree" in comments.
>>>  *
>>
>> Was the empty bullet point meant to cover the new patch 6?
> 
> The "Updates in v6" section was botched a bit. If you look closely,
> the remaining bullet points actually ended up in the "Updates in v5"
> section. The complete "Updates in v6" section should have been
> (approximately):

>  * Delay some allocations to avoid leaking memory in error conditions.
>  * Use "main worktree" over "base worktree" in comments.
>  * Removed use of git_configset_get_string_tmp() and added a patch that
>    removes its public declaration.
>  * Fragile variables are replaced with better ones.
>  * Variable names and code style improved.
>  * Several test cleanups in patch 5.
> 
> [1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24