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

Ronnie Sahlberg wrote:

quoted

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.

[...]

quoted

+++ b/refs.c

[...]

quoted

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

[...]

quoted

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

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,