Re: [PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

From: Nipunn Koorapati <nipunn1313@gmail.com>
Date: Mar 18, 2021, 22:57 UTC
Message-ID: <CAN8Z4-Ug4i-zBrz0xSRXZUCM5+50Cg46m6Ap5rsJWHu+=Nr6eQ@mail.gmail.com>
In-Reply-To: <xmqqk0q4rxxh.fsf@gitster.g>

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

Show 8 quoted lines

> Isn't this the other way around, wrt to the previous step?
>
> At least, "pass around istate throughout the callchain in the
> diff-lib.c file" change should stand alone and come much earlier in
> the series (perhaps as step #1).  Then "call refresh_fsmonitor from
> run_diff_index() and make sure in check_removed() that fsmonitor
> does not have bogus VALID bit" assertion should come on top, as a
> single step, I would think.

Just to make sure I understand - it sounds like you're recommending I split this diff up into two parts - one which passes istate through the callstack, and a second which provides this assertion. It also sounds like you're recommending reordering the series to

1 - pass istate around callstack 2 - add is_fsmonitor_refreshed and use it to assert fsmonitor is refreshed in check_removed in prep for usage 3 - use fsmonitor bit to save a call to lstat 4 - Add perf benchmark test

This makes sense to me - and I can execute the factoring in the next patch series iteration

--Nipunn