Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile
flat view
From: Jeff King <hidden>
Date: 2017-06-23 19:46:17
On Fri, Jun 23, 2017 at 09:01:40AM +0200, Michael Haggerty wrote:
quoted hunk ↗ jump to hunk
@@ -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).
quoted hunk ↗ jump to hunk
- 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...
quoted hunk ↗ jump to hunk
@@ -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