Subject: Re: [PATCH 1/3] refs: push lock management into packed backend

From: Junio C Hamano <[email-masked]>
Date: Mon, 18 Sep 2023 15:46:21 -0700

"Han-Wen Nienhuys via GitGitGadget" <[email-masked]> writes:

>  	struct ref_transaction *tr;
> +	int ret = 0;
>  	assert(err);
>  
>  	CALLOC_ARRAY(tr, 1);
>  	tr->ref_store = refs;
> +
> +	if (refs->be->transaction_begin)
> +		ret = refs->be->transaction_begin(refs, tr, err);
> +	if (ret) {
> +		free(tr);
> +		return NULL;
> +	}
>  	return tr;
>  }

This looks a bit more convoluted than necessary.  Is it the same as

	if (refs->be->transaction_begin &&
	    refs->be->transaction_begin(refs, tr, err))
		FREE_AND_NULL(tr);

> +	if (backend_data->packed_transaction) { 
> +		if (backend_data->packed_transaction_needed) {
> +			ret = ref_transaction_commit(packed_transaction, err);
> +			if (ret)
> +				goto cleanup;
> +			/* TODO: leaks on error path. */
> +			ref_transaction_free(packed_transaction);
> +			packed_transaction = NULL;
> +			backend_data->packed_transaction = NULL;
> +		} else {

If it were just a matter of flipping the early return and freeing of
the transaction before going to clean-up, then that would have been
less effort than leaving the TODO: comment.  What other things are
needed to plug this leak?