Re: [PATCH] mailmap: fix check for freeing memory

[Jeff King <peff@peff.net>]

On Tue, Aug 20, 2013 at 03:40:02PM +0200, Thomas Rast wrote:

> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
> > The condition as it is written in that line was most likely intended to
> > check for the pointer passed to free(), rather than checking for the
> > 'repo_abbrev', which is already checked against being non null at the
> > beginning of the function.
> [...]
> > -			if (repo_abbrev)
> > +			if (*repo_abbrev)
> >  				free(*repo_abbrev);
> 
> But now the test is useless, because free(NULL) is defined to be a
> no-op.

Yeah, I think we should just drop the conditional completely.

I am not sure of the complete back-story. The earlier check for
repo_abbrev to be non-NULL was added by 8503ee4, after this check on
free() already existed. So that was when this conditional became
redundant.

But the line right after this one unconditionally assigns to
"*repo_abbrev", so we would always segfault in such a case anyway (which
is what 8503ee4 was fixing).

So I think it was either a misguided "don't pass NULL to free" check
that was simply wrong, or it was an incomplete "make sure repo_abbrev is
not NULL" check. And the first is useless, and the second is now
redundant due to 8503ee4. So it should simply be free().

-Peff