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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 28, 2014, 17:07 UTC
Message-ID: <20140528170700.GM12314@google.com>
In-Reply-To: <CAL=YDWkQhq2oCkyBG0-ojUDwgApYj1qZt1vXa2gnYsJOEbnvxQ@mail.gmail.com>

Ronnie Sahlberg wrote:

> Updated the comment in refs.h

Thanks.

Show 17 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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