Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

From: Ronnie Sahlberg <sahlberg@google.com>
Date: May 21, 2014, 22:11 UTC
Message-ID: <CAL=YDW=Vpc8XGLGUTgKQc4yP5v+ViN02EeLzgRipntskok+fbw@mail.gmail.com>
In-Reply-To: <20140521220035.GE12314@google.com>

Please pull my ref-transactions branch.

On Wed, May 21, 2014 at 3:00 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 15 quoted lines

> Ronnie Sahlberg wrote:
>
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3308,6 +3308,12 @@ struct ref_update {
>>       const char refname[FLEX_ARRAY];
>>  };
>>
>> +enum ref_transaction_status {
>> +     REF_TRANSACTION_OPEN   = 0,
>> +     REF_TRANSACTION_CLOSED = 1,
>> +     REF_TRANSACTION_ERROR  = 2,
>
> What is the difference between _TRANSACTION_CLOSED and
> _TRANSACTION_ERROR?

Closed is a transaction that has been committed successfully, and which we can not do any more updates onto. Error is a transaction that has failed, and which we can not do any more updates onto.

The distinction could be useful if in the future we add support to reuse a transaction

Show 16 quoted lines

>
> [...]
>> @@ -3340,6 +3347,11 @@ void ref_transaction_free(struct ref_transaction *transaction)
>>
>>  void ref_transaction_rollback(struct ref_transaction *transaction)
>>  {
>> +     if (!transaction)
>> +             return;
>> +
>> +     transaction->status = REF_TRANSACTION_ERROR;
>> +
>>       ref_transaction_free(transaction);
>
> Once the transaction is freed, transaction->status is not reachable any
> more so no one can tell that you've set it to _ERROR.  What is the
> intended effect?

ref_transaction_rollback is no more. It has been removed.

Show 20 quoted lines

>
> [...]
>> @@ -3366,6 +3378,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>>       if (have_old && !old_sha1)
>>               die("BUG: have_old is true but old_sha1 is NULL");
>>
>> +     if (transaction->status != REF_TRANSACTION_OPEN)
>> +             die("BUG: update on transaction that is not open");
>
> Ok.
>
> [...]
>> @@ -3538,6 +3564,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>       clear_loose_ref_cache(&ref_cache);
>>
>>  cleanup:
>> +     transaction->status = ret ? REF_TRANSACTION_ERROR
>> +       : REF_TRANSACTION_CLOSED;
>
> Nit: odd use of whitespace.

fixed in ref-transactions.

Show 6 quoted lines

>
> Overall thoughts: I like the idea of enforcing the API more strictly
> ("after an error, the only permitted operations are...").  The details
> leave me a little confused because I don't think anything is
> distinguishing between _CLOSED and _ERROR.  Maybe the enum only needs
> two states.

A buggy caller might do :

transaction_begin() transaction_update() transaction_commit() (A) transaction_update() (B) transaction_commit() (C)

After A the transaction in no longer open and until we decide we want to add support for re-usable transactions (which may or may not be a good idea) we need to make sure that both B and C fails. Since the transaction in A completed successfully we can't really mark is as ERROR, instead we flag it as CLOSED.

>
> Thanks,
> Jonathan