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

Re: [PATCH v2 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`

From
Junio C Hamano <gitster@pobox.com>
Date
May 8, 2025, 20:05 UTC
Message-ID
<xmqq4ixu6f6q.fsf@gitster.g>
In-Reply-To
<20250508195714.GA18229@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 25 quoted lines
>> -	if (mmap_strategy != MMAP_OK && snapshot->mmapped) {
>> -		/*
>> -		 * We don't want to leave the file mmapped, so we are
>> -		 * forced to make a copy now:
>> -		 */
>> -		size_t size = snapshot->eof - snapshot->start;
>> -		char *buf_copy = xmalloc(size);
>> -
>> -		memcpy(buf_copy, snapshot->start, size);
>> -		clear_snapshot_buffer(snapshot);
>> -		snapshot->buf = snapshot->start = buf_copy;
>> -		snapshot->eof = buf_copy + size;
>> -	}
>> +	if (mmap_strategy == MMAP_TEMPORARY && snapshot->mmapped)
>> +		munmap_temporary_snapshot(snapshot);
>
> The original triggers this conditional whenever the strategy is not
> MMAP_OK (so MMAP_TEMPORARY or MMAP_NONE). But in your post-image, we do
> so only for MMAP_TEMPORARY.
>
> I can guess that the two end up the same, because snapshot->mmapped
> would never be set when MMAP_NONE is set. But if we are going to make
> such a logical inference, it should be explained in the commit message
> (though my preference is to leave the code as-is, or to pull the
> refactor into its own commit).
Good thinking.

An "extract" step that is meant as a preliminary refactoring should not make such a change. The change may or may not prepare the code for better maintainability, but I agree that such a change needs to be justified separately.

Thanks.
Previous: Jeff KingNext: shejialuo
Message 22 of 67 in “align the behavior when opening "packed-refs"”
  1. 0/4 align the behavior when opening "packed-refs"shejialuo, May 6, 2025
  2. 1/4 packed-backend: skip checking consistency of empty packed-refs fileshejialuo, May 6, 2025
  3. Junio C HamanoMay 6, 2025
  4. shejialuoMay 7, 2025
  5. Junio C HamanoMay 6, 2025
  6. shejialuoMay 7, 2025
  7. 2/4 packed-backend: extract snapshot allocation in `load_contents`shejialuo, May 6, 2025
  8. Junio C HamanoMay 6, 2025
  9. 3/4 packed-backend: extract munmap operation for `MMAP_TEMPORARY`shejialuo, May 6, 2025
  10. Junio C HamanoMay 6, 2025
  11. Junio C HamanoMay 6, 2025
  12. shejialuoMay 7, 2025
  13. 4/4 packed-backend: use mmap when opening large "packed-refs" fileshejialuo, May 6, 2025
  14. Junio C HamanoMay 6, 2025
  15. Junio C HamanoMay 6, 2025
  16. shejialuoMay 7, 2025
  17. 0/4 align the behavior when opening "packed-refs"shejialuo, May 7, 2025
  18. 1/4 packed-backend: fsck should allow an empty "packed-refs" fileshejialuo, May 7, 2025
  19. 2/4 packed-backend: extract snapshot allocation in `load_contents`shejialuo, May 7, 2025
  20. 3/4 packed-backend: extract munmap operation for `MMAP_TEMPORARY`shejialuo, May 7, 2025
  21. Jeff KingMay 8, 2025
  22. Junio C HamanoMay 8, 2025
  23. shejialuoMay 9, 2025
  24. 4/4 packed-backend: mmap large "packed-refs" file during fsckshejialuo, May 7, 2025
  25. Jeff KingMay 8, 2025
  26. shejialuoMay 9, 2025
  27. Jeff KingMay 9, 2025
  28. shejialuoMay 9, 2025
  29. Junio C HamanoMay 7, 2025
  30. Jeff KingMay 8, 2025
  31. Junio C HamanoMay 8, 2025
  32. Jeff KingMay 8, 2025
  33. shejialuoMay 9, 2025
  34. 0/3 align the behavior when opening "packed-refs"shejialuo, May 11, 2025
  35. 1/3 packed-backend: fsck should allow an empty "packed-refs" fileshejialuo, May 11, 2025
  36. Patrick SteinhardtMay 12, 2025
  37. shejialuoMay 12, 2025
  38. Patrick SteinhardtMay 12, 2025
  39. Jeff KingMay 12, 2025
  40. Junio C HamanoMay 12, 2025
  41. Patrick SteinhardtMay 13, 2025
  42. shejialuoMay 13, 2025
  43. 2/3 packed-backend: extract snapshot allocation in `load_contents`shejialuo, May 11, 2025
  44. Patrick SteinhardtMay 12, 2025
  45. shejialuoMay 12, 2025
  46. Patrick SteinhardtMay 12, 2025
  47. Jeff KingMay 12, 2025
  48. shejialuoMay 13, 2025
  49. 3/3 packed-backend: mmap large "packed-refs" file during fsckshejialuo, May 11, 2025
  50. Jeff KingMay 12, 2025
  51. 0/3 align the behavior when opening "packed-refs"shejialuo, May 13, 2025
  52. 1/3 packed-backend: fsck should warn when "packed-refs" file is emptyshejialuo, May 13, 2025
  53. Junio C HamanoMay 13, 2025
  54. shejialuoMay 14, 2025
  55. 3/3 packed-backend: mmap large "packed-refs" file during fsckshejialuo, May 13, 2025
  56. Junio C HamanoMay 13, 2025
  57. shejialuoMay 14, 2025
  58. 2/3 packed-backend: extract snapshot allocation in `load_contents`shejialuo, May 13, 2025
  59. 0/3 align the behavior when opening "packed-refs"shejialuo, May 14, 2025
  60. 2/3 packed-backend: extract snapshot allocation in `load_contents`shejialuo, May 14, 2025
  61. 1/3 packed-backend: fsck should warn when "packed-refs" file is emptyshejialuo, May 14, 2025
  62. 3/3 packed-backend: mmap large "packed-refs" file during fsckshejialuo, May 14, 2025
  63. Junio C HamanoMay 15, 2025
  64. Junio C HamanoMay 21, 2025
  65. Jeff KingMay 22, 2025
  66. Patrick SteinhardtMay 23, 2025
  67. Junio C HamanoMay 23, 2025

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.