Re: [PATCH v11 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

> Ronnie Sahlberg wrote:
>
>> --- a/refs.c
>> +++ b/refs.c
> [...]
>> @@ -3385,6 +3408,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>  {
>>       struct ref_update *update;
>>
>> +     if (transaction->state != REF_TRANSACTION_OPEN)
>> +             return -1;
>
> I still think this is a step in the wrong direction.  It means that
> instead of being able to do
>
>         if (ref_transaction_update(..., &err))
>                 die("%s", err.buf);
>
> and be certain that err.buf has a meaningful message, now I have to
> worry that if the state were not REF_TRANSACTION_OPEN (e.g., due to
> someone else's code forgetting to handle an error) then the result
> would be a plain
>
>         fatal:
>
> The behavior would be much easier to debug if the code were
>
>         if (transaction->state != REF_TRANSACTION_OPEN)
>                 die("BUG: update on transaction that is not open");
>
> since then the symptom would be
>
>         fatal: BUG: update on transaction that is not open
>
> which is easier to work with.
>
> If a non-OPEN state were a normal, recoverable error, then it could
> append a message to the *err argument.  But there's no message that
> could be put there that would be meaningful to the end-user.  So I
> suspect it's not a normal, recoverable error.
>
> If we want to collect errors to make _commit() later fail with a
> message like '38 refs failed to update' or something (or a
> string_list, if the API is to change that way in the future), then
> _update() should not fail.

> wrong with this update in the transaction and succeed so the caller
> knows to keep going and collect other updates before the _commit()
> that will fail.
>
> Of course it's easily possible I'm missing something.  That's just how
> I see it now.
>
> Does that make sense?