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

Please pull my ref-transactions branch.

On Wed, May 21, 2014 at 3:00 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

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

[...]

quoted

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

[...]

quoted

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

[...]

quoted

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