[v2,3/5] worktree: add upgrade_to_worktree_config()

Commit Message

Derrick Stolee Dec. 21, 2021, 7:14 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 worktree.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 12 ++++++++++++
 2 files changed, 59 insertions(+)

Comments

Eric Sunshine Dec. 22, 2021, 12:45 a.m. UTC | #1

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
> 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?

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

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

> + * 1. Upgrading the repository format version to 1.
> + * 2. Adding extensions.worktreeConfig to the base config file.
> + * 3. Moving core.bare=true from the base config file to the base
> + *    repository's config.worktree file.

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

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

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

Junio C Hamano Dec. 22, 2021, 5:38 a.m. UTC | #2

"Derrick Stolee via GitGitGadget" <[e-mail]> 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()?

Is "core.bare" the only thing that needs special treatment?  Will it
stay to be, or will we see more configuration variables that need
special casing like this?

> +	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 Dec. 28, 2021, 3:03 p.m. UTC | #3

On 12/21/2021 7:45 PM, Eric Sunshine wrote:
> On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
> <[e-mail]> 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."

Bare worktrees don't exist, that is correct. But if one existed it
would be a directory where you could operate as if it is a bare repo,
but it has its own HEAD different from the base repo's HEAD. Not sure
why one would want it.

I guess the question is: what happens if someone writes core.bare=true
into their worktree config? A question to resolve another day, perhaps.
 
>> To help resolve this transition, create upgrade_to_worktree_config() to
>> navigate the intricacies of this operation. In particular, we need to
>> look for core.bare=true within the base config file and move that
>> setting into the core repository's config.worktree file.
>>
>> To gain access to the core repository's config and config.worktree file,
>> we reference a repository struct's 'commondir' member. If the repository
>> was a submodule instead of a worktree, then this still applies
>> correctly.
> 
> I'm not sure how much this commit message is going to help someone who
> did not participate in the discussion which led to this patch series.
> I think the entire commit message could be written more concisely like
> this:

Good suggestions to add the necessary context here. Thanks.

>     worktree: add helper to upgrade repository to per-worktree config
> 
>     Changing a repository to use per-worktree configuration is a
>     somewhat involved manual process, as described in the
>     `git-worktree` documentation, but it's easy to get wrong if the
>     steps are not followed precisely, which could lead to odd
>     anomalies such as Git thinking that a worktree is "bare" (which
>     conceptually makes no sense). Therefore, add a utility function to
>     automate the process of switching to per-worktree configuration
>     for modules which require such configuration. (In the future, it
>     may make sense to also expose this convenience as a `git-worktree`
>     command to automate the process for end-users, as well.)
> 
>     To upgrade the repository to per-worktree configuration, it performs
>     these steps:
> 
>     * enable `extensions.worktreeConfig` in .git/config
> 
>     * relocate `core.bare` from .git/config to .git/config.worktree
>       (if key exists)
> 
>     * relocate `core.worktree` from .git/config to
>       .git/config.worktree (if key exists)
> 
>     It also upgrades the repository format to 1 if necessary since
>     that is a prerequisite of using `extensions`.
> 
>> Helped-by: Eric Sunshine <[e-mail]>
>> Signed-off-by: Derrick Stolee <[e-mail]>
>> ---
>> diff --git a/worktree.c b/worktree.c
>> @@ -830,3 +831,49 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
>> +int upgrade_to_worktree_config(struct repository *r)
>> +{
>> +       int res;
>> +       int bare = 0;
>> +       struct config_set cs = { 0 };
> 
> This function is doing a lot of unnecessary work if per-worktree
> configuration is already enabled. The very first thing it should be
> doing is checking whether or not it actually needs to do that work,
> and short-circuit if it doesn't. I would think that simply checking
> whether `extensions.worktreeConfig` is true and returning early if it
> is would be sufficient.

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

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

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

>> +       git_configset_init(&cs);
>> +       git_configset_add_file(&cs, base_config_file);
>> +
>> +       /*
>> +        * If the base repository is bare, then we need to move core.bare=true
>> +        * out of the base config file and into the base repository's
>> +        * config.worktree file.
>> +        */
> 
> Here, too, it's not clear what "base" means. I think you want to say
> that it needs to "move `core.bare` from the shared config to the
> repo-specific config".

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

>> +       if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
>> +               if ((res = git_config_set_in_file_gently(base_worktree_file,
>> +                                                       "core.bare", "true"))) {
>> +                       error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
>> +                       goto cleanup;
>> +               }
>> +
>> +               if ((res = git_config_set_in_file_gently(base_config_file,
>> +                                                       "core.bare", NULL))) {
>> +                       error(_("unable to unset core.bare=true in '%s'"), base_config_file);
>> +                       goto cleanup;
>> +               }
>> +       }
> 
> This seems unnecessarily complicated and overly specific, thus
> potentially confusing. The requirements laid out in git-worktree.txt
> say only to move the configuration key from .git/config to
> .git/config.worktree; it doesn't add any qualifiers about the value
> being "true". And, indeed, we should not care about the value; it's
> immaterial to this operation. Instead, we should just treat it
> opaquely and relocate the key and value from .git/config (if it
> exists) to .git/config.worktree without interpretation.
> 
> The error messages are too specific, as well, by mentioning "true".
> They could instead be:
> 
>     unable to set `core.bare` in '%s'
> 
> and
> 
>     unable to remove `core.bare` from '%s'
> 
> However, there is a much more _severe_ problem with this
> implementation: it is incomplete. As documented in git-worktree.txt
> (and mentioned several times during this discussion), this utility
> function _must_ relocate both `core.bare` _and_ `core.worktree` (if
> they exist) from .git/config to .git/config.worktree. This
> implementation neglects to relocate `core.worktree`, which can leave
> things in quite a broken state (just as neglecting to relocate
> `core.bare` can).

It took me a long time to actually understand the purpose of
core.worktree, since it seems in conflict with the 'git worktree'
feature. Indeed, it is special-cased the same way core.bare is, so
this relocation is required.

>> +       if (upgrade_repository_format(r, 1) < 0) {
>> +               res = error(_("unable to upgrade repository format to enable worktreeConfig"));
>> +               goto cleanup;
>> +       }
>> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
>> +               error(_("failed to set extensions.worktreeConfig setting"));
>> +               goto cleanup;
>> +       }
> 
> The order in which this function performs its operations can leave the
> repository in a broken state if any of the steps fails. For instance,
> if setting `extensions.worktreeConfig=true` fails _after_ you've
> relocated `core.bare` (and `core.worktree`) to .git/config.worktree,
> then those configuration values will no longer be "active" since the
> config system won't consult .git/config.worktree without
> `extensions.worktreeConfig` enabled.
> 
> To be resilient against this sort of problem, you should perform the
> operations in this order:
> 
> (1) upgrade repository format to 1
> (2) enable `extensions.worktreeConfig`
> (3) relocate `core.bare` and `core.worktree`

This order can still cause some issues, specifically the worktree will
still think it is bare or the core.worktree value is incorrect, but that
is less serious than a broken base repo.

>> +cleanup:
>> +       git_configset_clear(&cs);
>> +       free(base_config_file);
>> +       free(base_worktree_file);
>> +       trace2_printf("returning %d", res);
> 
> Is this leftover debugging or intentional?

Leftover debugging. Thanks for catching.

Thanks,
-Stolee

Derrick Stolee Dec. 28, 2021, 3:13 p.m. UTC | #4

On 12/22/2021 12:38 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> 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).

Deep down in the git_config_set_... stack, it returns helpful error
codes that I thought would be good to communicate upwards. cmd_config()
uses these error codes, too, but that's a more obvious place where the
user is expecting the error code to be related to the config operation.
 
If this communication is not helpful, then I will use the pattern you
suggest. I will assume this is the case unless told otherwise.

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

I don't think we should add burden to the callers to check that the
repo is not using worktree config. Thinking back to your rename idea
this could be "ensure_using_worktree_config()" to make it clear that
we will use the worktree config after the method, whether or not we
were using it beforehand.

I think this current implementation is non-damaging if someone calls
it after already using worktree config. The change being that a user
can go and add core.bare=false to the common config file and break all
worktrees again, and the current implementation will help heal that.
It's probably better to let the user have that ability to mess things
up and not try to fix something so broken.

Thanks,
-Stolee

Eric Sunshine Dec. 28, 2021, 4:58 p.m. UTC | #5

On Tue, Dec 28, 2021 at 10:03 AM Derrick Stolee <[e-mail]> 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.

> >> +       /*
> >> +        * If the base repository is bare, then we need to move core.bare=true
> >> +        * out of the base config file and into the base repository's
> >> +        * config.worktree file.
> >> +        */
> >
> > Here, too, it's not clear what "base" means. I think you want to say
> > that it needs to "move `core.bare` from the shared config to the
> > repo-specific config".
>
> Yes, but specific to the original/root/bare repo. I'm open to preferences
> here, but "repo" isn't specific enough.

There's only a single repository, so this should be clear, however,
there appears to be some terminology mismatch. I'll enumerate a few
items in an effort to clarify how I'm using the terminology...

    .git/ -- the repository residing within the main worktree

    bare.git/ -- a bare repository

    .git/config -- configuration shared by the repository and all worktrees

    bare.git/config -- configuration shared by the repository and all worktrees

    .git/config.worktree -- configuration specific to the main worktree

    bare.git/config.worktree -- configuration specific to the bare repository

    .git/worktrees/<id>/config -- configuration specific to worktree <id>

    bare.git/worktrees/<id>/config -- configuration specific to worktree <id>

> > However, there is a much more _severe_ problem with this
> > implementation: it is incomplete. As documented in git-worktree.txt
> > (and mentioned several times during this discussion), this utility
> > function _must_ relocate both `core.bare` _and_ `core.worktree` (if
> > they exist) from .git/config to .git/config.worktree. This
> > implementation neglects to relocate `core.worktree`, which can leave
> > things in quite a broken state (just as neglecting to relocate
> > `core.bare` can).
>
> It took me a long time to actually understand the purpose of
> core.worktree, since it seems in conflict with the 'git worktree'
> feature. Indeed, it is special-cased the same way core.bare is, so
> this relocation is required.

Indeed, the situation is unfortunately confusing in this area.
`core.worktree` predates multiple-worktree support (i.e.
`git-worktree`) by quite a long time and is a mechanism for allowing
the repository (.git/) to exist at a distinct location from the
worktree (by which I mean "main worktree" since there was no such
thing as a "linked worktree" at a time). `git-worktree` generalized
the concept by making multiple worktrees first-class citizens, but
`core.worktree` and GIT_WORKTREE still need to be supported for
backward compatibility even though they conflict (or can conflict)
rather badly with multiple-worktrees.

Derrick Stolee Dec. 28, 2021, 5:03 p.m. UTC | #6

On 12/28/2021 11:58 AM, Eric Sunshine wrote:
> On Tue, Dec 28, 2021 at 10:03 AM Derrick Stolee <[e-mail]> 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.

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

Thanks,
-Stolee

Patch

diff --git a/worktree.c b/worktree.c
index 2c155b10150..e8ddbe2bfcc 100644
--- a/worktree.c
+++ b/worktree.c
@@ -5,6 +5,7 @@ 
 #include "worktree.h"
 #include "dir.h"
 #include "wt-status.h"
+#include "config.h"
 
 void free_worktrees(struct worktree **worktrees)
 {
@@ -830,3 +831,49 @@  int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 	*wtpath = path;
 	return 0;
 }
+
+int upgrade_to_worktree_config(struct repository *r)
+{
+	int res;
+	int bare = 0;
+	struct config_set cs = { 0 };
+	char *base_config_file = xstrfmt("%s/config", r->commondir);
+	char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, base_config_file);
+
+	/*
+	 * If the base repository is bare, then we need to move core.bare=true
+	 * out of the base config file and into the base repository's
+	 * config.worktree file.
+	 */
+	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
+		if ((res = git_config_set_in_file_gently(base_worktree_file,
+							"core.bare", "true"))) {
+			error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
+			goto cleanup;
+		}
+
+		if ((res = git_config_set_in_file_gently(base_config_file,
+							"core.bare", NULL))) {
+			error(_("unable to unset core.bare=true in '%s'"), base_config_file);
+			goto cleanup;
+		}
+	}
+	if (upgrade_repository_format(r, 1) < 0) {
+		res = error(_("unable to upgrade repository format to enable worktreeConfig"));
+		goto cleanup;
+	}
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
+		error(_("failed to set extensions.worktreeConfig setting"));
+		goto cleanup;
+	}
+
+cleanup:
+	git_configset_clear(&cs);
+	free(base_config_file);
+	free(base_worktree_file);
+	trace2_printf("returning %d", res);
+	return res;
+}
diff --git a/worktree.h b/worktree.h
index 8b7c408132d..170b6b1e1f5 100644
--- a/worktree.h
+++ b/worktree.h
@@ -182,4 +182,16 @@  void strbuf_worktree_ref(const struct worktree *wt,
 			 struct strbuf *sb,
 			 const char *refname);
 
+/**
+ * Upgrade the config of the current repository and its base (if different
+ * from this repository) to use worktree-config. This might adjust config
+ * in both repositories, including:
+ *
+ * 1. Upgrading the repository format version to 1.
+ * 2. Adding extensions.worktreeConfig to the base config file.
+ * 3. Moving core.bare=true from the base config file to the base
+ *    repository's config.worktree file.
+ */
+int upgrade_to_worktree_config(struct repository *r);
+
 #endif