On Fri, Jun 23, 2017 at 09:01:40AM +0200, Michael Haggerty wrote:
> @@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int
> flags)
> timeout_configured = 1;
> }
>
> + /*
> + * Note that we close the lockfile immediately because we
> + * don't write new content to it, but rather to a separate
> + * tempfile.
> + */
> if (hold_lock_file_for_update_timeout(
> &refs->lock,
> refs->path,
> - flags, timeout_value) < 0)
> + flags, timeout_value) < 0 ||
> + close_lock_file(&refs->lock))
> return -1;
I was wondering whether we'd catch a case which accidentally wrote to
the lockfile (instead of the new tempfile, but this close() is a good
safety check).
> - if (commit_lock_file(&refs->lock)) {
> - strbuf_addf(err, "error overwriting %s: %s",
> + if (rename_tempfile(&refs->tempfile, refs->path)) {
> + strbuf_addf(err, "error replacing %s: %s",
> refs->path, strerror(errno));
> goto out;
> }
So our idea of committing now is the tempfile rename, and then...
> @@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store,
> struct strbuf *err)
> goto out;
>
> error:
> - rollback_lock_file(&refs->lock);
> + delete_tempfile(&refs->tempfile);
>
> 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.
So this all looks good to me.
-Peff
