On Wed, May 28, 2014 at 11:51 AM, Jonathan Nieder <jrnie...@gmail.com> wrote:
> 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.

I have removed the if (transaction->state != REF_TRANSACTION_OPEN)
check from _update/_delete/_create and documented this usecase.


  It can record information about what is
> 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?

Makes perfect sense.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to