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

[Jeff King <peff@peff.net>]

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

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