Sparse checkout: fix bug with worktree of bare repo #1101
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1101.git.1640015844.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -2880,9 +2880,41 @@ int git_config_set_gently(const char *key, const char *value) | |||
| return git_config_set_multivar_gently(key, value, NULL, 0); | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
...
> + /*
> + * 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
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Dec 20, 2021 at 7:01 PM Eric Sunshine <sunshine@sunshineco.com> 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).
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/20/2021 7:01 PM, Eric Sunshine wrote:
> 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
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
| @@ -2880,9 +2880,41 @@ int git_config_set_gently(const char *key, const char *value) | |||
| return git_config_set_multivar_gently(key, value, NULL, 0); | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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.)
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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:
>> 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:
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
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Tue, Dec 21, 2021 at 8:46 AM Derrick Stolee <stolee@gmail.com> wrote:
> 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.
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.
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
derrickstolee
force-pushed
the
sparse-checkout/bare-worktree-bug
branch
from
6202f76
to
06457fa
Compare
|
/submit |
|
Submitted as pull.1101.v2.git.1640114048.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -5,6 +5,7 @@ | |||
| #include "worktree.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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?
> + 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);
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/21/2021 7:45 PM, Eric Sunshine wrote:
> 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."
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 <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.
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
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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.
> >> + /*
> >> + * 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.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/28/2021 11:58 AM, Eric Sunshine wrote:
> 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.
Your use of "main worktree" is what I am meaning. I will adopt your
terminology.
Thanks,
-Stolee
| @@ -21,6 +21,7 @@ | |||
| #include "dir.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
I still feel very uncomfortable about this function conflating two
very different concerns (upgrading the repository to per-worktree
config, and the simple setting of a config variable). Since I've
already explained my discomfort and suggested alternatives several
times during this discussion (most recently at [1]), I don't have all
that much more to say. However, I do have a few comments...
First, I would have no problem if this function "did the right thing"
where "the right thing" means correctly choosing between .git/config
and .git/config.worktree depending upon whether or not
`extensions.worktreeConfig` is set, and only that. It should not
perform any sort of repository upgrade on its own. Doing it this way
should satisfy your major concern of relieving callers from having to
figure out the correct configuration file name.
Second, if you absolutely must have this function, as implemented, as
part of the public API (despite my protests), please give it a name
which is sufficiently different from the other "config setter"
functions so that people won't be confused into thinking it's just
another "setter" without realizing that calling it has repository-wide
consequences.
Third, I don't have an objection if you want to make this function
private (static) to builtin/sparse-checkout.c, thus omitting it from
the public API. This way you can have its convenience where you want
it, and we can delay finishing this discussion until such time that it
becomes apparent that other modules want its convenience, as well, if
that ever comes about.
[1]: https://lore.kernel.org/git/CAPig+cRombN-8g0t7Hs9qQypJoY41gK3+kvypH4D0G6EB4JgbQ@mail.gmail.com/
> 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.
> */
> --
| @@ -5,6 +5,7 @@ | |||
| #include "worktree.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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()?
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
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/22/2021 12:38 AM, Junio C Hamano wrote:
> "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).
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
| @@ -356,26 +356,17 @@ enum sparse_checkout_mode { | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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' '
Nit: Although `init` doing an incomplete job of enabling per-worktree
config was indeed the source of the problem, it actually manifested
when invoking other commands, such as `set`. Consequently, it may be
slightly misleading to talk about `init` in the test title. A title
such as "worktree of a bare repo" might be good enough. Anyhow, just a
nit.
> + 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 &&
Nit: I'm rather "meh" on explicitly checking `core.bare` here since
it's not particularly relevant to the test: just testing `init + set`
alone is enough to trigger the bug which begat this patch series.
Future readers of this test might even be confused by the presence of
this `core.bare` check.
> + git sparse-checkout set /*
The `/*` is expanding to all entries in the root of the filesystem,
which probably isn't what you intended. I suspect you want literal
"/*", in which case you need to quote it:
git sparse-checkout set "/*"
> + ) &&
> + git -C bare config --list --show-origin >actual &&
> + grep "file:config.worktree core.bare=true" actual
As mentioned above, I'm fairly meh on this part (and perhaps leaning
toward the negative) since it places too much emphasis on a low-level
detail. I _could_ see this as a test of the new function which
upgrades the repo to per-worktree config, but that's not what _this_
test is about.
> +'
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
This branch is now known as |
|
This patch series was integrated into seen via git@3dcb349. |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
User |
|
This patch series was integrated into seen via git@2121a5e. |
|
This patch series was integrated into seen via git@0df8531. |
|
There was a status update in the "New Topics" section about the branch "git sparse-checkout" wants to work with per-worktree configration, but did not work well in a worktree attached to a bare repository. Expecting a redesign? cf. <CABPp-BG7nwsdEYrnfqhAbWU4ndJHcqGf6RS_6DzJittuNVLvoA@mail.gmail.com> source: <pull.1101.v2.git.1640114048.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@7bc84a0. |
|
This patch series was integrated into seen via git@0db6365. |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
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. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
`git sparse-checkout set/init` enables worktree-specific configuration[*] by setting extensions.worktreeConfig=true, but neglects to perform the additional necessary bookkeeping of relocating `core.bare=true` and `core.worktree` from $GIT_COMMON_DIR/config to $GIT_COMMON_DIR/config.worktree, as documented in git-worktree.txt. As a result of this oversight, these settings, which are nonsensical for secondary worktrees, can cause Git commands to incorrectly consider a worktree bare (in the case of `core.bare`) or operate on the wrong worktree (in the case of `core.worktree`). Fix this problem by taking advantage of the recently-added init_worktree_config() which enables `extensions.worktreeConfig` and takes care of necessary bookkeeping. While at it, for backward-compatibility reasons, also stop upgrading the repository format to "1" since doing so is (unintentionally) not required to take advantage of `extensions.worktreeConfig`, as explained by 1166419 ("Revert "check_repository_format_gently(): refuse extensions for old repositories"", 2020-07-15). [*] 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>
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>
This method was created in f1de981 (config: fix leaks from git_config_get_string_const(), 2020-08-14) but its only use was in the repo_config_get_string_tmp() method, also declared in config.h and implemented in config.c. Since this is otherwise unused and is a very similar implementation to git_configset_get_value(), let's remove this declaration. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee
force-pushed
the
sparse-checkout/bare-worktree-bug
branch
from
85779df
to
f687a0b
Compare
|
/submit |
|
/submit |
|
Submitted as pull.1101.v6.git.1644269583.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
| @@ -5,6 +5,7 @@ | |||
| #include "worktree.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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")? 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.
> + 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
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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.
I think the end state is correct for all uses here, since we only
run this after checking to see if the config value exists in the
'from_file', so 'value' is correct (and this is a static method,
not a generally-useful method for config.h).
Perhaps a "write_in_new_and_remove_from_old()" would be a better,
if verbose, name. I struggle to find a less cumbersome name, and
"move" seems to match the intent pretty well in the context of its
use.
>> + 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?
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.
>> + 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?
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.
>> + * 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.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, Feb 8, 2022 at 2:09 PM Junio C Hamano <gitster@pobox.com> 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 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")?
Yes, and in fact that was the case which generated the original bug
report -- a bare clone where the affected individual started using
`git worktree add` to create some non-primary worktrees (and then also
used sparse-checkout in some of them).
> 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?
Moving the core.bare==false setting might make sense. In the previous
discussions, we tried to hypothesize about usage of old git clients
and non-git clients (jgit, etc.) on the same repos, and didn't know if
some of those would break if they couldn't find a `core.bare` setting
anywhere (since they wouldn't know to look in config.worktree). We
needed to migrate core.bare=true to avoid an incorrect value affecting
all worktrees (and thus we figured it was worth the risk of breaking
older git/non-git clients because having older clients be broken is
better than having all clients including current git be broken), but
the same wasn't true for core.bare=false. That said, we don't
actively know of any such clients that would be hurt by such a
migration.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Derrick Stolee <stolee@gmail.com> writes:
> 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.
Yes. "Why are we _ignoring_ what is in the old file when we claim
to be _moving_?" was the question I had upon seeing this function.
>> 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.
> I think the end state is correct for all uses here, since we only
> run this after checking to see if the config value exists in the
> 'from_file', so 'value' is correct (and this is a static method,
> not a generally-useful method for config.h).
As long as this is used on a single-valued "last one wins" variable,
the callers and this helper taken together will do the right thing.
> Perhaps a "write_in_new_and_remove_from_old()" would be a better,
> if verbose, name. I struggle to find a less cumbersome name, and
> "move" seems to match the intent pretty well in the context of its
> use.
The name is fine as long as the requirement for the caller is made
clear. A short comment to help the next reader from having to ask
the same question before the helper may be sufficient.
>> 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.
Unless there is a lower-precedence configuration file that we have
to override, yes, not writing core.bare=false upon "worktree add" is
fine. I simply do not know if we need to do something special in
order to defeat /etc/gitconfig or $HOME/.gitconfig with the repository
or the worktree specific configuration file.
> 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.
OK. The .git _file_ is our moral equivalent to a symbolic link, and
I forgot about that.
I also wonder if we should do something like what we do for refs
(i.e. the API knows which refs are per-worktree and which are
global, so the callers do not have to care and just can say things
like "update HEAD to this value", and "give me the value of
refs/bisect/good") when repo_set_config*() is called, but that is
outside the scope of this step, which is about one-time migration.
As the code for migration go, I think I am happy with what it wants
to do and how it does it.
Thanks.
| @@ -6,3 +6,34 @@ extensions.objectFormat:: | |||
| Note that this setting should only be set by linkgit:git-init[1] or | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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)
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 2/8/2022 5:20 PM, Junio C Hamano wrote:
> "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.
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/
> 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.
Right. I think that since the primary worktree uses $GIT_COMMON_DIR
as its location for base information (like HEAD) it also means that
its worktree-specific config file cannot be called "config".
Perhaps there could have been a way to split the worktrees so the
primary worktree got its own directory within ".git/worktrees/", but
my guess is that the design optimized for backwards compatibility:
Git clients that don't understand worktrees could still interact with
the primary worktree.
> 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)
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".
Other than that technicality, which could be completely correct, this
is a good idea to include.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 2/9/2022 12:19 PM, Junio C Hamano wrote:
> 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.
Thanks for this additional context! It means that I need to look
around more carefully, not that your patch needs any changes.
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Wed, Feb 9, 2022 at 9:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
You had me worried for a second; things would be really broken if
these pseudorefs weren't per-worktree.
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?)
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, Feb 8, 2022 at 2:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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)
We could also add pseudorefs to the list of things maintained
separately in the final parenthetical comment, but otherwise looks
good.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Elijah Newren <newren@gmail.com> writes:
> 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?)
I actually think the longer-term direction is to describe that these
are always per-worktree, without referring to $GIT_DIR or giving any
hints that these may be represented as a file in the filesystem.
That would leave the door open for the reftable backend to take them
over as well as the normal refs.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Elijah Newren <newren@gmail.com> writes:
>> [[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.
I think what needs updating is the per_worktree_ref section. Before
we say "later include other unusual refs", not so unusual pseudorefs
can be mentioned there.
| @@ -21,6 +21,7 @@ | |||
| #include "dir.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
Makes sense.
> +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);
> +}
OK.
> @@ -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?
> +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;
> }
OK.
> 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.
> 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);
>
> /**
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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.
>> 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.
Here, you're right. This one should have followed the same pattern
of having the "git_*" equivalent call the "repo_*" method, but
instead I incorrectly inlined some of the code.
The proper body should be
void git_config_set_multivar(const char *key, const char *value,
const char *value_pattern, unsigned flags)
{
repo_config_set_multivar_gently(the_repository, key, value,
value_pattern, flags);
}
to follow convention.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 2/9/2022 12:49 PM, Junio C Hamano wrote:
> 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?
True, they could be split. The reason to create the _multivar_
version is for the case that worktree config is not specified,
so that is the only caller at the moment.
> 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.
This is an interesting idea. This would require creating a list
of "should be per-worktree" config keys that are checked within
the different *_config_set_* methods.
The biggest technical hurdle is that the multivar versions might
need to send a subset of the given config into the worktree
config and the rest to the common config.
Let's see how this progresses in the future, and whether we
have more config that we believe _must_ be stored on a per-
worktree basis. At that point, we can see whether the current
"distributed responsibility" model is too cumbersome.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Derrick Stolee <stolee@gmail.com> writes:
> True, they could be split. The reason to create the _multivar_
> version is for the case that worktree config is not specified,
> so that is the only caller at the moment.
Sorry, but I am not following this part.
> This is an interesting idea. This would require creating a list
> of "should be per-worktree" config keys that are checked within
> the different *_config_set_* methods.
Yes.
> The biggest technical hurdle is that the multivar versions might
> need to send a subset of the given config into the worktree
> config and the rest to the common config.
Yes, instead of having the caller do it.
> Let's see how this progresses in the future, and whether we
> have more config that we believe _must_ be stored on a per-
> worktree basis. At that point, we can see whether the current
> "distributed responsibility" model is too cumbersome.
It is not too distributed, which is a saving grace. The callers
know they are setting core.sparseCheckout* and they are responsible
to call the per-worktree version. It would be like having in ref
API an update_HEAD() helper for modifying HEAD, instead of having a
more generic update_ref() that can modify any ref and pass "HEAD" as
an argument to the latter. The caller needs to know a bit more
details about what needs to happen when dealing with a special thing,
but the special case knowledge is fairly concentrated.
|
This patch series was integrated into seen via git@51e2a3f. |
|
This patch series was integrated into seen via git@5022211. |
|
There was a status update in the "Cooking" section about the branch "git sparse-checkout" wants to work with per-worktree configration, but did not work well in a worktree attached to a bare repository. Will merge to 'next'? cf. <20220204081336.3194538-1-newren@gmail.com> cf. <CAPig+cRrRxuTeByhKkLs_KDaWY8-r4+jrwT83A-r+sBQsmebMw@mail.gmail.com> source: <pull.1101.v6.git.1644269583.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@63bdb4c. |
|
This patch series was integrated into seen via git@c88ab0d. |
|
This patch series was integrated into seen via git@69e1a1e. |
|
There was a status update in the "Cooking" section about the branch "git sparse-checkout" wants to work with per-worktree configration, but did not work well in a worktree attached to a bare repository. Will merge to 'next'? cf. <20220204081336.3194538-1-newren@gmail.com> cf. <CAPig+cRrRxuTeByhKkLs_KDaWY8-r4+jrwT83A-r+sBQsmebMw@mail.gmail.com> source: <pull.1101.v6.git.1644269583.gitgitgadget@gmail.com> |
| @@ -335,6 +335,69 @@ static int add_worktree(const char *path, const char *refname, | |||
| strbuf_addf(&sb, "%s/commondir", sb_repo.buf); | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Feb 7, 2022 at 4:33 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.
> [...]
> 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);
> + }
Given that add_worktree() is already overly long, a good future
cleanup would be to move these two new hunks of functionality into
separate functions -- copy_sparsity() and copy_config(), perhaps -- in
builtin/worktree.c. This should be simple enough since (I think) the
only state that needs to be passed to them is `sb_repo`. But such
cleanup needn't hold up this series.
|
This patch series was integrated into seen via git@aba77ec. |
|
This patch series was integrated into next via git@f86bec6. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
<id>over<worktree-name>Updates in v5
Responded to most of Eric's suggestions. I pushed back on one about a comment including information from the commit message, but everything else should be as Eric suggested.
Updates in v4
Thanks,
-Stolee
cc: stolee@gmail.com
cc: sunshine@sunshineco.com
cc: allred.sean@gmail.com
cc: gitster@pobox.com
cc: Elijah Newren newren@gmail.com
cc: Bagas Sanjaya bagasdotme@gmail.com
cc: Jean-Noël AVILA jn.avila@free.fr
cc: derrickstolee@github.com