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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html