On Wed, Apr 21, 2021 at 1:11 PM Elijah Newren <newren@xxxxxxxxx> wrote: > > // Adding Matheus to cc due to the ignore_skip_worktree bit, given his > experience and expertise with the checkout and unpack-trees code. > > On Wed, Apr 21, 2021 at 6:41 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: > > > > On 4/20/2021 7:00 PM, Elijah Newren wrote: > > > On Tue, Apr 13, 2021 at 7:01 AM Derrick Stolee via GitGitGadget > > > <gitgitgadget@xxxxxxxxx> wrote: > > >> > > >> diff --git a/read-cache.c b/read-cache.c > > >> index 29ffa9ac5db9..6308234b4838 100644 > > >> --- a/read-cache.c > > >> +++ b/read-cache.c > > >> @@ -1594,6 +1594,9 @@ int refresh_index(struct index_state *istate, unsigned int flags, > > >> if (ignore_skip_worktree && ce_skip_worktree(ce)) > > >> continue; > > >> > > >> + if (istate->sparse_index && S_ISSPARSEDIR(ce->ce_mode)) > > >> + continue; > > >> + > > > > > > I'm a bit confused about what could trigger ce_skip_worktree(ce) && > > > !ignore_skip_worktree and why it'd be desirable to refresh > > > skip-worktree entries. The skip-worktree entries are not really refreshed in refresh_index(), even when !ignore_skip_worktree (which is the default case; i.e. without the REFRESH_IGNORE_SKIP_WORKTREE flag). This flag (which is currently only used by `git add --refresh`s code at `builtin/add.c:refresh()`), just makes refresh_index() skip the following operations on skip-worktree entries: pathspec matching, marking the matches on `seen`, checking/warning if unmerged, and marking the entry as up-to-date (i.e. with the in-memory CE_UPTODATE bit). I added this flag in mt/add-rm-in-sparse-checkout and changed `builtin/add.c:refresh()` to use it mainly because we needed a `seen` array with only matches from non-skip-worktree entries so that we could later decide when to emit the warning. (In fact, the original implementation of the flag only controlled whether sparse matches would be marked on `seen` or not [1]) [1]: https://lore.kernel.org/git/d65b214dd1d83a2e8710a9bbf98477c1929f0d5e.1614138107.git.matheus.bernardino@xxxxxx/ Perhaps we could alternatively make refresh_index() skip the previously mentioned operations on all skip-worktrees entries *unconditionally*. I.e. having, early in the loop: if (ce_skip_worktree(ce)) continue; But I'm not familiar enough with CE_UPTODATE and how it's used in different parts of the code base, so I didn't want to risk introducing any bugs at refresh_index() callers that might want/expect the function to set the CE_UPTODATE bit on the skip-worktree entries. The case of `git add --refresh` was much narrower and easier to analyze, and that's what we were interested in for the warning. That's why I only changed the behavior there :) > > > However, this is tangential to your patch and > > > has apparently been around since 2009 (in particular, from 56cac48c35 > > > ("ie_match_stat(): do not ignore skip-worktree bit with > > > CE_MATCH_IGNORE_VALID", 2009-12-14)). Note that the `CE_MATCH_IGNORE_SKIP_WORKTREE` added in this patch does control if refresh_cache_ent() will refresh skip-worktree entries, but refresh_index() allways calls this function *without* this flag.
