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.


> 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

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);

                return strbuf_detach(&sb);

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

The rest looks good to me.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to