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

[Junio C Hamano <gitster@pobox.com>]

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.

These cmd_foo() functions, being roughly equivalent to main(), have logic
to interpret the end-user's intentions (i.e. parse_options()), and carry
out that intention.  In the long run, some _other_ parts of the system may
want to do "foo" (whatever that "foo" may be) from inside the process
without forking, and the first step to do so may be to split cmd_foo()
into two, one that does "parse options", and the other that does "foo".
At that point, it starts to matter that we make the part that does "foo"
leak free, especially if such a caller (or callers) can ask to do "foo"
number of times.

If you have a plan to split cmd_fetch_pack() and make other parts of the
system call it, probably restructuring the current separation between
"figure out what refs will be fetched" done in cmd_fetch_pack() and "fetch
these refs in heads[] array" in fetch_pack() into something else (like
"the new caller also wants to read the list of refs from the standard
input, instead of having parse them into head[] array itself"), then
freeing the memory would matter a lot more, and the approach this patch
takes is a first step in the right direction, I would think.

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