Please pull my ref-transactions branch.
On Wed, May 21, 2014 at 3:00 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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.
The distinction could be useful if in the future we add support to
reuse a transaction
>
> [...]
>> @@ -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.
>
> [...]
>> @@ -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.
>
> 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
--
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