Re: [PATCH 12/23] ref_transaction_commit(): break into multiple functions

[Michael Haggerty]

On 05/17/2017 07:44 PM, Stefan Beller wrote:
> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty  
> wrote:
>> Break the function `ref_transaction_commit()` into two functions,
>> `ref_transaction_prepare()` and `ref_transaction_finish()`, with a
>> third function, `ref_transaction_abort()`, that can be used to abort
>> the transaction. Break up the `ref_store` methods similarly.
>>
>> This split will make it easier to implement compound reference stores,
>> where a transaction might have to span multiple underlying stores. In
>> that case, we would first want to "prepare" the sub-transactions in
>> each of the reference stores. If any of the "prepare" steps fails, we
>> would "abort" all of the sub-transactions. Only if all of the
>> "prepare" steps succeed would we "finish" each of them.
> [...]

Thanks for your comments. While I was incorporating them, I realized
that other parts of the documentation needed updates, too. And while I
was fixing that, I noticed that the interface was unnecessarily
complicated. The old version required either `commit` or `prepare`
followed by `finish`. But there is no reason that the public API has to
expose `finish`. So instead, let's call `prepare` an optional step that
is allowed to precede `commit`, and make `commit` smart enough to call
`prepare` if it hasn't been called already, and then call `finish`.

Furthermore, let's make it possible to call `abort` when the transaction
is in OPEN as well as PREPARED state rather than requiring `free` in
OPEN state and `abort` in PREPARED state.

I will make these changes and revamp the docs for v2.

Michael

Re: [PATCH 12/23] ref_transaction_commit(): break into multiple functions

[Stefan Beller]

On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty  wrote:
> Break the function `ref_transaction_commit()` into two functions,
> `ref_transaction_prepare()` and `ref_transaction_finish()`, with a
> third function, `ref_transaction_abort()`, that can be used to abort
> the transaction. Break up the `ref_store` methods similarly.
>
> This split will make it easier to implement compound reference stores,
> where a transaction might have to span multiple underlying stores. In
> that case, we would first want to "prepare" the sub-transactions in
> each of the reference stores. If any of the "prepare" steps fails, we
> would "abort" all of the sub-transactions. Only if all of the
> "prepare" steps succeed would we "finish" each of them.
>
> Signed-off-by: Michael Haggerty 
> ---

Code looks good to me.

>  /*
>   * Transaction states.
> - * OPEN:   The transaction is in a valid state and can accept new updates.
> - * An OPEN transaction can be committed.
> - * CLOSED: A closed transaction is no longer active and no other operations
> - * than free can be used on it in this state.
> - * A transaction can either become closed by successfully committing
> - * an active transaction or if there is a failure while building
> - * the transaction thus rendering it failed/inactive.
> + *
> + * OPEN:   The transaction is in a valid state and new updates can be
> + * added to it. An OPEN transaction can be prepared or
> + * committed.

All of these states are valid, OPEN is not more valid than the others. ;)
Maybe:

OPEN: initial state. new updates can be added to it. An OPEN
transaction can be prepared, committed.

Is it possible to also abort/free an open transaction?

> + *
> + * PREPARED: ref_transaction_prepare(), which locks all of the
> + * references involved in the update and checks that the
> + * update has no errors, has been called successfully for the
> + * transaction.

Maybe add that it cannot be altered any more? Possible ways out are
commit and abort IIUC?


> + *
> + * CLOSED: The transaction is no longer active. No other operations
> + * than free can be used on it in this state. A transaction
> + * becomes closed if there is a failure while building the
> + * transaction, if an active transaction is committed,  or if a

The wording here is off as it doesn't use the lingo as introduced before.
(What is an "active" transaction?  I think you mean OPEN in this case)



> + * prepared transaction is finished.
>   */


Also s/prepared/PREPARED/, add "or aborted"(?)
as that is also a viable way IIUC.

Thanks,
Stefan

[PATCH 12/23] ref_transaction_commit(): break into multiple functions

[Michael Haggerty]

Break the function `ref_transaction_commit()` into two functions,
`ref_transaction_prepare()` and `ref_transaction_finish()`, with a
third function, `ref_transaction_abort()`, that can be used to abort
the transaction. Break up the `ref_store` methods similarly.

This split will make it easier to implement compound reference stores,
where a transaction might have to span multiple underlying stores. In
that case, we would first want to "prepare" the sub-transactions in
each of the reference stores. If any of the "prepare" steps fails, we
would "abort" all of the sub-transactions. Only if all of the
"prepare" steps succeed would we "finish" each of them.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 34 ++---
 refs.h   | 37 ++-
 refs/files-backend.c | 71 +---
 refs/refs-internal.h | 42 ---
 4 files changed, 157 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 14dec88e7f..689362db1e 100644
--- a/refs.c
+++ b/refs.c
@@ -1689,8 +1689,8 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
  refs_heads_master, logmsg);
 }
 
-int ref_transaction_commit(struct ref_transaction *transaction,
-  struct strbuf *err)
+int ref_transaction_prepare(struct ref_transaction *transaction,
+   struct strbuf *err)
 {
struct ref_store *refs = transaction->ref_store;
 
@@ -1700,7 +1700,35 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
return -1;
}
 
-   return refs->be->transaction_commit(refs, transaction, err);
+   return refs->be->transaction_prepare(refs, transaction, err);
+}
+
+int ref_transaction_finish(struct ref_transaction *transaction,
+  struct strbuf *err)
+{
+   struct ref_store *refs = transaction->ref_store;
+
+   return refs->be->transaction_finish(refs, transaction, err);
+}
+
+int ref_transaction_abort(struct ref_transaction *transaction,
+ struct strbuf *err)
+{
+   struct ref_store *refs = transaction->ref_store;
+
+   return refs->be->transaction_abort(refs, transaction, err);
+}
+
+int ref_transaction_commit(struct ref_transaction *transaction,
+  struct strbuf *err)
+{
+   int ret;
+
+   ret = ref_transaction_prepare(transaction, err);
+   if (ret)
+   return ret;
+
+   return ref_transaction_finish(transaction, err);
 }
 
 int refs_verify_refname_available(struct ref_store *refs,
diff --git a/refs.h b/refs.h
index b2d9626be6..4139e917f5 100644
--- a/refs.h
+++ b/refs.h
@@ -525,7 +525,10 @@ int ref_transaction_verify(struct ref_transaction 
*transaction,
 
 /*
  * Commit all of the changes that have been queued in transaction, as
- * atomically as possible.
+ * atomically as possible. This is equivalent to calling
+ * `ref_transaction_prepare()` then `ref_transaction_finish()` (see
+ * below), which you can call yourself if you want finer control over
+ * reference updates.
  *
  * Returns 0 for success, or one of the below error codes for errors.
  */
@@ -536,6 +539,38 @@ int ref_transaction_verify(struct ref_transaction 
*transaction,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err);
 
+/*
+ * Perform the preparatory stages of commiting `transaction`. Acquire
+ * any needed locks, check preconditions, etc.; basically, do as much
+ * as possible to ensure that the transaction will be able to go
+ * through, stopping just short of making any irrevocable or
+ * user-visible changes. Anything that this function does should be
+ * able to be finished up by calling `ref_transaction_finish()` or
+ * rolled back by calling `ref_transaction_abort()`.
+ *
+ * On success, return 0 and leave the transaction in "prepared" state.
+ * On failure, abort the transaction and return one of the
+ * `TRANSACTION_*` constants.
+ *
+ * Callers who don't need such fine-grained control over commiting
+ * reference transactions should just call `ref_transaction_commit()`.
+ */
+int ref_transaction_prepare(struct ref_transaction *transaction,
+   struct strbuf *err);
+
+/*
+ * Perform the final commit of `transaction`, which has already been
+ * prepared.
+ */
+int ref_transaction_finish(struct ref_transaction *transaction,
+  struct strbuf *err);
+
+/*
+ * Abort `transaction`, which has already been prepared.
+ */
+int ref_transaction_abort(struct ref_transaction *transaction,
+ struct strbuf *err);
+
 /*
  * Like ref_transaction_commit(), but optimized for creating
  * references when originally initializing a repository (e.g., by "git
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7ddd4f87d5..f48409132d 100644
---