Re: [PATCH 2/3] xdiff: refactor a function

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Rather than having to remember exactly what to free after an
> allocation failure use the standard "goto out" pattern. This will
> simplify the next commit that starts handling currently unhandled
> failures.

It sound like this is meant to be a no-op clean-up; let's see.

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  xdiff/xmerge.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index fff0b594f9a..48c5e9e3f35 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
>  int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
>  		xmparam_t const *xmp, mmbuffer_t *result)
>  {
> -	xdchange_t *xscr1, *xscr2;
> -	xdfenv_t xe1, xe2;
> -	int status;
> +	xdchange_t *xscr1 = NULL, *xscr2 = NULL;
> +	xdfenv_t xe1 = { 0 }, xe2 = { 0 };
> +	int status = -1;
>  	xpparam_t const *xpp = &xmp->xpp;
>  
>  	result->ptr = NULL;
>  	result->size = 0;
>  
> -	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {
> -		return -1;
> -	}
> -	if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {
> -		xdl_free_env(&xe1);
> -		return -1;
> -	}

OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I
didn't check if patience and histogram also do so correctly), so the
original was not leaking xe1 or xe2.

> +	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0)
> +		goto out;
> +
> +	if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0)
> +		goto out;
> +

And this does not change that.

>  	if (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||
>  	    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||
> -	    xdl_build_script(&xe1, &xscr1) < 0) {
> -		xdl_free_env(&xe1);
> -		return -1;
> -	}
> +	    xdl_build_script(&xe1, &xscr1) < 0)
> +		goto out;
> +

Here, as xdl_build_script() does free the script it failed to build,
but not the xe it was given, the original is correct to free xe1.

We jump from here to leave the freeing of xe1 to the clean-up part.

>  	if (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||
>  	    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||
> -	    xdl_build_script(&xe2, &xscr2) < 0) {
> -		xdl_free_script(xscr1);
> -		xdl_free_env(&xe1);
> -		xdl_free_env(&xe2);
> -		return -1;
> -	}
> +	    xdl_build_script(&xe2, &xscr2) < 0)
> +		goto out;

Ditto for xe2 here.

>  	status = 0;
>  	if (!xscr1) {
>  		result->ptr = xdl_malloc(mf2->size);
> @@ -727,6 +722,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
>  				      &xe2, xscr2,
>  				      xmp, result);
>  	}
> + out:
>  	xdl_free_script(xscr1);
>  	xdl_free_script(xscr2);

And after the post-context of this hunk, we do free_env() both xe1
and xe2, so we should be doing the same thing as before.

Looking good.