Re: [PATCH v3 02/10] xdiff: delete local variables and initialize/free xdfile_t directly
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 20, 2025, 17:36 UTC
- Message-ID
- <xmqqcy7lxbyz.fsf@gitster.g>
- In-Reply-To
- <b79157e64f0950d25a23c50a8ea83cfddf67ddf5.1758294992.git.gitgitgadget@gmail.com>
"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 9 quoted lines
> +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 ;-).
Show 7 quoted lines
> 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?
Show 12 quoted lines
> 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.