Re: [PATCH v4 0/7] add/rm: honor sparse checkout and warn on sparse paths

[Elijah Newren <newren@gmail.com>]

On Thu, Apr 8, 2021 at 1:41 PM Matheus Tavares
<matheus.bernardino@usp.br> 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@mail.gmail.com/

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?