Re: [PATCH 12/26] checkout: fix memory leak

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Discovered via Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/checkout.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index bfa5419f335..98f98256608 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct 
> > checkout *state)
> > if (!ce)
> > die(_("make_cache_entry failed for path '%s'"), path);
> > status = checkout_entry(ce, state, NULL);
> > +   free(ce);
> > return status;
> >  }
> 
> Thanks for spotting this one and fixing it.  
> 
> In case anybody is wondering what the "only to leak" comment before
> this part of the code is about (which by the way may need to be
> updated, as the bulk of its reasoning still applies but at least we
> are no longer leaking with this patch), back when this code was
> written in 2008 or so it wasn't kosher to free cache_entry under
> certain conditions, but it has been a long time since it became OK
> to free any cache entries in later 2011---we should have done this a
> long time ago.

Thanks for the background. The next iteration will change that comment,
too (simply removing "just to leak" and rewrapping to 74 columns/line.

Ciao,
Dscho

Re: [PATCH 12/26] checkout: fix memory leak

[Junio C Hamano]

Johannes Schindelin  writes:

> Discovered via Coverity.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/checkout.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f335..98f98256608 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout 
> *state)
>   if (!ce)
>   die(_("make_cache_entry failed for path '%s'"), path);
>   status = checkout_entry(ce, state, NULL);
> + free(ce);
>   return status;
>  }

Thanks for spotting this one and fixing it.  

In case anybody is wondering what the "only to leak" comment before
this part of the code is about (which by the way may need to be
updated, as the bulk of its reasoning still applies but at least we
are no longer leaking with this patch), back when this code was
written in 2008 or so it wasn't kosher to free cache_entry under
certain conditions, but it has been a long time since it became OK
to free any cache entries in later 2011---we should have done this a
long time ago.