Re: [PATCH 3/4] config: add repo_config_set_worktree_gently()

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 21, 2021, 13:41 UTC
Message-ID: <73744786-4248-f04f-5d56-11e938d174e7@gmail.com>
In-Reply-To: <CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com>

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

Show 57 quoted lines

> 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

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