Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status

> 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.