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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
