From: Jonathan Nieder <jrnieder@gmail.com>
Date: Fri, 25 Apr 2014 22:10:01 GMT
Subject: Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string
Message-ID: <20140425221001.GB9218@google.com>
In-Reply-To: <1398442494-23438-4-git-send-email-sahlberg@google.com>

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