Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 2, 2013, 17:25 UTC
Message-ID: <CAMP44s2ym5UEPo8kr6YKf1x_P_0L+o_9vWdCTA6MPDQsRy7d1A@mail.gmail.com>
In-Reply-To: <1370188017-24672-8-git-send-email-rene.scharfe@lsrfire.ath.cx>

On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:

Show 30 quoted lines

> The merge functions duplicate entries as needed and they don't free
> them.  Release them in unpack_nondirectories, the same function
> where they were allocated, after we're done.
>
> As suggested by Felipe, use the same loop style (zero-based for loop)
> for freeing as for allocating.
>
> Improved-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  unpack-trees.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 2dbc05d..57b4074 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -600,9 +600,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
>                 src[i + o->merge] = create_ce_entry(info, names + i, stage);
>         }
>
> -       if (o->merge)
> -               return call_unpack_fn((const struct cache_entry * const *)src,
> -                                     o);
> +       if (o->merge) {
> +               int rc = call_unpack_fn((const struct cache_entry * const *)src,
> +                                       o);
> +               for (i = 0; i < n; i++) {
> +                       struct cache_entry *ce = src[i + o->merge];
> +                       if (ce != o->df_conflict_entry)

It's possible that ce is NULL, but you didn't add that check because free(NULL) still works? Or because ce cannot be NULL?

If it's the former, I think it's clearer if we check that ce is not NULL either way.

Otherwise it's OK by me.

> +                               free(ce);
> +               }
> +               return rc;
> +       }

-- 
Felipe Contreras