Re: Bug report - Can create worktrees from bare repo / such worktrees can fool is_bare_repository()

`On Mon, Dec 20, 2021 at 9:11 AM Derrick Stolee [off-list ref] wrote:

On 12/19/2021 7:58 PM, Eric Sunshine wrote:

quoted

On Sun, Dec 19, 2021 at 3:47 PM Sean Allred [off-list ref] wrote:

quoted

quoted

However, you missed the step (discussed in [1]) in which it is your
responsibility to move the `core.bare=true` setting from
git.git/config to git.git/worktree.config manually after setting
`extensions.worktreeconfig=true`.

Thanks for this context of added responsibility. It seems a bit strange
to require this, since it doesn't make any sense to have a bare worktree
(at least not to me, feel free to elaborate on the need of such a
situation).

I think the most defensive thing to do would be to always special case
core.bare to false when in a worktree. But if there is a reason to allow
bare worktrees, then that isn't feasible.

I suppose one of Duy's motivations when adding worktree-specific
configuration -- and, importantly, configuration specific to the main
worktree -- was to get rid of the ugly special cases (such as
hard-coding overrides for `core.bare` and `core.worktree`) required by
the original (mis-)design. Importantly, those special-cases need to be
implemented by all third-party tools too, so it's not a winning
approach, whereas worktree-specific configuration gets by without
special cases and could easily be implemented by foreign tools. In the
long run, rather than re-introducing such overrides, a better solution
probably would be to make worktree-specific configuration the default,
but that's a decision for some other day.

quoted

quoted

I think either the git-sparse-checkout-set command (or the
git-checkout I ran after) would fail complaining that I was not in a
worktree.

It is indeed the `git sparse-checkout set` command which fails.

Right, 'init' will set the sparse-checkout information in your worktree
config and initialize it as needed, here:

        if (upgrade_repository_format(1) < 0)
                die(_("unable to upgrade repository format to enable worktreeConfig"));
        if (git_config_set_gently("extensions.worktreeConfig", "true")) {
                error(_("failed to set extensions.worktreeConfig setting"));
                return 1;
        }

So, we are manually specifying "put this in the config.worktree file"
and not going through some "initialize worktree config" helper. Such
a helper would be useful to avoid this issue in the future.

Yes, I was planning to suggest this in a follow-up message.
Specifically, I think top-level worktree.[hc] (not builtin/worktree.c)
should publish a function which enables worktree-specific
configuration _and_ does all the necessary bookkeeping, such as moving
`core.bare` and `core.worktree` from .git/config to
.git/worktree.config. That way, not only can git-sparse-checkout take
advantage of it, but so can any command which needs the functionality
in the future, as well as the fictitious "git worktree manage" command
I mentioned earlier if it ever materializes.

quoted

quoted

I see two options for when to set `core.bare=false` in
worktree-specific config:

  1. At git-worktree-add: This is probably the earliest time which
     makes sense, but may be over-reach.  I'm not up-to-speed on how
     worktree-specific configs are generally considered on this list.
     If I were implementing a workaround, though, this is probably
     where I'd make it.

My knee-jerk reaction is that applying a "fix" to `git worktree add`
to assign `core.bare=false` in the worktree-specific config may be
misguided, or at least feels too much like a band-aid. Although it's
true that that would address the problem for worktrees created after
`extensions.worktreeconfig=true` is set, it won't help
already-existing worktrees. This reason alone makes me hesitant to
endorse this approach.

Yeah, my concern is that it requires the extension and could cause
some tools to stop working immediately. If we have the extension
already, it might make sense to initialize the file then.

I'm not following what you're saying. Initialize which file? When?

Anyhow, this does bring up a good point and it makes me wonder if
git-sparse-checkout (or whatever helper function is implemented)
should warn the user that setting this extension could break foreign
tools and that the repository format is being upgraded.

quoted

quoted

  2. At git-sparse-checkout-init: This is where the problem begins to
     have an effect, so this might also make sense.

Yes, if I'm understanding everything correctly, this seems like the
best and most localized place to address the problem at this time. The
simple, but too minimal, approach would be for `git sparse-checkout
init` to simply add `core.bare=false` to the worktree-specific config,
however, this only addresses the immediate problem and still leaves
things broken in general for any non-sparse worktrees.

So, the better and more correct approach, while still being localized
to `git sparse-checkout init` is for it to respect the documented
requirement and automatically move `core.bare` and `core.worktree`
from .git/config to .git/worktree.config if those keys exist. That
should leave everything in a nice Kosher state for all worktrees,
existing and newly-created.

I agree that this is the only place that currently _writes_ to the
worktree config. All other code paths that seem to care about the
worktree config specifically only read from the config using a
modified scope.

My thought on the direction to go would be to extract some code
from the set_config() in builtin/sparse-checkout.c into a config
helper, say "ensure_worktree_config_exists()", that adds the
extension, creates the file, and then adds core.bare=false.

Even better, we could create a config helper that writes to the
worktree config, and _that_ could ensure the config is set
correctly before writing the requested value.

Please do not take the approach of setting `core.bare=false` in the
worktree-specific config file. As I mentioned in the quoted text just
above, that only resolves the problem for the worktree in question but
leaves all other potentially worktrees broken (both existing worktrees
and all new worktrees which are not being used for sparse checkout).
The _only_ correct thing to do, as far as I can see, is for the new
helper function to precisely implement the requirements as laid out by
git-worktree.txt: specifically, enable
`extensions.worktreeConfig=true` _and_ relocate `core.bare` and
`core.worktree` from .git/config to .git/worktree.config.