Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

Good points.



On Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

Let ref_transaction_commit return an optional error string that describes
the transaction failure.  Start by returning the same error as update_ref_lock
returns, modulo the initial error:/fatal: preamble.

s/returns/prints/?

Done, and then was deleted when I reworded the message.

quoted

This will make it easier for callers to craft better error messages when
a transaction call fails.

Interesting.  Can you give an example?  What kind of behavior are we
expecting in callers other than die()-ing or cleaning up and then
die()-ing?

I was thinking a bit too far ahead. You could in theory keep logging multiple
lock failures during the _commit() and then when the transaction fails
and returns
it will have appended a list of all refs that failed and not just the
first ref that failed.

I like this more than having the caller pass in a flag/callback/etc to
decide how noisy to be and whether to gracefully accept errors or exit.
So it seems like an improvement, but may always returning error()
would be better --- more context would help in clarifying this.

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 error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-                        const char *msg, enum action_on_err onerr);
+                        const char *msg, char **err,
+                        enum action_on_err onerr);

Is the idea that if I pass in a pointer &err then
ref_transaction_commit will take the action described by onerr *and*
write its error message to err?

Temporarily, yes.
Shortly after this patch I remove the onerr argument completely.
But I want to keep the "pass error back to caller" and "get rid of
onerr" as two separate patches.
I think it is easier to follow the flow of changes if they are done in
two separate steps.

Probably squashing with patch 07 would make this easier to read (and
wouldn't require changing any messages at that point).

See above.

[...]

quoted

--- a/refs.c
+++ b/refs.c

[...]

quoted

@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
                                             update->flags,
                                             &update->type, onerr);
              if (!update->lock) {
+                     if (err) {
+                             const char *str = "Cannot lock the ref '%s'.";
+                             *err = xmalloc(PATH_MAX + 24);
+                             snprintf(*err, PATH_MAX + 24, str,
+                                      update->refname);
+                     }

Might be clearer to use a helper similar to path.c::mkpathdup

        char *aprintf(const char *fmt, ...)
        {
                char *result;
                struct strbuf sb = STRBUF_INIT;
                va_list args;

                va_start(args, fmt);
                strbuf_vaddf(&sb, fmt, args);
                va_end(args);

                return strbuf_detach(&sb);
        }

or to have the caller pass in a pointer to strbuf instead of char *.

strbuf as argument is probably the right thing to do. I am doing that change.

The rest looks good to me.

Thanks,
Jonathan