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.
> --- 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).
[...]
> --- a/refs.c
> +++ b/refs.c
[...]
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html