Re: [PATCH 09/11] read-cache: require flags for `write_locked_index()`

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 06:00 UTC
Message-ID: <20171002060018.m5dvxt2qtgkeixc7@sigill.intra.peff.net>
In-Reply-To: <d912e33a1395ff25c1496715d0a537858daa885a.1506862824.git.martin.agren@gmail.com>

On Sun, Oct 01, 2017 at 04:56:10PM +0200, Martin Ågren wrote:

Show 12 quoted lines

> `write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.
> At most one is allowed. But it is also possible to use no flag, i.e.,
> `0`. But when `write_locked_index()` calls `do_write_index()`, the
> temporary file, a.k.a. the lockfile, will be closed. So passing `0` is
> effectively the same as `CLOSE_LOCK`, which seems like a bug.
> 
> We might feel tempted to restructure the code in order to close the file
> later, or conditionally. It also feels a bit unfortunate that we simply
> "happen" to close the lock by way of an implementation detail of
> lockfiles. But note that we need to close the temporary file before
> `stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close
> index.lock in do_write_index, 2017-04-26).

Interesting. So it seems like this is potentially a bug introduced by 9f41c7a6b, as before then passing "0" for the flags would neither commit nor close the lockfile.

At the same time, I cannot imagine why one would want that behavior. The only use would be if you planned to append more to the index. Which seems kind of odd to do outside of write_locked_index() itself. And certainly there aren't any callers in the existing codebase who pass 0. Your assert() should help catch any that are added.

-Peff