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

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> Please pull my ref-transactions branch.

I'm at bd5736cb (2014-05-21 13:46) now.

> On Wed, May 21, 2014 at 3:00 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> 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.

That means that both mean the caller cannot do any more updates,
right?  Why not call them both _CLOSED?

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

If the distinction becomes useful in the future, wouldn't that
be the moment to add a new state?

I don't mean that there has to be a big useful distinction.  E.g.,
maybe the idea is that when using a debugger it can be useful to see
the difference between _ERROR and _CLOSED or something, and I think
that would be fine as long as the intended meaning is documented
(e.g., using comments).  My only complaint is that it's hard to
understand the code and keep track of which status should be used in a
given place unless there is some distinction between them.

[...]
> ref_transaction_rollback is no more. It has been removed.

Thanks.  Sorry I missed that last time.

Jonathan