Re: [PATCH v2 3/4] fetch: allow passing a transaction to `s_update_ref()`

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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.