Re: [PATCH v2 3/4] fetch: allow passing a transaction to `s_update_ref()`
From: Junio C Hamano <hidden>
Date: 2021-01-08 23:54:30
Patrick Steinhardt [off-list ref] writes:
quoted hunk
@@ -597,10 +598,17 @@ static int s_update_ref(const char *action, rla = default_rla.buf; msg = xstrfmt("%s: %s", rla, action); - transaction = ref_transaction_begin(&err); + /* + * If no transaction was passed to us, we manage the transaction + * ourselves. Otherwise, we trust the caller to handle the transaction + * lifecycle. + */ if (!transaction) { - ret = STORE_REF_ERROR_OTHER; - goto out; + transaction = our_transaction = ref_transaction_begin(&err); + if (!transaction) { + ret = STORE_REF_ERROR_OTHER; + goto out; + } }
OK, this answers the question I posed in the review of the previous step. We need to separate out "if (!transaction || ...)" into two anyway with this step, so it is easier to see what changed in this step if we separated in the previous preparatory clean-up step.
quoted hunk
@@ -611,15 +619,17 @@ static int s_update_ref(const char *action, goto out; } - ret = ref_transaction_commit(transaction, &err); - if (ret) { - ret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT - : STORE_REF_ERROR_OTHER; - goto out; + if (our_transaction) { + ret = ref_transaction_commit(our_transaction, &err); + if (ret) { + ret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT + : STORE_REF_ERROR_OTHER; + goto out; + }
The switch statement suggested earlier would shine when the constants involved have such long names.
quoted hunk
} out: - ref_transaction_free(transaction); + ref_transaction_free(our_transaction); if (ret) error("%s", err.buf); strbuf_release(&err);
Makes sense. Thanks.