Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
On Wed, May 21, 2014 at 3:22 PM, Jonathan Nieder jrnie...@gmail.com wrote:
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 jrnie...@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.
I have documented the transaction states in refs.c
Thanks.
ronnie sahlberg
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
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?
[...]
@@ -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?
[...]
@@ -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.
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.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 jrnie...@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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
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 jrnie...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
