On Thu, Apr 8, 2021 at 1:41 PM Matheus Tavares <matheus.bernardino@xxxxxx> wrote: > > Make `rm` honor sparse checkouts, and make both `rm` and `add` warn > when asked to update sparse entries. > > There are two changes since v3: > > - `test_i18ncmp` and `test_i18ngrep` were replaced by `test_cmp` and > `grep` > > - The flag added in patch 5 now makes refresh_index() completely ignore > skip_worktree entries, instead of just suppressing their matches on > the seen[] array. The previous implementation was not necessarily > wrong but, as Junio pointed out, it was rather odd to keep matching > the entries if we no longer want to use the matches. > > As "side effects", the new version of the flag also makes > refresh_index() refrain from both: > > (1) checking and warning if skip_worktree entries matching the given > pathspec are unmerged. > > (2) marking skip_worktree entries matching the given pathspec with > CE_UPTODATE. > > The change (1) is actually interesting because `git add` doesn't > update skip_worktree entries, and thus, it doesn't make much sense to > warn if they are unmerged. Besides, we will already warn if the user > requests to update such entries, anyway. And finally, unmerged > entries should not have the skip_worktree bit set in the first place. > (`git merge` should clean this bit when writing the new index, and > neither `git sparse-checkout` nor `git update-index` allow to set the > bit on an unmerged entry.) > > Change (2) is perhaps not very beneficial, but it is also not harmful. > The only practical difference we get by not setting the CE_UPTODATE > flag in the skip_worktree entries is that, when writing a new index at > the end of `git add --refresh`, do_write_index() will start checking > if these entries are racy clean. Note that it already does that for > all the skip_worktree entries that do not match the user-given > pathspecs. And, in fact, this behavior distinction based on the > pathspec only happens with `--refresh`. Plain `git add` and other > options don't mark any skip_worktree entry with CE_UPTODATE > (regardless of the pathspecs) and thus, all these entries are checked > when writing the index. So `git add --refresh` will only do what the > other options already do. Sorry for the delay. These two changes sound good to me, and the range-diff looks reasonable. > (Additionally, as I mentioned in [1], there might actually be at least > one advantage in checking if the skip_worktree entries are racy clean. > But this is a very specific case, and it's probably a topic for a > another thread :) > > [1]: https://lore.kernel.org/git/CAHd-oW4kRLjV9Sq3CFt-V1Ot9pYFzJggU1zPp3Hcuw=qWfq7Mg@xxxxxxxxxxxxxx/ This I'm a bit surprised by. I thought the outcome there was that you didn't want to mark skip_worktree entries as CE_UPTODATE in order to force them to be stat'd in the future when someone clears the skip_worktree bit. (And that we'd expect the sparse-checkout add/disable command to be the likely one to handle that, though if users use `git update-index --no-skip-worktree PATH` then they'd also be responsible for any refreshing as well.) Am I misunderstanding?
