Re: [PATCH v3 02/10] xdiff: delete local variables and initialize/free xdfile_t directly

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

"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> These local variables are essentially a hand-rolled additional
> implementation of xdl_free_ctx() inlined into xdl_prepare_ctx(). Modify
> the code to use the existing xdl_free_ctx() function so there aren't
> two ways to free such variables.

Sensible.

> +static void xdl_free_ctx(xdfile_t *xdf)
> +{
> +	xdl_free(xdf->rhash);
> +	xdl_free(xdf->rindex);
> +	xdl_free(xdf->rchg - 1);
> +	xdl_free(xdf->ha);
> +	xdl_free(xdf->recs);
> +	xdl_cha_free(&xdf->rcha);
> +}

And I like the attention to the detail of where the opening brace is
in the "moved" existing function ;-).

>  abort:
> -	xdl_free(ha);
> -	xdl_free(rindex);
> -	xdl_free(rchg);
> -	xdl_free(rhash);
> -	xdl_free(recs);
> -	xdl_cha_free(&xdf->rcha);

Upon an error, the original and the updated would behave a bit
differently here, as the original would not have touched xdf, other
than its rcha member, so the caller _could_ make use of the original
contents in the structure after seeing an error return.  With the
new code, that is no longer possible.

Its only caller is xdl_prepare_env(), and its caller is
xdl_do_diff(), both of which passes the xdfenv_t *xe given by their
callers.  There are four callers of xdl_do_diff():

 xdl_fall_back_diff() in xdiff/xutils.c
 xdl_merge() and xdl_refine_conflicts() in xdiff/xmerge.c
 xdl_diff() in xdiff/xdiffi.c

and all of them seem to pass an uninitialized piece of memory as
xdfenv_t *xe down the callchain, so this behaviour change does not
make any difference.

> +	xdl_free_ctx(xdf);

And the code certainly is safer as we know we have one place to look
at when we added a member that holds resources to xdfile_t.

> -static void xdl_free_ctx(xdfile_t *xdf) {

We know clearing/freeing side is fine, but what about initializing
side?

>  static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_t const *xpp,
>  			   xdlclassifier_t *cf, xdfile_t *xdf) {
> +	long bsize;
>  	unsigned long hav;
>  	char const *blk, *cur, *top, *prev;
>  	xrecord_t *crec;
>  
> +	xdf->ha = NULL;
> +	xdf->rindex = NULL;
> +	xdf->rchg = NULL;
> +	xdf->rhash = NULL;
> +	xdf->recs = NULL;

It turns out that this is the only place that initializes xdfile_t
in xdiff/ API, so we are covered on both ends.  xdiff/xprepare.c is
the only place we need to look at if we ever want to futz with
xdfile_t members, and with this change, we know there aren't two
ways to free things in it (there weren't two ways to initialize,
either, even before this patch).

Nice.