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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Junio C Hamano <gitster@pobox.com>
Cc: git <git@vger.kernel.org>, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v3 5/7] refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag
Date: Tue, 30 Mar 2021 15:51:17 -0300	[thread overview]
Message-ID: <CAHd-oW4kRLjV9Sq3CFt-V1Ot9pYFzJggU1zPp3Hcuw=qWfq7Mg@mail.gmail.com> (raw)
In-Reply-To: <xmqqlfajnn6w.fsf@gitster.g>

On Fri, Mar 19, 2021 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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?