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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

Jonathan