Re: [PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index

From: Nipunn Koorapati <nipunn1313@gmail.com>
Date: Mar 18, 2021, 21:36 UTC
Message-ID: <CAN8Z4-VQHJVx6+-Lc4jF7NRwLxS6+9wW03FiZ2Ydqn0Mi30dKg@mail.gmail.com>
In-Reply-To: <xmqqo8fgry3g.fsf@gitster.g>

On Thu, Mar 18, 2021 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> >
> > +     refresh_fsmonitor(istate);
>
> And the VALID bit is set only for the ones that are untouched?  When
> core_fsmonitor is not set, or istate->fsmonitor_has_run_once is set,
> refresh_fsmonitor() becomes no-op and does not even drop the VALID
> bit from the cache entries.  As run_diff_index() is rather
> library-ish part of the system, are we sure no earlier attempts to
> invoke fsmonitor have touched ce to set the VALID bit on at this
> point?
>
> Assuming that we won't see stray VALID bit to confuse us, the patch
> looks good to me, but I am not sure what to base confidence on that
> assumption.
>
> Thanks.

My understanding is that git's invariants around fsmonitor bit would ensure that the bit is set simultaneously w/ the rest of the index entry after a stat (at the cursor/timestamp of the most recent refresh_fsmonitor).

Regardless of whether earlier access to the VALID bit happened within the command, the state should be internally consistent at this point, meaning that if valid is set, the rest of the entry is sensible.

I did just manually confirm this here $ bin-wrappers/git diff HEAD $ rm zlib.c $ GIT_TRACE_FSMONITOR=1 bin-wrappers/git diff HEAD 21:21:22.911316 fsmonitor.c:97 read fsmonitor extension successful 'c:1615751750:32210:5:60' 21:21:22.911417 fsmonitor.c:249 refresh fsmonitor 21:21:22.937726 fsmonitor.c:301 fsmonitor process '.git/hooks/query-watchman' returned success 21:21:22.937749 fsmonitor.c:228 fsmonitor_refresh_callback 'zlib.c' 21:21:22.937755 fsmonitor.c:228 fsmonitor_refresh_callback '.git'

diff --git a/zlib.c b/zlib.c
deleted file mode 100644
[rest of output omitted]

This should be confirmable in the testsuite via
ls t*.sh | grep diff | GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all xargs prove

Perhaps dscho might be amenable to adding this variant to
https://github.com/gitgitgadget/git/blob/master/.github/workflows/main.yml
in the future - so fsmonitor tests run automatically on diffs there.

--Nipunn