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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Sean Allred <allred.sean@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 3/4] config: add repo_config_set_worktree_gently()
Date: Mon, 20 Dec 2021 19:01:34 -0500	[thread overview]
Message-ID: <CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com> (raw)
In-Reply-To: <6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com>

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

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.