Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:00:50
Ronnie Sahlberg wrote:
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/?
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 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 hunk ↗ jump to hunk
--- 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? Probably squashing with patch 07 would make this easier to read (and wouldn't require changing any messages at that point). [...]
quoted hunk ↗ jump to hunk
--- a/refs.c +++ b/refs.c
[...]
quoted hunk ↗ jump to hunk
@@ -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 *.
The rest looks good to me.
Thanks,
Jonathan