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

"Ezekiel Newren via GitGitGadget" [off-list ref] writes:

From: Ezekiel Newren <redacted>

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.