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

[Jeff King <peff@peff.net>]

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