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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 30, 2015 at 9:22 PM, Stefan Beller <sbeller@google.com> wrote:
> 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.

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