From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 21 May 2012 08:13:48 GMT
Subject: Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
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:
> 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.

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