Subject: Re: [PATCH v6 03/42] refs.c: add a strbuf argument to ref_transaction_commit for error logging

From: Jonathan Nieder <[email-masked]>
Date: Tue, 13 May 2014 16:10:12 -0700

Ronnie Sahlberg wrote:

> Add a strbuf argument to _commit so that we can pass an error string back to
> the caller. So that we can do error logging from the caller instead of from
> _commit.
>
> Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
> and craft any log messages from the callers themselves and finally remove the
> onerr argument completely.

Very nice.

[...]
> +++ b/refs.c
[...]
> @@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  					       update->flags,
>  					       &update->type, onerr);
>  		if (!update->lock) {
> +			if (err)
> +				strbuf_addf(err ,"Cannot lock the ref '%s'.",
> +					    update->refname);

Tiny nit: whitespace.

[...]
> --- a/refs.h
> +++ b/refs.h
> @@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction *transaction,
>   * Commit all of the changes that have been queued in transaction, as
>   * atomically as possible.  Return a nonzero value if there is a
>   * problem.  The ref_transaction is freed by this function.
> + * If err is non-NULL we will add an error string to it to explain why
> + * the transaction failed.

Probably worth mentioning the error string doesn't end with a newline
so the caller knows how to use it.

With the whitespace fix and with or without the comment tweak,
Reviewed-by: Jonathan Nieder <[email-masked]>

diff --git i/refs.c w/refs.c
index 64e8feb..2ca3169 100644
--- i/refs.c
+++ w/refs.c
@@ -3445,7 +3445,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       &update->type, onerr);
 		if (!update->lock) {
 			if (err)
-				strbuf_addf(err ,"Cannot lock the ref '%s'.",
+				strbuf_addf(err, "Cannot lock the ref '%s'.",
 					    update->refname);
 			ret = 1;
 			goto cleanup;
diff --git i/refs.h w/refs.h
index ff87e14..d45212f 100644
--- i/refs.h
+++ w/refs.h
@@ -268,8 +268,8 @@ void ref_transaction_delete(struct ref_transaction *transaction,
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
  * problem.  The ref_transaction is freed by this function.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed.
+ * If err is non-NULL we will append an error string (with no trailing
+ * newline) to it to explain why the transaction failed.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   const char *msg, struct strbuf *err,