From: Junio C Hamano Date: Sat, 20 Sep 2025 17:36:36 GMT Subject: Re: [PATCH v3 02/10] xdiff: delete local variables and initialize/free xdfile_t directly Message-ID: In-Reply-To: "Ezekiel Newren via GitGitGadget" writes: > From: Ezekiel Newren > > 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.