Re: [PATCH v8 30/44] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:01:17
Ronnie Sahlberg wrote:
quoted hunk ↗ jump to hunk
--- 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? [...]
quoted hunk ↗ jump to hunk
@@ -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? [...]
quoted hunk ↗ jump to hunk
@@ -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 hunk ↗ jump to hunk
@@ -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