Good points.
On Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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/?
Done, and then was deleted when I reworded the message.
>
>> 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.
>
>> --- 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.
>
> [...]
>> --- 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 *.
strbuf as argument is probably the right thing to do. I am doing that change.
>
> 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