On 9/20/2017 10:00 PM, Junio C Hamano wrote:
Ben Peart <peartben@xxxxxxxxx> writes:
Pretty much the same places you would also use CE_MATCH_IGNORE_VALID
and CE_MATCH_IGNORE_SKIP_WORKTREE which serve the same role for those
features. That is generally when you are about to overwrite data so
want to be *really* sure you have what you think you have.
Now that makes me worried gravely.
IGNORE_VALID is ignored in these places because we have been burned
by end-users lying to us. IGNORE_SKIP_WORKTREE must be ignored
because we know that the working tree state does not match the
"reality" the index wants to have. The fact that the code treats
the status reported and kept up to date by fsmonitor the same way as
these two implies that it is merely advisory and cannot be trusted?
Is that the reason why we tell the codepath with IGNORE_FSMONITOR to
ignore the state fsmonitor reported and check the state ourselves?
Sorry for causing unnecessary worry. The fsmonitor data can be trusted
(as much as you can trust that Watchman or your file system monitor is
not buggy). I wasn't 100% sure *why* these places passed the various
IGNORE_VALID and IGNORE_SKIP_WORKTREE flags. When I looked at them,
that lack of trust seemed to be the reason.
Adding IGNORE_FSMONITOR in those same places was simply an abundance of
caution on my part. The only down side of passing the flag for
fsmonitor is that we will end up calling lstat() on a file where we
technically didn't need too. That seemed safer than potentially missing
a change if I had misunderstood the code.
I'd much rather return correct results (and fall back to the old
performance) than potentially be incorrect. I followed that same
principal in the entire design of fsmonitor - if anything doesn't look
right, fall back to the old code path just in case...
Oh, wait...
The other place I used it was in preload_index(). In that case, I
didn't want to trigger the call to refresh_fsmonitor() as
preload_index() is about trying to do a fast precompute of state for
the bulk of the index entries but is not required for correctness as
refresh_cache_ent() will ensure any "missed" by preload_index() are
up-to-date if/when that is needed.
That is a very valid design decision. So IGNORE_FSMONITOR is,
unlike IGNORE_VALID and IGNORE_SKIP_WORKTREE, to tell us "do not
bother asking fsmonitor to refresh the state of this entry--it is OK
for us to use a slightly stale information"? That would make sense
as an optimization, but that does not mesh well with the previous
"we need to be really really sure" usecase. That one wants "we do
not trust fsmonitor, so do not bother asking to refresh; we will do
so ourselves", which would not help the "we can use slightly stale
one and that is OK" usecase.
Puzzled...
