Re: [PATCH v20 20/48] refs.c: change ref_transaction_create to do error checking and return status

[Ronnie Sahlberg <sahlberg@google.com>]

I have changed the comment.  Thanks.

On Tue, Jul 8, 2014 at 4:48 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> I'm in my next attempt to get through your patch series.  Sorry for the
> long hiatus.
>
> Patches 1-19 look OK aside from a minor typo that I just reported.
>
> See below for a comment on this patch.
>
> On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
>> Do basic error checking in ref_transaction_create() and make it return
>> non-zero on error. 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. Add an err argument that
>> will be updated on failure.
>>
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/update-ref.c |  4 +++-
>>  refs.c               | 18 +++++++++++------
>>  refs.h               | 55 +++++++++++++++++++++++++++++++++++++++++++++-------
>>  3 files changed, 63 insertions(+), 14 deletions(-)
>>
>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index 3067b11..41121fa 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -226,7 +226,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, &err))
>> +             die("%s", err.buf);
>>
>>       update_flags = 0;
>>       free(refname);
>> diff --git a/refs.c b/refs.c
>> index 3f05e88..c49f1c6 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3449,18 +3449,24 @@ 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 strbuf *err)
>>  {
>> -     struct ref_update *update = add_update(transaction, refname);
>> +     struct ref_update *update;
>> +
>> +     if (!new_sha1 || is_null_sha1(new_sha1))
>> +             die("BUG: 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 c5376ce..33b4383 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -10,6 +10,45 @@ struct ref_lock {
>>       int force_write;
>>  };
>>
>> +/*
>> + * A ref_transaction represents a collection of ref updates
>> + * that should succeed or fail together.
>> + *
>> + * Calling sequence
>> + * ----------------
>> + * - Allocate and initialize a `struct ref_transaction` by calling
>> + *   `ref_transaction_begin()`.
>> + *
>> + * - List intended ref updates by calling functions like
>> + *   `ref_transaction_update()` and `ref_transaction_create()`.
>> + *
>> + * - Call `ref_transaction_commit()` to execute the transaction.
>> + *   If this succeeds, the ref updates will have taken place and
>> + *   the transaction cannot be rolled back.
>> + *
>> + * - At any time call `ref_transaction_free()` to discard the
>> + *   transaction and free associated resources.  In particular,
>> + *   this rolls back the transaction if it has not been
>> + *   successfully committed.
>> + *
>> + * Error handling
>> + * --------------
>> + *
>> + * On error, transaction functions append a message about what
>> + * went wrong to the 'err' argument.  The message mentions what
>> + * ref was being updated (if any) when the error occurred so it
>> + * can be passed to 'die' or 'error' as-is.
>> + *
>> + * The message is appended to err without first clearing err.
>> + * This allows the caller to prepare preamble text to the generated
>> + * error message:
>> + *
>> + *     strbuf_addf(&err, "Error while doing foo-bar: ");
>> + *     if (ref_transaction_update(..., &err)) {
>> + *         ret = error("%s", err.buf);
>> + *         goto cleanup;
>> + *     }
>> + */
>
> I don't have a problem with the API, but I think the idiom suggested in
> the comment above is a bit silly.  Surely one would do the following
> instead:
>
>     if (ref_transaction_update(..., &err)) {
>         ret = error("Error while doing foo-bar: %s", err.buf);
>         goto cleanup;
>     }
>
> I think it would also be helpful to document whether the error string
> that is appended to the strbuf is terminated with a LF.
>
>> [...]
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>