On Wed, May 28, 2014 at 10:07 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Ronnie Sahlberg wrote:
>
>> Updated the comment in refs.h
>
> Thanks.
>
>> +++ b/refs.h
>> @@ -215,6 +215,31 @@ enum action_on_err {
>> };
>>
>> /*
>> + * 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;
>> + * }
>> + *
>> + * The message is appended to err without first clearing err.
>> + * This allow shte caller to prepare preamble text to the generated
>
> s/allow shte/allows the/
>
>> + * error message:
>> + *
>> + * strbuf_addf(&err, "Error while doing foo-bar: ");
>> + * if (ref_transaction_update(..., &err)) {
>> + * ret = error("%s", err.buf);
>> + * goto cleanup;
>> + * }
>
> Nice.
>
>> + *
>> + * If the caller wants err to only contain the message for the current error
>> + * and nothing else caller will need to guarantee that err is empty or
>> + * call strbuf_reset before calling the transaction function.
>
> I don't think this paragraph is needed --- especially with the
> clarification about how to add a preamble, the contract is clear.
>
>> + */
>> +/*
>> * Begin a reference transaction. The reference transaction must
>> * be freed by calling ref_transaction_free().
>> */
>
> Now that the comment is longer, it's harder to miss, but it still is
> in an odd place for someone looking to understand what the 'err'
> argument to ref_transaction_update() means.
>
> How about this patch for squashing in?
>
> diff --git i/refs.h w/refs.h
> index c741a6c..913ca93 100644
> --- i/refs.h
> +++ w/refs.h
> @@ -10,6 +10,45 @@ struct ref_lock {
> int force_write;
> };
>
> +/*
> + * A ref_transaction represents a collection of ref updates
> + * that should succeed or fail together.
> + *
> + * Calling sequence
> + * ----------------
> + * - Allocate and initialize a `struct ref_transaction` by calling
> + * `ref_transaction_begin()`.
> + *
> + * - List intended ref updates by calling functions like
> + * `ref_transaction_update()` and `ref_transaction_create()`.
> + *
> + * - Call `ref_transaction_commit()` to execute the transaction.
> + * If this succeeds, the ref updates will have taken place and
> + * the transaction cannot be rolled back.
> + *
> + * - At any time call `ref_transaction_free()` to discard the
> + * transaction and free associated resources. In particular,
> + * this rolls back the transaction if it has not been
> + * successfully committed.
> + *
> + * Error handling
> + * --------------
> + *
> + * 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.
> + *
> + * The message is appended to err without first clearing err.
> + * This allows the caller to prepare preamble text to the generated
> + * error message:
> + *
> + * strbuf_addf(&err, "Error while doing foo-bar: ");
> + * if (ref_transaction_update(..., &err)) {
> + * ret = error("%s", err.buf);
> + * goto cleanup;
> + * }
> + */
> struct ref_transaction;
>
> /*
> @@ -215,31 +254,6 @@ enum action_on_err {
> };
>
> /*
> - * 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;
> - * }
> - *
> - * The message is appended to err without first clearing err.
> - * This allow shte caller to prepare preamble text to the generated
> - * error message:
> - *
> - * strbuf_addf(&err, "Error while doing foo-bar: ");
> - * if (ref_transaction_update(..., &err)) {
> - * ret = error("%s", err.buf);
> - * goto cleanup;
> - * }
> - *
> - * If the caller wants err to only contain the message for the current error
> - * and nothing else caller will need to guarantee that err is empty or
> - * call strbuf_reset before calling the transaction function.
> - */
> -/*
> * Begin a reference transaction. The reference transaction must
> * be freed by calling ref_transaction_free().
> */
Done.
Please re-review.
--
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