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

On Wed, May 28, 2014 at 11:51 AM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

--- a/refs.c
+++ b/refs.c

[...]

quoted

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