Re: [PATCH v3 5/7] refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Mar 30, 2021, 18:51 UTC
Message-ID: <CAHd-oW4kRLjV9Sq3CFt-V1Ot9pYFzJggU1zPp3Hcuw=qWfq7Mg@mail.gmail.com>
In-Reply-To: <xmqqlfajnn6w.fsf@gitster.g>

On Fri, Mar 19, 2021 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

>
> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:
>
> >> In other words, the change makes me wonder why we are not adding a
> >> flag that says "do we or do we not want to match paths outside the
> >> sparse checkout cone?", with which the seen[] would automatically
> >> record the right thing.
> >
> > Yeah, makes sense. I didn't want to make the flag skip the sparse
> > paths unconditionally (i.e. without matching them) because then we
> > would also skip the ce_stage() checkings below and the later
> > ce_mark_uptodate(). And I wasn't sure whether this could cause any
> > unwanted side effects.
> >
> > But thinking more carefully about this now, unmerged paths should
> > never have the SKIP_WORKTREE bit set anyway, right? What about the
> > CE_UPTODATE mark, would it be safe to skip it? I'm not very familiar
> > with this code, but I'll try to investigate more later.

Sorry I haven't given any update on this yet. From what I could see so far, it seems OK to ignore the skip_worktree entries in refresh_index() when it is called from `git add --refresh`. But because we would no longer mark the skip_worktree entries that match the pathspec with CE_UPTODATE, do_write_index() would start checking if they are racy clean (this is only done when `!ce_uptodate(ce)`), which could add some lstat() overhead.

However, this made me think what happens today if we do have a racy clean entry with the skip_worktree bit set... `git add --refresh` will probably update the index without noticing that the entry is racy clean (because it won't check CE_UPTODATE entries, and skip_worktree entries will have this bit set in refresh_index()). Thus the entries' size won't be truncated to zero when writing the index, and the entry will appear unchanged even if we later unset the skip_worktree bit.

But can we have a "racy clean skip_worktree entry"? Yes, this seems possible e.g. if the following sequence happens fast enough for mtime to be the same before and after the update:

  echo x >file
  git update-index --refresh --skip-worktree file
  echo y>file

Here is a more complete example which artificially creates a "racy clean skip_worktree entry", runs `git add --refresh`, and shows that the racy clean entry was not detected:

# Setup echo sparse >sparse echo dense >dense git add . git commit -m files

# Emulate a racy clean situation touch -d yesterday date touch -r date sparse git update-index --refresh --skip-worktree sparse touch -r date .git/index echo xparse >sparse touch -r date sparse

# `git add` will now write a new index without checking if # `sparse` is racy clean nor truncating its size touch -r date dense git add --refresh .

git update-index --no-skip-worktree sparse git status <doesn't show that `sparse` was modified>

This situation feels rather uncommon, but perhaps the same could happen with `git sparse-checkout set` instead of `git update-index --refresh --skip-worktree`? IDK. This made me think whether refresh_index() should really mark skip_worktree entries with CE_UPTODATE, in the first place.

Any thoughts?