Patrick Steinhardt <ps@xxxxxx> writes:
> @@ -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.
> @@ -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.
> }
>
> out:
> - ref_transaction_free(transaction);
> + ref_transaction_free(our_transaction);
> if (ret)
> error("%s", err.buf);
> strbuf_release(&err);
Makes sense. Thanks.
