On Tue, May 27, 2014 at 5:42 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Hi,
>
> Ronnie Sahlberg wrote:
>
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -215,6 +215,15 @@ enum action_on_err {
>> };
>>
>> /*
>> + * Transaction functions that take an err argument will append an error
>> + * string to this buffer if there was a failure.
>> + * This string is not cleared on each call and may contain an aggregate of
>> + * errors from several previous calls.
>> + * If the caller needs a guarantee that the buffer will only contain the
>> + * current or most recent error it must call strbuf_reset before calling
>> + * the transaction function.
>> + */
>> +/*
>
> If I look at the documentation for ref_transaction_create(), it is not
> obvious I should look up here for how it handles errors. Not sure how
> to fix that --- maybe this should go in a new
> Documentation/technical/api-ref-transactions.txt file? Or maybe the
> top of refs.h where struct ref_transaction is declared.
>
> The content seems odd to me. It says the string will contain an
> aggregate of errors from previous calls, but what it doesn't say is
> that that aggregate is a bunch of error messages juxtaposed without a
> newline or space between. Is the idea that some callers will want
> this aggregate?
>
> Wouldn't it be clearer to say how the err argument is meant to be used
> from the caller's perspective? E.g.,
>
> On error, transaction functions append a message about what
> went wrong to the 'err' argument. The message mentions what
> ref was being updated (if any) when the error occurred so it
> can be passed to 'die' or 'error' as-is:
>
> if (ref_transaction_update(..., &err)) {
> ret = error("%s", err.buf);
> goto cleanup;
> }
>
> If it's expected that the err argument will be used to aggregate
> multiple messages in some callers then it would be useful to give an
> example of that, too, but I don't think that's needed.
Thanks. Updated the comment in refs.h
>
> 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