Paul Tan <pyoka...@gmail.com> writes: > 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. > 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.
