Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2023, 17:07 UTC
Message-ID: <xmqqfs3jpbg2.fsf@gitster.g>
In-Reply-To: <CAJiyOijMvqK184wFgoXFyX5kmURkX3k2OmuiBpArikj26iHpMA@mail.gmail.com>

Josip Sokcevic <sokcevic@google.com> writes:

> Yes, I agree we should optimize this in a follow up. One thing I'm not
> sure about is if we should try to construct `struct stat` using
> `cache_index`, or we should check for `CE_FSMONITOR_VALID` in a way
> that `stat` would no longer be needed for those code paths.

Good point.

It seems to be entirely doable, even though the stench from the abstraction layer violation may be horrible.

ie_match_stat(), which is called by match_stat_with_submodule(), does pay attention to CE_FSMONITOR_VALID bit, so none of the members of struct stat matters when the bit is set. But the bit is not set, all members that fill_stat_data() and ce_match_stat_basic() care about do matter. Other code that follows callers of check_removed() do care about at least .st_mode member, and I suspect that in the current code .st_mode is the only member that gets looked at.

So after all, I think your original "fix" was correct, but it took us some time to figure out why it was, which means we would want to explain it in the log message for developers who would want to touch the same area in the future.

Thanks.