Re: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames()

[Elijah Newren <newren@gmail.com>]

On Sun, Feb 20, 2022 at 6:35 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Sun, Feb 20, 2022 at 01:29:50AM +0000, Elijah Newren via GitGitGadget wrote:
> >  merge-ort.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
>
> Both versions of this patch look good to me (in fact, I appreciated
> seeing both v1 and v2, since v1 makes it more obvious what is changing,
> but v2 does the whole thing in a little bit of a cleaner way).

Thanks for taking a look!

> > diff --git a/merge-ort.c b/merge-ort.c
> > index d85b1cd99e9..3d7f9feb6f7 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,
> >                                     struct tree *side1,
> >                                     struct tree *side2)
> >  {
> > -     struct diff_queue_struct combined;
> > +     struct diff_queue_struct combined = { 0 };
> >       struct rename_info *renames = &opt->priv->renames;
> > -     int need_dir_renames, s, clean = 1;
> > +     int need_dir_renames, s, i, clean = 1;
>
> And this entire patch looks good to me, but I did wonder about why "i"
> is an int here. Shouldn't it be a size_t instead? Looking at
> diff_queue_struct's definition, both "alloc" and "nr" are signed ints,
> when they should almost certainly be unsigned to avoid overflow.

You may be right, but I'm not sure we're too worried right now about
folks having billions of paths involved in renames; such a repo would
make even the Microsoft monorepos look miniscule.  ;-)