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

Re: [PATCH] Copy resolve_ref() return value for longer use

From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date
Nov 13, 2011, 07:09 UTC
Message-ID
<CACsJy8ARAzNWjZfXwnNG0AprCFXLCkiDrE+eFj9icbeNX14xKw@mail.gmail.com>
In-Reply-To
<7vehxcpns5.fsf@alter.siamese.dyndns.org>
2011/11/13 Junio C Hamano <gitster@pobox.com>:
Show 7 quoted lines
> The above log message with the callchain would be a perfect explanation
> for a patch to fix builtin/merge.c and nothing else, but among 50+
> callsites of resolve_ref(), only 5 places are modified in this patch, and
> it is not explained in the commit log message at all how they were chosen.
> Were the other 40+ or so places inspected and deemed to be OK? Or is this
> "I just did a few of them in addition to the immediate need of fixing what
> was reported, and the rest are left as an exercise to the readers" patch?

I went through all of them. Most of them only checks if return value is NULL, or does one-time string comparison. Others do xstrdup() to save the return value. Will update the patch message to reflect this.

Show 5 quoted lines
> Some variables that receive xstrdup()'ed result with this patch are
> globals whose lifetime lasts while the process is alive, and I do not
> think it is a huge problem that this patch does not free it, but it seems
> the patch does not free some other function scope variables once their use
> is done even when it is fairly easy to do so.
Will fix.
> How much overhead would it incur if we return xstrdup()'ed memory from the
> resolve_ref() and make it the caller's responsibility to free it? Would it
> make the calling site too ugly?

Given a lot of callsites do "if (resolve_ref(...)) ...", yes it may look ugly. Though if you don't mind a bigger patch, we could turn

const char *resolve_ref(const char *path, const char *sha1, int reading, int *flag);

to

int resolve_ref(const char *path, const char *sha1, int reading, int *flag, char **ref);

where *ref is the current return value and is only allocated by resolve_ref() if ref != NULL.

-- 
Duy
Previous: Junio C HamanoNext: Junio C Hamano
Message 8 of 31 in “Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7”
  1. Nguyen Thai Ngoc DuyNov 7, 2011
  2. Tony WangNov 7, 2011
  3. Nguyen Thai Ngoc DuyNov 7, 2011
  4. Tony WangNov 7, 2011
  5. Nguyen Thai Ngoc DuyNov 7, 2011
  6. Copy resolve_ref() return value for longer useNguyễn Thái Ngọc Duy, Nov 8, 2011
  7. Junio C HamanoNov 13, 2011
  8. Nguyen Thai Ngoc DuyNov 13, 2011
  9. Junio C HamanoNov 13, 2011
  10. 1/2 Convert many resolve_ref() calls to read_ref*() and ref_exists()Nguyễn Thái Ngọc Duy, Nov 13, 2011
  11. 2/2 Copy resolve_ref() return value for longer useNguyễn Thái Ngọc Duy, Nov 13, 2011
  12. Junio C HamanoNov 13, 2011
  13. Nguyen Thai Ngoc DuyNov 14, 2011
  14. Junio C HamanoNov 14, 2011
  15. Jeff KingNov 14, 2011
  16. Nguyen Thai Ngoc DuyNov 15, 2011
  17. 01/10 Allow resolve_ref() caller to decide whether to receive static bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  18. 02/10 cmd_merge: convert to single exit pointNguyễn Thái Ngọc Duy, Nov 15, 2011
  19. 03/10 merge: do not point "branch" to a resolve_ref()'s static bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  20. 04/10 commit: move resolve_ref() closer to where the return value is usedNguyễn Thái Ngọc Duy, Nov 15, 2011
  21. 05/10 checkout: do not try xstrdup() on NULLNguyễn Thái Ngọc Duy, Nov 15, 2011
  22. 06/10 reflog-walk.c: request allocated buffer from resolve_ref()Nguyễn Thái Ngọc Duy, Nov 15, 2011
  23. 07/10 receive-pack: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  24. 08/10 notes: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  25. 09/10 fmt-merge-msg: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  26. 10/10 branch: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  27. Junio C HamanoNov 15, 2011
  28. Junio C HamanoNov 13, 2011
  29. Tony WangDec 10, 2011
  30. Nguyen Thai Ngoc DuyDec 10, 2011
  31. Tony WangDec 11, 2011

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.