Re: [PATCH 2/3] xdiff: refactor a function
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 9, 2022, 18:04 UTC
- Message-ID
- <xmqqmtiz9aro.fsf@gitster.g>
- In-Reply-To
- <8655bb0348d7344ae85c8d521fb1ec7a5f4188d2.1644404356.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 32 quoted lines
>
> 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.
Show 6 quoted lines
> + 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.
Show 9 quoted lines
> 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.
Show 10 quoted lines
> 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.
Show 10 quoted lines
> 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.