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

Ronnie Sahlberg wrote:

quoted

Updated the comment in refs.h

Thanks.

quoted

+++ 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/

quoted

+ * error message:
+ *
+ *     strbuf_addf(&err, "Error while doing foo-bar: ");
+ *     if (ref_transaction_update(..., &err)) {
+ *         ret = error("%s", err.buf);
+ *         goto cleanup;
+ *     }

Nice.

quoted

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

quoted

+ */
+/*
  * 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().
  */