Re: [PATCHv3 1/6] shallow: fix a memleak

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 31, 2015, 02:52 UTC
Message-ID: <CAPig+cRi9L+bEJsjFdihLQJjb38Bg500=sZ+w4FubR5f3=n-bQ@mail.gmail.com>
In-Reply-To: <1427764931-27745-2-git-send-email-sbeller@google.com>

On Mon, Mar 30, 2015 at 9:22 PM, Stefan Beller <sbeller@google.com> wrote:

Show 19 quoted lines

> shallow: fix a memleak
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/shallow.c b/shallow.c
> index d8bf40a..11d5c4e 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -412,11 +412,12 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
>         struct commit_list *head = NULL;
>         int bitmap_nr = (info->nr_bits + 31) / 32;
>         int bitmap_size = bitmap_nr * sizeof(uint32_t);
> -       uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
> -       uint32_t *bitmap = paint_alloc(info);
> +       uint32_t *tmp, *bitmap;
>         struct commit *c = lookup_commit_reference_gently(sha1, 1);
>         if (!c)
>                 return;
> +       tmp = xmalloc(bitmap_size); /* to be freed before return */
> +       bitmap = paint_alloc(info);

You've made two unrelated changes in this patch, only one of which is a fix for the memory leak mentioned by the commit message.

The 'tmp' change fixes the memory leak. The 'bitmap' change, however, merely avoids doing work unnecessarily in the "early return" case. Since 'bitmap' gets released by the caller of paint_down() when the paint_info structure is freed, it is misleading to claim that that particular change is a "memleak" fix.

Either split the two changes into separate patches or augment the commit message to say something along the lines of:

    While here, also avoid unnecessarily allocating 'bitmap' within
    paint_info in the early-return case.

Show 5 quoted lines

>         memset(bitmap, 0, bitmap_size);
>         bitmap[id / 32] |= (1 << (id % 32));
>         commit_list_insert(c, &head);
> --
> 2.3.0.81.gc37f363