[0/4] Sparse checkout: fix bug with worktree of bare repo

Message

Philippe Blain via GitGitGadget Dec. 20, 2021, 3:57 p.m. UTC

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 and 4 which introduce the helper and then
consume it in builtin/sparse-checkout.c and sparse-index.c.

[1]
https://lore.kernel.org/git/[e-mail]/
[2]
https://lore.kernel.org/git/[e-mail]/

Thanks, -Stolee

Derrick Stolee (4):
  setup: use a repository when upgrading format
  config: make some helpers repo-aware
  config: add repo_config_set_worktree_gently()
  sparse-checkout: use repo_config_set_worktree_gently()

 builtin/sparse-checkout.c          | 25 +++++--------
 config.c                           | 56 ++++++++++++++++++++++++++++--
 config.h                           | 13 +++++++
 list-objects-filter-options.c      |  2 +-
 repository.h                       |  2 +-
 setup.c                            |  6 ++--
 sparse-index.c                     | 10 ++----
 t/t1091-sparse-checkout-builtin.sh | 14 +++++++-
 8 files changed, 95 insertions(+), 33 deletions(-)


base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Comments

Eric Sunshine Dec. 20, 2021, 4:21 p.m. UTC | #1

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> 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.

Thanks for jumping on this so quickly. Unfortunately, however, as
mentioned in [1] and [2], I think the approach implemented here of
setting `core.bare=false` in the worktree-specific config is
fundamentally flawed since it only addresses the problem for worktrees
in which `git sparse-checkout init` has been run, but leaves all other
worktrees potentially broken (both existing and new worktrees). As far
as I can see, the _only_ correct solution is for the new helper
function to enable `extensions.worktreeConfig` _and_ relocate
`core.bare` and `core.worktree` from .git/config to
.git/worktree.config, thus implementing the requirements documented in
git-worktree.txt.

I also raised a separate question in [2] about whether `git
sparse-checkout init` or the new helper function should be warning the
user that upgrading the repository format and setting
`extensions.worktreeConfig` might break third-party tools. However,
that question is tangential to the fix being addressed here and
doesn't need to be addressed by this series.

[1]: https://lore.kernel.org/git/[e-mail]/
[2]: https://lore.kernel.org/git/[e-mail]/

Derrick Stolee Dec. 20, 2021, 5:34 p.m. UTC | #2

On 12/20/2021 11:21 AM, Eric Sunshine wrote:
> On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>> 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.
> 
> Thanks for jumping on this so quickly. Unfortunately, however, as
> mentioned in [1] and [2], I think the approach implemented here of
> setting `core.bare=false` in the worktree-specific config is
> fundamentally flawed since it only addresses the problem for worktrees
> in which `git sparse-checkout init` has been run, but leaves all other
> worktrees potentially broken (both existing and new worktrees). As far
> as I can see, the _only_ correct solution is for the new helper
> function to enable `extensions.worktreeConfig` _and_ relocate
> `core.bare` and `core.worktree` from .git/config to
> .git/worktree.config, thus implementing the requirements documented in
> git-worktree.txt.

Thanks for clarifying what I had misread. I commented on Patch 3 at the
place that should be changed to relocate the setting. The test in patch 4
could have multiple worktrees to verify that it works.

I'll plan on providing a v2 with that change tomorrow, leaving time to
find any other glaring errors.
 
> I also raised a separate question in [2] about whether `git
> sparse-checkout init` or the new helper function should be warning the
> user that upgrading the repository format and setting
> `extensions.worktreeConfig` might break third-party tools. However,
> that question is tangential to the fix being addressed here and
> doesn't need to be addressed by this series.

Let's continue to simmer on this one. If there is a clear direction for
doing this (should it just be an advice message?) then we can do that
whenever.
 
> [1]: https://lore.kernel.org/git/[e-mail]/
> [2]: https://lore.kernel.org/git/[e-mail]/

Thanks,
-Stolee

Eric Sunshine Dec. 21, 2021, 6:10 a.m. UTC | #3

On Mon, Dec 20, 2021 at 12:34 PM Derrick Stolee <[e-mail]> wrote:
> On 12/20/2021 11:21 AM, Eric Sunshine wrote:
> > Thanks for jumping on this so quickly. Unfortunately, however, as
> > mentioned in [1] and [2], I think the approach implemented here of
> > setting `core.bare=false` in the worktree-specific config is
> > fundamentally flawed since it only addresses the problem for worktrees
> > in which `git sparse-checkout init` has been run, but leaves all other
> > worktrees potentially broken (both existing and new worktrees). As far
> > as I can see, the _only_ correct solution is for the new helper
> > function to enable `extensions.worktreeConfig` _and_ relocate
> > `core.bare` and `core.worktree` from .git/config to
> > .git/worktree.config, thus implementing the requirements documented in
> > git-worktree.txt.
>
> Thanks for clarifying what I had misread. I commented on Patch 3 at the
> place that should be changed to relocate the setting. The test in patch 4
> could have multiple worktrees to verify that it works.

I sent several pages worth of response to patch [3/4] because
(apparently) I don't know how to be laconic.

> I'll plan on providing a v2 with that change tomorrow, leaving time to
> find any other glaring errors.

Let's make sure we agree on the proper approach and solution before
firing off v2.

> > I also raised a separate question in [2] about whether `git
> > sparse-checkout init` or the new helper function should be warning the
> > user that upgrading the repository format and setting
> > `extensions.worktreeConfig` might break third-party tools. However,
> > that question is tangential to the fix being addressed here and
> > doesn't need to be addressed by this series.
>
> Let's continue to simmer on this one. If there is a clear direction for
> doing this (should it just be an advice message?) then we can do that
> whenever.

Indeed, no hurry on this one. It's entirely tangential to the present
patch series, and requires discussion and thought; it can be tackled
later (if at all).