Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2016, 18:01 UTC
Message-ID: <xmqqmvg6ti3x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACRoPnRpZr=E6SW81Vg-2TiOr=RJo1YouAt5iZoE0CNBx-qesg@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 9 quoted lines

> Hmm, to add on, looking at the three other call sites of this
> function, two of them (builtin/commit.c and builtin/describe.c)
> basically do:
>
>     if (0 <= fd)
>         update_index_if_able(...)
>
> with that 0 <= fd conditional. With this patch it becomes three out of
> four.

The other one is diff.c::refresh_index_quietly() that you are not counting, I think, but if you look at it again, it also is not called after hold_locked_index() fails to acquire the lock, so with this fix everybody refrains from calling it when it does not hold the lock.

> Perhaps the repeated use of this conditional is a sign that the
> 0 <= fd check could be built into update_index_if_able()?

That condition is "do we have the lock? Otherwise we are not even allowed to update it", so in that sense it may make sense.

Show 6 quoted lines

> I think there is precedent for building in these kind of checks --
> rollback_lock_file() also does not fail if the lock file was not
> successfully opened.
>
> That said, the number of call sites is quite low so it's probably not
> worth doing this.

Yeah, I can go either way. At least with the change things are consistent.