Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

From: Ronnie Sahlberg <sahlberg@google.com>
Date: May 15, 2014, 16:23 UTC
Message-ID: <CAL=YDWn5+_jA55KGx74We=41ngxSDA6yKJbuZ=3zy8gZTYPYnQ@mail.gmail.com>
In-Reply-To: <20140515000409.GF9218@google.com>

On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 8 quoted lines

> 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.

Show 20 quoted lines

>
> [...]
>> +++ 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);

Show 16 quoted lines

>
> 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.