From: Jonathan Nieder <jrnieder@gmail.com>
Date: Fri, 25 Apr 2014 22:36:20 GMT
Subject: Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message
Message-ID: <20140425223620.GD9218@google.com>
In-Reply-To: <1398442494-23438-6-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

> Call ref_transaction_commit with QUIET_ON_ERR and use the error string
> that is returned to print a better log message if/after the transaction
> fails.

Ah, so that's how the transition to a better API happens.  Makes sense.

(A mention of QUIET_ON_ERR in the patch that introduces the &err
parameter could help, or feel free to ignore these comments, since it's
all well by the end of the series.)

> Update the tests to reflect that the log message is now slightly different
>   fatal: update_ref failed: Cannot lock the ref 'some ref'
> versus from the previous
>   fatal: Cannot lock the ref 'some ref'

Makes sense as a demo of what the new code allows, but is this error
message better?  The use of 'git update-ref' is an implementation
detail that the user doesn't need to know about.

I think I'd prefer the result of plain die("%s", err), even though
that's a no-op.

[...]
> +++ b/builtin/update-ref.c
[...]
> @@ -359,17 +360,18 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>  		die("Refusing to perform update with empty message.");
>  
>  	if (read_stdin) {
> -		int ret;
>  		transaction = ref_transaction_begin();
> -
> +		if (!transaction)
> +			die("failed to update refs");

This can't happen (xcalloc is defined to die() on malloc failure).
If you want to protect against it anyway, die("BUG: ...")?

Thanks,
Jonathan