[3/4] config: add repo_config_set_worktree_gently()

Commit Message

Derrick Stolee Dec. 20, 2021, 3:57 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
---
 config.c | 27 +++++++++++++++++++++++++++
 config.h |  6 ++++++
 2 files changed, 33 insertions(+)

Comments

Derrick Stolee Dec. 20, 2021, 5:32 p.m. UTC | #1

On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>

...

> +	/*
> +	 * Ensure that core.bare reflects the current worktree, since the
> +	 * logic for is_bare_repository() changes if extensions.worktreeConfig
> +	 * is disabled.
> +	 */
> +	if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
> +							  r->worktree ? "false" : "true",
> +							  NULL, 0))) {
> +		error(_("unable to set core.bare setting in worktree config"));
> +		return res;
> +	}

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

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

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

What do others think?

Thanks,
-Stolee

Eric Sunshine Dec. 21, 2021, 12:01 a.m. UTC | #2

On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee <[e-mail]> 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

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

Eric Sunshine Dec. 21, 2021, 5:53 a.m. UTC | #3

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
<[e-mail]> 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/[e-mail]/
[2]: https://lore.kernel.org/git/[e-mail]/
[3]: https://lore.kernel.org/git/[e-mail]/
[4]: https://lore.kernel.org/git/[e-mail]/

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
> 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.)

Eric Sunshine Dec. 21, 2021, 5:59 a.m. UTC | #4

On Mon, Dec 20, 2021 at 7:01 PM Eric Sunshine <[e-mail]> wrote:
> Regarding the actual "fix": we want a new utility function which
> enables per-worktree configuration and handles all the required
> bookkeeping actions described in git-worktree.txt. Specifically, if
> per-worktree configuration is not already enabled, the function will
> need to:
>
> (1) set `extensions.worktreeConfig=true` in .git/config
>
> (1) relocate `core.bare` from .git/config to .git/config.worktree if
> that key exists
>
> (2) relocate `core.worktree` from .git/config to .git/config.worktree
> if that key exists

A couple additional notes:

First, I can't count to three.

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

Derrick Stolee Dec. 21, 2021, 1:41 p.m. UTC | #5

On 12/20/2021 7:01 PM, Eric Sunshine wrote:
> On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee <[e-mail]> 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

You are describing (in better detail) what I meant in my message about
what needs to change in this patch.
 
> That's it. It doesn't need to create or touch any
> .git/worktrees/<id>/config.worktree file(s); it should _not_ add a
> `core.bare=false` to .git/worktrees/<id>/config.worktree, as this v1
> patch series does.

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

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

Thanks,
-Stolee

Derrick Stolee Dec. 21, 2021, 1:45 p.m. UTC | #6

On 12/21/2021 12:53 AM, Eric Sunshine wrote:
> On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> <[e-mail]> 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/[e-mail]/
> [2]: https://lore.kernel.org/git/[e-mail]/
> [3]: https://lore.kernel.org/git/[e-mail]/
> [4]: https://lore.kernel.org/git/[e-mail]/
> 
>> Signed-off-by: Derrick Stolee <[e-mail]>
>> ---
>> diff --git a/config.c b/config.c
>> @@ -2880,6 +2880,33 @@ int git_config_set_gently(const char *key, const char *value)
>> +int repo_config_set_worktree_gently(struct repository *r,
>> +                                   const char *key, const char *value)
>> +{
>> +       int res;
>> +       const char *config_filename = repo_git_path(r, "config.worktree");
>> +
>> +       /*
>> +        * Ensure that core.bare reflects the current worktree, since the
>> +        * logic for is_bare_repository() changes if extensions.worktreeConfig
>> +        * is disabled.
>> +        */
>> +       if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
>> +                                                         r->worktree ? "false" : "true",
>> +                                                         NULL, 0))) {
>> +               error(_("unable to set core.bare setting in worktree config"));
>> +               return res;
>> +       }
>> +       if (upgrade_repository_format(r, 1) < 0)
>> +               return error(_("unable to upgrade repository format to enable worktreeConfig"));
>> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
>> +               error(_("failed to set extensions.worktreeConfig setting"));
>> +               return res;
>> +       }
>> +
>> +       return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
>> +}
>> diff --git a/config.h b/config.h
>> @@ -253,6 +253,12 @@ void git_config_set_in_file(const char *, const char *, const char *);
>> +/**
>> + * Write a config value into the config.worktree file for the current
>> + * worktree. This will initialize extensions.worktreeConfig if necessary.
>> + */
>> +int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
> 
> I understand your desire to make this "setter" function as transparent
> and simple for clients as possible, however, I think it does too much
> by conflating two very distinct operations (one which changes the
> nature of the repository itself, and one which simply sets a config
> variable), and is far too magical. It doesn't help that the function
> name gives no indication of just how magical it is, and it is easy to
> imagine people calling this function thinking that it's just a simple
> "config setter" like all the other similarly-named functions, without
> realizing the impact it may have on the repository overall (i.e.
> upgrading to version 1 and changing to per-worktree config).
> 
> I would feel much more comfortable for the new utility function to
> have a single-purpose: namely, to upgrade the repository to a
> per-worktree configuration mode (if not already upgraded) as outlined
> in [4]. That's it. It shouldn't do anything other than that. This way,
> callers which need per-worktree configuration must call the new
> function explicitly to obtain the desired behavior, rather than
> getting per-worktree configuration as a magical and implicit
> side-effect of calling what looks like a plain old "config setter".
> This should make it easier to reason about. Taking this approach also
> means that you don't need to introduce a special-purpose "config
> setter" just for worktrees; instead, you'd use the normal existing
> "config setter" functions. For instance, if the new utility function
> is named enable_per_worktree_config(), then `git sparse-checkout init`
> might do something like this:

I understand your desire to separate these concerns, and maybe there
is value in having another method that _just_ does the "upgrade to
worktree config". However, if we don't also create this helper method
for setting worktree-specific config, then we are going to hit this
issue again.
 
>     enable_per_worktree_config(r);
>     config_path = repo_git_path(r, "config.worktree")
>     git_config_set_in_file_gently(config_path, "core.sparseCheckout", ...);
>     git_config_set_in_file_gently(config_path, "core.sparseCheckoutCone", ...);
> 
> (This, of course, assumes that repo_git_path() latches the changes
> made by enable_per_worktree_config() so that it "does the right
> thing", but it seems that existing code in `git sparse-checkout init`
> is already expecting it to do so, so perhaps it does work that way.)

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

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

Thanks,
-Stolee

Eric Sunshine Dec. 21, 2021, 11:29 p.m. UTC | #7

On Tue, Dec 21, 2021 at 8:46 AM Derrick Stolee <[e-mail]> wrote:
> On 12/21/2021 12:53 AM, Eric Sunshine wrote:
> > On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> > <[e-mail]> 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.

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

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

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

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

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

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

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

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

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

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

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

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

    % git sparse-checkout init --per-worktree

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

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

Referring back to what you said earlier:

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

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

Patch

diff --git a/config.c b/config.c
index 9c9eef16018..67f3d5015ef 100644
--- a/config.c
+++ b/config.c
@@ -2880,6 +2880,33 @@  int git_config_set_gently(const char *key, const char *value)
 	return git_config_set_multivar_gently(key, value, NULL, 0);
 }
 
+int repo_config_set_worktree_gently(struct repository *r,
+				    const char *key, const char *value)
+{
+	int res;
+	const char *config_filename = repo_git_path(r, "config.worktree");
+
+	/*
+	 * Ensure that core.bare reflects the current worktree, since the
+	 * logic for is_bare_repository() changes if extensions.worktreeConfig
+	 * is disabled.
+	 */
+	if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
+							  r->worktree ? "false" : "true",
+							  NULL, 0))) {
+		error(_("unable to set core.bare setting in worktree config"));
+		return res;
+	}
+	if (upgrade_repository_format(r, 1) < 0)
+		return error(_("unable to upgrade repository format to enable worktreeConfig"));
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
+		error(_("failed to set extensions.worktreeConfig setting"));
+		return res;
+	}
+
+	return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
+}
+
 void git_config_set(const char *key, const char *value)
 {
 	repo_config_set(the_repository, key, value);
diff --git a/config.h b/config.h
index 5531fc018e3..d8db420849e 100644
--- a/config.h
+++ b/config.h
@@ -253,6 +253,12 @@  void git_config_set_in_file(const char *, const char *, const char *);
 
 int git_config_set_gently(const char *, const char *);
 
+/**
+ * Write a config value into the config.worktree file for the current
+ * worktree. This will initialize extensions.worktreeConfig if necessary.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
+
 /**
  * write config values to `.git/config`, takes a key/value pair as parameter.
  */