On 12/29/2021 4:39 AM, Elijah Newren wrote: > On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxxxxx/) 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
