On Sat, Jun 24, 2017 at 01:43:09PM +0200, Michael Haggerty wrote: > >> 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
