Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:01:09
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: [...]
quoted hunk ↗ jump to hunk
+++ 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. [...]
quoted hunk ↗ jump to hunk
@@ -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.