Re: [PATCH v2 0/5] Sparse checkout: fix bug with worktree of bare repo

From: Elijah Newren <newren@gmail.com>
Date: Dec 22, 2021, 22:54 UTC
Message-ID: <CABPp-BG7nwsdEYrnfqhAbWU4ndJHcqGf6RS_6DzJittuNVLvoA@mail.gmail.com>
In-Reply-To: <pull.1101.v2.git.1640114048.gitgitgadget@gmail.com>

On Wed, Dec 22, 2021 at 8:00 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 17 quoted lines

>
> 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 might need to be set. This only
> matters when the base repository is bare, since creating the config.worktree
> file and enabling extensions.worktreeConfig will cause Git to treat the base
> repo's core.bare=false as important for this worktree.
>
> This series fixes this, but also puts in place some helpers to prevent this
> from happening in the future. While here, some of the config paths are
> modified to take a repository struct.
>
> The critical bits are in Patches 3, 4, and 5 which introduce a helper for
> upgrading to worktree config, a helper to write to worktree config, and then
> consume that config helper in builtin/sparse-checkout.c and sparse-index.c.

Based on the description I went and fetched the patch series and tried it out.

This feels like a bandaid to me. In addition to fixating on core.bare (thus overlooking core.worktree), it also overlooks that people can use worktrees without using sparse-checkout. What if they do something like:

  git clone --bare $URL myrepo
  cd myrepo
  git worktree add foo
  git worktree add bar
  git worktree add baz
  ... days/weeks later ...
  cd foo
  git config extensions.worktreeConfig true
  git config status.showUntrackedFiles no  # Or other config options
  ... hours/days later ..
  cd ../bar
  git status

At this point the user gets "fatal: this operation must be run in a work tree". And it's much, much worse if the original clone was not bare, but had core.worktree pointing somewhere else (because then the `git status` invocation will show the differences between the *other* worktree with the HEAD of *this* worktree). I think that "git worktree add" should check if either core.bare is false or core.worktree is set, and if so then set extensions.worktreeConfig and migrate the relevant config.

While there may be some concern about non-git clients not understanding extensions.worktreeConfig, I'd say that this type of situation is one where we are just flat broken without it. Without it, we're stuck in a situation that will: (a) confuse users ("Why does core.bare/core.worktree seem to get ignored or have incorrect values?"), (b) confuse non-git clients (are they really going to have the tricky logic to overrule core.bare/core.worktree when in another worktree?), (c) confuse git itself after subsequent configuration tweaks, and (d) (related to item c) lead to ever more complicated logic in core git to attempt to know when and how to overrule core.bare and core.worktree as additional concerns enter the picture (e.g. will someone contribute code to override core.bare/core.worktree when run from a worktree with extensions.worktreeConfig=true, just as someone originally wrote code to override core.bare/core.worktree when the extensions.worktreeConfig setting wasn't present?)

I also think `git worktree add` should handle additional configuration items related to sparse checkouts (as we've discussed elsewhere in the past), but that's going a bit outside the scope of this series; I only mention it so that we're aware the functionality added to `git worktree add` will be getting some additions in the future.