Subject: Re: [PATCH v6 13/42] commit.c: use ref transactions for updates

From: Jonathan Nieder <[email-masked]>
Date: Wed, 14 May 2014 18:11:34 -0700

Ronnie Sahlberg wrote:

[...]
> +++ b/builtin/commit.c
> @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
[...]
> @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&author_ident);
>  	free_commit_extra_headers(extra);
>  
> -	ref_lock = lock_any_ref_for_update("HEAD",
> -					   !current_head
> -					   ? NULL
> -					   : current_head->object.sha1,
> -					   0, NULL);
> -	if (!ref_lock) {
> -		rollback_index_files();
> -		die(_("cannot lock HEAD ref"));
> -	}
> -
>  	nl = strchr(sb.buf, '\n');
>  	if (nl)
>  		strbuf_setlen(&sb, nl + 1 - sb.buf);
> 	else
> 		strbuf_addch(&sb, '\n');
>  	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>  	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>  
> -	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
> +	transaction = ref_transaction_begin();
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, "HEAD", sha1,
> +				   current_head ?
> +				   current_head->object.sha1 : NULL,
> +				   0, !!current_head) ||
> +	    ref_transaction_commit(transaction, sb.buf, &err)) {
>  		rollback_index_files();
> -		die(_("cannot update HEAD ref"));
> +		die(_("HEAD: cannot update ref: %s"), err.buf);

Same question about !transaction (it also applies to later patches but I
won't mention it any more).

The error message changed from

	fatal: cannot lock HEAD ref

to

	fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.

Does the message from ref_transaction_commit always say what ref
was being updated when it failed?  If so, it's tempting to just use
the message as-is:

	fatal: Cannot lock the ref 'HEAD'

If the caller should add to the message, it could say something about
the context --- e.g.,

	fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'

Looking at that,

	die("%s", err.buf)

seems simplest since even if "git commit" was being called in a loop,
it's already clear that git was trying to lock HEAD to advance it.

Thanks,
Jonathan