On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > This series continues the work to integrate commands with the sparse index, > adding integrations with 'git clean', 'git checkout-index', and 'git > update-index'. These three commands, while useful in their own right, are > updated mainly because they're used in 'git stash'. A future series will > integrate sparse index with 'stash' directly, but its subcommands must be > integrated to avoid the performance cost of each one expanding and > collapsing the index. Thanks for working on these; it's very nice to see others continuing to work on sparse-checkout issues, since it still feels somewhat incomplete to me. This work is perhaps less glamorous since you're starting to work on commands less frequently used by users, but I still feel it's very important. Thanks for your diligence. > The series is broken up into 4 parts: > > * Patches 1-2 are minor fixups to the 'git reset' sparse index integration > in response to discussion [1] that came after the series was ready for > merge to 'next'. These look good. > * Patch 3 integrates 'git clean' with the sparse index. Yeah, since clean tends to work with untracked files instead of tracked, and SKIP_WORKTREE is all about tracked files, I'd expect the conversion would be simple. Nice to see verification that it was. > * Patches 4-6 integrate 'git checkout-index' with the sparse index and > introduce a new '--ignore-skip-worktree-bits' option for use with 'git > checkout-index --all'. > * This involves changing the behavior of '--all' to respect > 'skip-worktree' by default (i.e., it won't check out 'skip-worktree' > files). The '--ignore-skip-worktree-bits' option can be specified to > force checkout of 'skip-worktree' files, if desired. I think the basic idea makes sense, and the patches are certainly moving in the right direction. If others agree with the series I'm about to submit, then patch 4 looks fine as-is and there's just a minor item about --ignore-skip-worktree-bits without --all for checkout-index in Patch 5 (either further behavior changes, or a more detailed commit message). If others disagree with that other series, then I think both patches 4 & 5 should do more work to avoid problems with present-despite-SKIP_WORKTREE files. However, the issues raised in Patch 7 suggest that Patch 5 might need some reworking (independent of the other series I'm about to submit). Patch 6 looks good either way. > * Patches 7-9 integrate 'git update-index' with the sparse index. > * Note that, although this integrates the sparse index with > '--cacheinfo', sparse directories still cannot be updated using that > option (see the prior discussion [2] for more details on why) Patch 9 might benefit from a small commit message rewording. I either misunderstood Patch 8, or it has a disconnect of some kind between the commit message and the testcase. I've left various comments on patch 7; it'll probably need updates and likely points to further changes needed in patch 5. Anyway, I tend to concentrate on perceived issues when reviewing rather than on what looks good, but despite the fact that I think there are some further improvements needed, overall I think this looks like some nice work. Keep it up!
