Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder jrnie...@gmail.com wrote: Ronnie Sahlberg wrote: Do basic error checking in ref_transaction_create() and make it return status. 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. Same concerns as with _update: Done. [...] +++ b/builtin/update-ref.c @@ -225,7 +225,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)) + die(failed transaction create for %s, refname); If it were ever triggered, the message error: some bad thing fatal: failed transaction create for refs/heads/master looks overly verbose and unclear. Something like fatal: cannot create ref refs/heads/master: some bad thing I changed it to : die(cannot create ref '%s', refname); But it would still mean you would have error: some bad thing fatal: cannot create 'refs/heads/master' To make it better we have to wait until the end of the second patch series, ref-transactions-next where we will have an err argument to _update/_create/_delete too and thus we can do this from update-ref.c : if (transaction_create_sha1(transaction, refname, new_sha1, update_flags, msg, err)) die(%s, err.buf); might work better. It's hard to tell without an example in mind. [...] @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction, [...] - assert(!is_null_sha1(new_sha1)); + if (!new_sha1 || is_null_sha1(new_sha1)) + die(create ref with null new_sha1); One less 'assert' is nice. :) As with _update, the message should start with BUG: to make it clear to users and translators that this should never happen, even with malformed user input. That gets corrected in patch 28 but it's clearer to include it from the start. -- 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 v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder jrnie...@gmail.com wrote: If it were ever triggered, the message error: some bad thing fatal: failed transaction create for refs/heads/master looks overly verbose and unclear. Something like fatal: cannot create ref refs/heads/master: some bad thing I changed it to : die(cannot create ref '%s', refname); But it would still mean you would have error: some bad thing fatal: cannot create 'refs/heads/master' To make it better we have to wait until the end of the second patch series, ref-transactions-next where we will have an err argument to _update/_create/_delete too and thus we can do this from update-ref.c : if (transaction_create_sha1(transaction, refname, new_sha1, update_flags, msg, err)) die(%s, err.buf); Thanks. Sounds good. -- 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 v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
Ronnie Sahlberg wrote: Do basic error checking in ref_transaction_create() and make it return status. 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. Same concerns as with _update: [...] +++ b/builtin/update-ref.c @@ -225,7 +225,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)) + die(failed transaction create for %s, refname); If it were ever triggered, the message error: some bad thing fatal: failed transaction create for refs/heads/master looks overly verbose and unclear. Something like fatal: cannot create ref refs/heads/master: some bad thing might work better. It's hard to tell without an example in mind. [...] @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction, [...] - assert(!is_null_sha1(new_sha1)); + if (!new_sha1 || is_null_sha1(new_sha1)) + die(create ref with null new_sha1); One less 'assert' is nice. :) As with _update, the message should start with BUG: to make it clear to users and translators that this should never happen, even with malformed user input. That gets corrected in patch 28 but it's clearer to include it from the start. -- 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 v6 09/42] 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
status. 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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 4 +++-
refs.c | 17 +++--
refs.h | 8
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 59c4d6b..3fab810 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -225,7 +225,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))
+ die(failed transaction create for %s, refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 1a903fb..27db737 100644
--- a/refs.c
+++ b/refs.c
@@ -3353,18 +3353,23 @@ 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 ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die(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 0364a3e..01d773c 100644
--- a/refs.h
+++ b/refs.h
@@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* null SHA-1. It is verified that the reference does not exist
* already.
*/
-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);
/*
* Add a reference deletion to transaction. If have_old is true, then
--
2.0.0.rc1.351.g4d2c8e4
--
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
