On Wed, May 28, 2014 at 11:51 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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.
Agreed.
I have removed the if (transaction->state != REF_TRANSACTION_OPEN)
check from _update/_delete/_create and documented this usecase.
Thanks.!
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html