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

From: Stefan Beller <stefanbeller@googlemail.com>
Date: Aug 20, 2013, 14:17 UTC
Message-ID: <52137A63.3010609@googlemail.com>
In-Reply-To: <878uzw7a6l.fsf@linux-k42r.v.cablecom.net>

On 08/20/2013 03:40 PM, Thomas Rast wrote:

Show 14 quoted lines

> 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.
>

Yes, indeed. Thanks for reviewing.

Stepping two steps back, I am trying to figure out, what this repo_abrev thing is doing, as I could find no documentation.

It's passed as a double pointer as declared in mailmap.h: int read_mailmap(struct string_list *map, char **repo_abbrev);

However grepping for "read_mailmap(" (bracket to prevent finding 
read_mailmap_XXX as often used in mailmap.c itself) 
	grep -nHIirF --exclude-dir=.git -- "read_mailmap("
throughout all the sources I just find one occurence having the
second argument not being 'NULL' and that is in 
	builtin/shortlog.c:212:	read_mailmap(&log->mailmap, &log->common_repo_prefix);

which turns out to be:

void shortlog_init(struct shortlog *log)
{
	memset(log, 0, sizeof(*log));

	read_mailmap(&log->mailmap, &log->common_repo_prefix);
	...

So we're passing there an address, which was just set to zero. This is the only occurence of passing a value at all and the value being passed is 0, so the free in the original patch doesn't need that check either.

As I am resending the patch, could somebody please explain me the mechanism of the "# repo-abbrev:" line? Even git itself doesn't use it in the .mailmap file, but a quick google search shows up only kernel repositories.

Stefan