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

From: Josip Sokcevic <sokcevic@google.com>
Date: Sep 14, 2023, 22:39 UTC
Message-ID: <CAJiyOiidpD5eNvxVV8rZP7Xej4_8bBfgTOG75T8PVsmvwya3hg@mail.gmail.com>
In-Reply-To: <xmqqfs3jpbg2.fsf@gitster.g>

On Tue, Sep 12, 2023 at 10:07 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> 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.

I finished testing this - after my original fix and after running all tests, I can confirm that `st_mode` of `struct stat` is indeed not consumed if CE_FSMONITOR_VALID is set. But, it's fragile and likely to cause problems in the future. Your approach of constructing `struct stat` based on `struct cache_entry` is the way to go.

I see you created a new set of patches in a separate thread, so I'll start those tests and report back there.

Thanks!

-- 
Josip Sokcevic