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

[Derrick Stolee <stolee@gmail.com>]

On 12/19/2021 7:58 PM, Eric Sunshine wrote:
> On Sun, Dec 19, 2021 at 3:47 PM Sean Allred <allred.sean@gmail.com> wrote:
>>> 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.

>> Ahh, that makes sense!  I did notice the `core.bare` setting being
>> respected in source and figured this had a part to play (which is why
>> I included git-config output).
>>
>> I think then that I was overzealous in trying to MWE-ify the issue: as
>> I noted, I found this issue when I was trying to perform a
>> sparse-checkout within the worktree.  To memory (I don't have my work
>> system at the moment and don't have its `history`), I think it went
>> something like this:
>>
>>     git worktree add --no-checkout ../next && cd ../next
>>     git sparse-checkout init --cone # auto-created a worktree config
>>     git sparse-checkout set t
> 
> Thanks for this information. I haven't followed sparse-checkout mode
> at all and haven't used it, so I quickly scanned the man page for the
> worktree-relevant information, and indeed I was able to reproduce the
> problem using the recipe (with the prerequisite that the repository is
> bare) which you present here.
> 
>> 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:

static int set_config(enum sparse_checkout_mode mode)
{
	const char *config_path;

	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;
	}

	config_path = git_path("config.worktree");
	git_config_set_in_file_gently(config_path,
				      "core.sparseCheckout",
				      mode ? "true" : NULL);

	git_config_set_in_file_gently(config_path,
				      "core.sparseCheckoutCone",
				      mode == MODE_CONE_PATTERNS ? "true" : NULL);

	if (mode == MODE_NO_PATTERNS)
		set_sparse_index_config(the_repository, 0);

	return 0;
}

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.

>> Based on the above, it sounds like `init` is creating the
>> worktree-specific config, but is not overriding `core.bare` in that
>> config.  Would a patch to take this step this automatically be
>> well-received?
> 
> This looks like a legitimate oversight, so some sort of patch to
> resolve it would likely be welcomed.
> 
>> 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.

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

I'll take a stab at this today.

> (I've cc:'d a few sparse checkout area experts, though I may have
> forgotten some.)

Thank you for CC'ing me!
-Stolee