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
[PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 25 ++---
3 files changed, 33 insertions(+), 14 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..41121fa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(create %s: extra input: %s, refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 79a4477..709758d 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,18 +3397,24 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die(BUG: create ref with null new_sha1);
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update-new_sha1, new_sha1);
hashclr(update-old_sha1);
update-flags = flags;
update-have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 32edf3f..22b8cc3 100644
--- 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.
+ */
+/*
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
*/
@@ -236,7 +245,7 @@ struct ref_transaction *ref_transaction_begin(void);
* it must not have existed beforehand.
* Function returns 0 on success and non-zero on failure. A failure to update
* means that the transaction as a whole has failed and will need to be
- * rolled back. On failure the err buffer will be updated.
+ * rolled back.
*/
int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
@@ -250,11 +259,15 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* that the reference should have after the update; it must not be the
* null SHA-1. It is verified that the reference does not exist
* already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err);
/*
* Add a reference deletion to transaction. If have_old is true, then
@@ -270,8 +283,6 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, struct strbuf *err);
--
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
