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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2023, 22:46 UTC
Message-ID: <xmqqa5tjje0y.fsf@gitster.g>
In-Reply-To: <dea0fbb139a82fe449a7edab6a8f445ce763d0c0.1695059978.git.gitgitgadget@gmail.com>

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 15 quoted lines

>  	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);

Show 10 quoted lines

> +	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?