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

[Derrick Stolee <stolee@gmail.com>]

On 12/29/2021 4:39 AM, Elijah Newren wrote:
> On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This series is based on a merge of en/sparse-checkout-set,
>> en/worktree-chatty-to-stderr, and ds/sparse-checkout-malformed-pattern-fix.
> 
> I think you mean es/worktree-chatty-to-stderr (not 'en/')

Yes. Thanks.
 
>> 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.
>>
>>  * 'git sparse-checkout' will now modify the worktree config, if enabled. It
>>    will no longer auto-upgrade users into using worktree config.
> 
> This sounds dangerous to me.

> ...
>>      +    Let the sparse-checkout builtin use this helper instead of attempting to
>>      +    initialize the worktree config on its own. This changes behavior of 'git
>>      +    sparse-checkout set' in a few important ways:
>>
>>      -    The fix is to have this assignment into config.worktree be handled by
>>      -    the repo_config_set_worktree_gently() helper.
>>      +     1. Git will no longer upgrade the repository format and add the
>>      +        worktree config extension. The user should run 'git worktree
>>      +        init-worktree-config' to enable this feature.
>>      +
>>      +     2. If worktree config is disabled, then this command will set the
>>      +        core.sparseCheckout (and possibly core.sparseCheckoutCone and
>>      +        index.sparse) values in the common config file.
> 
> Yikes.
> 
>>      +     3. If the main worktree is bare, then this command will not put the
>>      +        worktree in a broken state.
>>      +
>>      +    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.
>>      +
>>      +    This new logic introduces a new user pattern that could lead to some
>>      +    confusion. Suppose a user has not upgraded to worktree config and
>>      +    follows these steps in order:
>>      +
>>      +     1. Enable sparse-checkout in a worktree.
>>      +
>>      +     2. Disable sparse-checkout in that worktree without deleting that
>>      +        worktree's sparse-checkout file.
>>      +
>>      +     3. Enable sparse-checkout in another worktree.
>>      +
>>      +    After these steps, the first worktree will have sparse-checkout enabled
>>      +    with whatever patterns exist. The worktree does not immediately have
>>      +    those patterns applied, but a variety of Git commands would apply the
>>      +    sparse-checkout patterns and update the worktree state to reflect those
>>      +    patterns. This situation is likely very rare and the workaround is to
> 
> No, it's not even rare, let alone very rare.  I'd actually call it
> common.  Since 'sparse-checkout disable' does not delete the
> sparse-checkout file, and we've encouraged folks to use the
> sparse-checkout command (or a wrapper thereof) instead of direct
> editing of the sparse-checkout file (and indeed, the sparse-checkout
> command will overwrite the sparse-checkout file which further
> discourages users from feeling they own it), having the file left
> around after disabling is the common case.  So, the only question is,
> how often do users disable and re-enable sparse-checkout, and
> potentially only do so in some of their worktrees?  At my $DAYJOB,
> that's actually quite common.  I got multiple reports quite soon after
> introducing our `sparsify` tool about users doing something like this;
> this is what led me to learn of the extensions.worktreeConfig, and why
> I pointed it out to you on your first submission of
> git-sparse-checkout[1]
> (https://lore.kernel.org/git/CABPp-BFcH5hQqujjmc88L3qGx3QAYZ_chH6PXQXyp13ipfV6hQ@mail.gmail.com/)

Thank you for these comments and the detailed descriptions of things
from your $DAYJOB. That's helpful context and I'm happy to switch back
to enabling the extension in the sparse-checkout builtin. I might need
to rearrange the code so there is an API in worktree.c instead of just
the subcommand in builtin/worktree.c, but that's pretty minor. I'll
keep Eric's earlier suggestion to have the upgrade be a separate call
from the repo_config_set_worktree_gently().

Thanks,
-Stolee