On 12/29/2021 4:39 AM, Elijah Newren wrote:
On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
quoted
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.
quoted
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.
...
quoted
+ 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.
quoted
+ 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/ (local))
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