Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 21, 2012, 08:13 UTC
Message-ID: <4FB9F93C.4010308@alum.mit.edu>
In-Reply-To: <7vk4069ug8.fsf@alter.siamese.dyndns.org>

On 05/21/2012 03:47 AM, Junio C Hamano wrote:

Show 11 quoted lines

> mhagger@alum.mit.edu writes:
>
>> I understand that it is not crucial to free memory allocated in a
>> cmd_*() function but it is unclear to me whether it is *preferred* to
>> let the process clean up take care of things.
>
> Traditionally, the cmd_foo() functions roughly correspond to main() in
> other programs, so from that point of view, "it is not crucial to free" is
> an understatement. It is not even worth wasting people's time to (1)
> decide which way is *preferred* and to (2) churn the code only to match
> whichever way.

OK, thanks for the info. I will remove the "freeing-memory" part of the patch.

> If you have a plan to split cmd_fetch_pack() and make other parts of the
> system call it, [...]

No, I have no plans for cmd_fetch_pack() besides cleaning up the constness errors that I found when randomly reading the code.

Show 7 quoted lines

> It also seems that some cruft has snuck into this patch, e.g. like this
> part,
>
>> -	int i, ret, nr_heads;
>> +	int i, ret;
>
> that do not have anything to do with "fix constness" nor "memory leak".

This particular hunk is part of moving alloc_heads, nr_heads, and heads together to make it more obvious that they are part of an ALLOC_GROW triplet. Previously alloc_heads was a block-local variable used only in the case of the --stdin option.

But I admit that the patch is harder than necessary to read because of the indentation changes etc, so I will break it up into more digestible quanta.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/