[PATCH v20 20/48] 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.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 4 +++-
refs.c | 18 +++--
refs.h | 55 +---
3 files changed, 63 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 3f05e88..c49f1c6 100644
--- a/refs.c
+++ b/refs.c
@@ -3449,18 +3449,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 c5376ce..33b4383 100644
--- a/refs.h
+++ b/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;
/*
@@ -248,7 +287,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,
@@ -262,11 +301,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,
- con
Re: [PATCH v20 20/48] refs.c: change ref_transaction_create to do error checking and return status
I'm in my next attempt to get through your patch series. Sorry for the
long hiatus.
Patches 1-19 look OK aside from a minor typo that I just reported.
See below for a comment on this patch.
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> 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.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> builtin/update-ref.c | 4 +++-
> refs.c | 18 +++--
> refs.h | 55
> +---
> 3 files changed, 63 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 3f05e88..c49f1c6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3449,18 +3449,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 c5376ce..33b4383 100644
> --- a/refs.h
> +++ b/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;
> + * }
> + */
I don't have a problem with the API, but I think the idiom suggested in
the comment above is a bit silly. Surely one would do the following
instead:
if (ref_transaction_update(..., &err)) {
ret = error("Error while doing foo-bar: %s", err.buf);
goto cleanup;
}
I think it would also be helpful to document whether the error string
that is appended to the strbuf is terminated with a LF.
> [...]
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.k
Re: [PATCH v20 20/48] refs.c: change ref_transaction_create to do error checking and return status
I have changed the comment. Thanks.
On Tue, Jul 8, 2014 at 4:48 AM, Michael Haggerty wrote:
> I'm in my next attempt to get through your patch series. Sorry for the
> long hiatus.
>
> Patches 1-19 look OK aside from a minor typo that I just reported.
>
> See below for a comment on this patch.
>
> On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
>> 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.
>>
>> Reviewed-by: Jonathan Nieder
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> builtin/update-ref.c | 4 +++-
>> refs.c | 18 +++--
>> refs.h | 55
>> +---
>> 3 files changed, 63 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 3f05e88..c49f1c6 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3449,18 +3449,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 c5376ce..33b4383 100644
>> --- a/refs.h
>> +++ b/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;
>> + * }
>> + */
>
> I don't have a problem with the API, but I think the idiom suggested in
> the comment above is a bit silly. Surely one would do the following
> instead:
>
> if (ref_transaction_update(..., &err)) {
> ret = error("Error while doing foo-bar: %s", err.buf);
> goto cleanup;
> }
>
> I think it would also be helpful to document whether the error string
>
