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

On 06/23/2017 09:46 PM, Jeff King wrote:

On Fri, Jun 23, 2017 at 09:01:40AM +0200, Michael Haggerty wrote:

quoted

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

-	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

@@ -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.

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.

Michael