Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
On Tue, May 27, 2014 at 5:42 PM, Jonathan Nieder jrnie...@gmail.com wrote:
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.
Thanks. Updated the comment in refs.h
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
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().
*/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
On Wed, May 28, 2014 at 10:07 AM, Jonathan Nieder jrnie...@gmail.com 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: Please re-review. 06df8942 is Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
Thanks Can you re-review these patches for me : please review 67b8fce refs.c: add an err argument to repack_without_refs 738ac43 refs.c: add an err argument to delete_ref_loose b78b0e0 refs.c: update ref_transaction_delete to check for error and return status e558f96 refs.c: add transaction.status and track OPEN/CLOSED/ERROR 5a7d9bf sequencer.c: use ref transactions for all ref updates 0ea69df fast-import.c: change update_branch to use ref transactions 57c92fb refs.c: change update_ref to use a transaction On Wed, May 28, 2014 at 10:25 AM, Jonathan Nieder jrnie...@gmail.com wrote: Ronnie Sahlberg wrote: Please re-review. 06df8942 is Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: Can you re-review these patches for me : please review 67b8fce refs.c: add an err argument to repack_without_refs 738ac43 refs.c: add an err argument to delete_ref_loose b78b0e0 refs.c: update ref_transaction_delete to check for error and return status e558f96 refs.c: add transaction.status and track OPEN/CLOSED/ERROR 5a7d9bf sequencer.c: use ref transactions for all ref updates 0ea69df fast-import.c: change update_branch to use ref transactions 57c92fb refs.c: change update_ref to use a transaction The easiest way for me is if you send them to the list so I can comment inline (and others can chime in with other comments, etc). This works like a reroll of an entire series (using format-patch -v if you use format-patch, etc) except that instead of sending as a new thread the updated patch is just sent in response to some review comments or the earlier version of that patch. git send-email has an --in-reply-to option for this. That involves getting the Message-Id, which can be a little fussy. Others who use send-email might know better tricks for this. I never ended up learning how to use git send-email (and I'm a little scared of it) so I just hit reply in mutt and put the patch there. Thanks and sorry for the fuss, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
