On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html