git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
May 21, 2012, 01:47 UTC
Message-ID
<7vk4069ug8.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1335955259-15309-3-git-send-email-mhagger@alum.mit.edu>
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".
Previous: Junio C HamanoNext: Michael Haggerty
Message 11 of 13 in “Fix some constness errors in fetch-pack”
  1. 0/2 Fix some constness errors in fetch-packmhagger@alum.mit.edu, May 2, 2012
  2. 1/2 cmd_fetch_pack(): declare dest to be constmhagger@alum.mit.edu, May 2, 2012
  3. 2/2 cmd_fetch_pack(): fix constness problem and memory leakmhagger@alum.mit.edu, May 2, 2012
  4. Nguyen Thai Ngoc DuyMay 2, 2012
  5. Michael HaggertyMay 2, 2012
  6. 0/3 Fix some constness errors in fetch-pack and parseopt conversionNguyễn Thái Ngọc Duy, May 2, 2012
  7. 1/3 cmd_fetch_pack(): declare dest to be constNguyễn Thái Ngọc Duy, May 2, 2012
  8. 2/3 fetch-pack: use parse_options()Nguyễn Thái Ngọc Duy, May 2, 2012
  9. 3/3 cmd_fetch_pack(): fix constness problem and memory leakNguyễn Thái Ngọc Duy, May 2, 2012
  10. Junio C HamanoMay 2, 2012
  11. Junio C HamanoMay 21, 2012
  12. Michael HaggertyMay 21, 2012
  13. Michael HaggertyMay 19, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.