git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Feb 23, 2022, 07:57 UTC
Message-ID
<CABPp-BFO0dz6t2ouC0+40Z6oDnGc32GKDHUpeFfh7C_Vpccerg@mail.gmail.com>
In-Reply-To
<YhL6i3x1l4qwQJg+@nand.local>
On Sun, Feb 20, 2022 at 6:35 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 8 quoted lines
>
> 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!
Show 18 quoted lines
> > 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. ;-)

Previous: Taylor BlauNext: Ævar Arnfjörð Bjarmason
Message 11 of 15 in “Fix a couple small leaks in merge-ort”
  1. 0/2 Fix a couple small leaks in merge-ortElijah Newren via GitGitGadget, Feb 19, 2022
  2. 1/2 merge-ort: fix small memory leak in detect_and_process_renames()Elijah Newren via GitGitGadget, Feb 19, 2022
  3. Ævar Arnfjörð BjarmasonFeb 19, 2022
  4. Elijah NewrenFeb 20, 2022
  5. 2/2 merge-ort: fix small memory leak in unique_path()Elijah Newren via GitGitGadget, Feb 19, 2022
  6. Ævar Arnfjörð BjarmasonFeb 19, 2022
  7. Elijah NewrenFeb 20, 2022
  8. 0/2 Fix a couple small leaks in merge-ortElijah Newren via GitGitGadget, Feb 20, 2022
  9. 1/2 merge-ort: fix small memory leak in detect_and_process_renames()Elijah Newren via GitGitGadget, Feb 20, 2022
  10. Taylor BlauFeb 21, 2022
  11. Elijah NewrenFeb 23, 2022
  12. Ævar Arnfjörð BjarmasonJun 1, 2022
  13. Flaky SANITIZE=leak test "regression" in v2.36.0 (was: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames())Ævar Arnfjörð Bjarmason, Jun 1, 2022
  14. 2/2 merge-ort: fix small memory leak in unique_path()Elijah Newren via GitGitGadget, Feb 20, 2022
  15. Taylor BlauFeb 21, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.