Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile

On Sat, Jun 24, 2017 at 01:43:09PM +0200, Michael Haggerty wrote:

quoted

quoted

 out:
+	rollback_lock_file(&refs->lock);

We always rollback the lockfile regardless, because committing it would
overwrite our new content with an empty file. There's no real safety
against somebody calling commit_lock_file() on it, but it also seems
like an uncommon error to make.

If this seems too dangerous, we could add a `LOCK_NO_COMMIT` flag for
`hold_lock_file_for_update()` and `hold_lock_file_for_update_timeout()`,
which would die() if somebody tries to commit the associated lockfile. I
think we can live without it. I hope that any such bugs would be caught
immediately by CI. But I admit that the prospect of renaming an empty
file on top of a `packed-refs` file is quite sobering, so if anybody is
worried about this, let me know and I'll implement it.

Yeah, that was the protection I was thinking of. Reflecting on it more,
though, it's not really any different than somebody calling
commit_lock_file() when we haven't correctly written out the whole
content. It's probably not worth adding code to protect against this
special case.

-Peff