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

Re: [PATCH 21/49] builtin/repack.c: factor our "generated_pack_install"

From
Taylor Blau <me@ttaylorr.com>
Date
Oct 10, 2025, 22:58 UTC
Message-ID
<aOmPg7Pb9RqOEuZ5@nand.local>
In-Reply-To
<20251010061449.GH1965904@coredump.intra.peff.net>
On Fri, Oct 10, 2025 at 02:14:49AM -0400, Jeff King wrote:
Show 26 quoted lines
> On Tue, Oct 07, 2025 at 04:26:26PM -0400, Taylor Blau wrote:
>
> > > > +	for_each_string_list_item(item, &names)
> > > > +		generated_pack_install((struct generated_pack *)item->util,
> > >
> > > This cast should be unnecessary, right? `item->util` is a void pointer,
> > > so C should do the cast implicitly.
> >
> > It's unnecessary, but I dislike implicit casts from 'void*' to any other
> > type. This makes it clearer how we're supposed to interpret the value in
> > item->util, but I'm happy to change it to use the implicit cast if you
> > feel strongly about it.
>
> I tend to avoid casts when possible, because they can mask unexpected
> conversions. E.g., if item->util's type changed, your cast means we
> would never notice it.
>
> But that cuts both ways. If the prototype for generated_pack_install()
> changes, it is only your cast that would tell the compiler that this
> caller needs to be updated. Ultimately it is the spot that _sets_
> item->util that needs to change, but hopefully flagging this spot would
> point us in the right direction.
>
> And that does seem more likely than item->util changing away from void.
> (To be clear, I don't think either is that likely, I'm just musing on
> whether casts like this are helpful in general).

OK, that's a very compelling argument. I'll drop the cast on my local version.

(As a side note, it doesn't look like we mention this convention in our CodingGuidelines, and we probably should, including the rationale that you gave above here. That seems like another good candidate for #leftoverbits.)

Thanks, Taylor

Previous: Jeff KingNext: Taylor Blau
Message 49 of 163 in “repack: prepare for incremental MIDX-based repacking”
  1. 00/49 repack: prepare for incremental MIDX-based repackingTaylor Blau, Sep 28, 2025
  2. 01/49 builtin/repack.c: avoid "the_repository" in `cmd_repack()`Taylor Blau, Sep 28, 2025
  3. 02/49 builtin/repack.c: avoid "the_repository" in existing packs APITaylor Blau, Sep 28, 2025
  4. Jeff KingOct 10, 2025
  5. Taylor BlauOct 10, 2025
  6. 03/49 builtin/repack.c: avoid "the_repository" when taking a ref snapshotTaylor Blau, Sep 28, 2025
  7. 04/49 builtin/repack.c: avoid "the_repository" when removing packsTaylor Blau, Sep 28, 2025
  8. Jeff KingOct 10, 2025
  9. Taylor BlauOct 10, 2025
  10. 05/49 builtin/repack.c: avoid "the_repository" when repacking promisor objectsTaylor Blau, Sep 28, 2025
  11. 06/49 builtin/repack.c: avoid "the_hash_algo" when deleting packsTaylor Blau, Sep 28, 2025
  12. 07/49 builtin/repack.c: avoid "the_hash_algo" in `write_oid()`Taylor Blau, Sep 28, 2025
  13. Patrick SteinhardtSep 29, 2025
  14. Taylor BlauOct 7, 2025
  15. 08/49 builtin/repack: avoid "the_hash_algo" in `repack_promisor_objects()`Taylor Blau, Sep 28, 2025
  16. 09/49 builtin/repack.c: avoid "the_hash_algo" in `finish_pack_objects_cmd()`Taylor Blau, Sep 28, 2025
  17. Jeff KingOct 10, 2025
  18. Taylor BlauOct 10, 2025
  19. 10/49 builtin/repack.c: avoid using `hash_to_hex()` in pack geometryTaylor Blau, Sep 28, 2025
  20. Patrick SteinhardtSep 29, 2025
  21. 11/49 repack: introduce new compilation unitTaylor Blau, Sep 28, 2025
  22. Jeff KingOct 10, 2025
  23. Taylor BlauOct 10, 2025
  24. 12/49 builtin/repack.c: pass both pack_objects args to repack_configTaylor Blau, Sep 28, 2025
  25. Patrick SteinhardtSep 29, 2025
  26. 13/49 repack: move 'delta_base_offset' to 'struct pack_objects_args'Taylor Blau, Sep 28, 2025
  27. Jeff KingOct 10, 2025
  28. Taylor BlauOct 10, 2025
  29. 14/49 repack: remove 'prepare_pack_objects' from the builtinTaylor Blau, Sep 28, 2025
  30. 15/49 builtin/repack.c: rename many 'struct existing_packs' functionsTaylor Blau, Sep 28, 2025
  31. 16/49 repack: remove 'remove_redundant_pack' from the builtinTaylor Blau, Sep 28, 2025
  32. 17/49 builtin/repack.c: pass "packdir" when removing packsTaylor Blau, Sep 28, 2025
  33. Patrick SteinhardtSep 29, 2025
  34. Taylor BlauOct 7, 2025
  35. Jeff KingOct 10, 2025
  36. Taylor BlauOct 10, 2025
  37. 18/49 builtin/repack.c: avoid unnecessary numeric casts in existing_packsTaylor Blau, Sep 28, 2025
  38. 19/49 repack: remove 'existing_packs' API from the builtinTaylor Blau, Sep 28, 2025
  39. Patrick SteinhardtSep 29, 2025
  40. Taylor BlauOct 7, 2025
  41. Jeff KingOct 10, 2025
  42. Taylor BlauOct 10, 2025
  43. 20/49 builtin/repack.c: rename "struct generated_pack_data"Taylor Blau, Sep 28, 2025
  44. 21/49 builtin/repack.c: factor our "generated_pack_install"Taylor Blau, Sep 28, 2025
  45. Patrick SteinhardtSep 29, 2025
  46. Taylor BlauOct 7, 2025
  47. Patrick SteinhardtOct 8, 2025
  48. Jeff KingOct 10, 2025
  49. Taylor BlauOct 10, 2025
  50. 22/49 builtin/repack.c: pass "packtmp" to `generated_pack_populate()`Taylor Blau, Sep 28, 2025
  51. Patrick SteinhardtSep 29, 2025
  52. Taylor BlauOct 7, 2025
  53. Patrick SteinhardtOct 8, 2025
  54. 23/49 builtin/repack.c: provide pack locations to `generated_pack_install()`Taylor Blau, Sep 28, 2025
  55. 24/49 repack: remove 'generated_pack' API from the builtinTaylor Blau, Sep 28, 2025
  56. 25/49 builtin/repack.c: pass "packtmp" to `repack_promisor_objects()`Taylor Blau, Sep 28, 2025
  57. 26/49 builtin/repack.c: remove "repack_promisor_objects()" from the builtinTaylor Blau, Sep 28, 2025
  58. Patrick SteinhardtSep 29, 2025
  59. Taylor BlauOct 7, 2025
  60. 27/49 builtin/repack.c: rename various pack_geometry functionsTaylor Blau, Sep 28, 2025
  61. Patrick SteinhardtSep 29, 2025
  62. Taylor BlauOct 7, 2025
  63. Patrick SteinhardtOct 8, 2025
  64. Taylor BlauOct 10, 2025
  65. 28/49 builtin/repack.c: pass 'pack_kept_objects' to `pack_geometry_init()`Taylor Blau, Sep 28, 2025
  66. 29/49 builtin/repack.c: pass 'packdir' to `pack_geometry_remove_redundant()`Taylor Blau, Sep 28, 2025
  67. 30/49 repack: remove pack_geometry API from the builtinTaylor Blau, Sep 28, 2025
  68. 31/49 builtin/repack.c: remove ref snapshotting from builtinTaylor Blau, Sep 28, 2025
  69. 32/49 builtin/repack.c: extract opts struct for 'write_midx_included_packs()'Taylor Blau, Sep 28, 2025
  70. 33/49 builtin/repack.c: use a string_list for 'midx_pack_names'Taylor Blau, Sep 28, 2025
  71. 34/49 repack: keep track of MIDX pack names using existing_packsTaylor Blau, Sep 28, 2025
  72. 35/49 builtin/repack.c: reorder `remove_redundant_bitmaps()`Taylor Blau, Sep 28, 2025
  73. 36/49 builtin/repack.c: inline `remove_redundant_bitmaps()`Taylor Blau, Sep 28, 2025
  74. 37/49 builtin/repack.c: pass `repack_write_midx_opts` to `midx_included_packs`Taylor Blau, Sep 28, 2025
  75. 38/49 builtin/repack.c: inline packs within `write_midx_included_packs()`Taylor Blau, Sep 28, 2025
  76. Patrick SteinhardtOct 10, 2025
  77. Taylor BlauOct 10, 2025
  78. Jeff KingOct 15, 2025
  79. 39/49 repack: 'write_midx_included_packs' API from the builtinTaylor Blau, Sep 28, 2025
  80. 40/49 builtin/repack.c: introduce `struct write_pack_opts`Taylor Blau, Sep 28, 2025
  81. Patrick SteinhardtOct 10, 2025
  82. Taylor BlauOct 10, 2025
  83. Jeff KingOct 15, 2025
  84. Taylor BlauOct 15, 2025
  85. 41/49 builtin/repack.c: use `write_pack_opts` within `write_cruft_pack()`Taylor Blau, Sep 28, 2025
  86. Patrick SteinhardtOct 10, 2025
  87. Taylor BlauOct 10, 2025
  88. Jeff KingOct 15, 2025
  89. 42/49 repack: move `find_pack_prefix()` out of the builtinTaylor Blau, Sep 28, 2025
  90. Jeff KingOct 15, 2025
  91. Taylor BlauOct 15, 2025
  92. 43/49 repack: extract `write_pack_opts_is_local()`Taylor Blau, Sep 28, 2025
  93. Patrick SteinhardtOct 10, 2025
  94. Taylor BlauOct 15, 2025
  95. Jeff KingOct 15, 2025
  96. Taylor BlauOct 15, 2025
  97. 44/49 builtin/repack.c: pass `write_pack_opts` to `finish_pack_objects_cmd()`Taylor Blau, Sep 28, 2025
  98. Jeff KingOct 15, 2025
  99. Taylor BlauOct 15, 2025
  100. 45/49 repack: move `finish_pack_objects_cmd()` out of the builtinTaylor Blau, Sep 28, 2025
  101. 46/49 repack: move `pack_kept_objects` to `struct pack_objects_args`Taylor Blau, Sep 28, 2025
  102. 47/49 repack: move `write_filtered_pack()` out of the builtinTaylor Blau, Sep 28, 2025
  103. 48/49 repack: move `write_cruft_pack()` out of the builtinTaylor Blau, Sep 28, 2025
  104. 49/49 builtin/repack.c: clean up unused `#include`sTaylor Blau, Sep 28, 2025
  105. Junio C HamanoSep 28, 2025
  106. Taylor BlauOct 7, 2025
  107. Jeff KingOct 10, 2025
  108. Taylor BlauOct 10, 2025
  109. Jeff KingOct 15, 2025
  110. 00/49 repack: prepare for incremental MIDX-based repackingTaylor Blau, Oct 15, 2025
  111. 01/49 builtin/repack.c: avoid "the_repository" in `cmd_repack()`Taylor Blau, Oct 15, 2025
  112. 02/49 builtin/repack.c: avoid "the_repository" in existing packs APITaylor Blau, Oct 15, 2025
  113. 03/49 builtin/repack.c: avoid "the_repository" when taking a ref snapshotTaylor Blau, Oct 15, 2025
  114. 04/49 builtin/repack.c: avoid "the_repository" when removing packsTaylor Blau, Oct 15, 2025
  115. 05/49 builtin/repack.c: avoid "the_repository" when repacking promisor objectsTaylor Blau, Oct 15, 2025
  116. 06/49 builtin/repack.c: avoid "the_hash_algo" when deleting packsTaylor Blau, Oct 15, 2025
  117. 07/49 builtin/repack.c: avoid "the_hash_algo" in `write_oid()`Taylor Blau, Oct 15, 2025
  118. 08/49 builtin/repack: avoid "the_hash_algo" in `repack_promisor_objects()`Taylor Blau, Oct 15, 2025
  119. 09/49 builtin/repack.c: avoid "the_hash_algo" in `finish_pack_objects_cmd()`Taylor Blau, Oct 15, 2025
  120. 10/49 builtin/repack.c: avoid using `hash_to_hex()` in pack geometryTaylor Blau, Oct 15, 2025
  121. 11/49 repack: introduce new compilation unitTaylor Blau, Oct 15, 2025
  122. 12/49 builtin/repack.c: pass both pack_objects args to repack_configTaylor Blau, Oct 15, 2025
  123. 13/49 repack: move 'delta_base_offset' to 'struct pack_objects_args'Taylor Blau, Oct 15, 2025
  124. 14/49 repack: remove 'prepare_pack_objects' from the builtinTaylor Blau, Oct 15, 2025
  125. 15/49 builtin/repack.c: rename many 'struct existing_packs' functionsTaylor Blau, Oct 15, 2025
  126. 16/49 repack: remove 'remove_redundant_pack' from the builtinTaylor Blau, Oct 15, 2025
  127. 17/49 builtin/repack.c: pass "packdir" when removing packsTaylor Blau, Oct 15, 2025
  128. 18/49 builtin/repack.c: avoid unnecessary numeric casts in existing_packsTaylor Blau, Oct 15, 2025
  129. 19/49 repack: remove 'existing_packs' API from the builtinTaylor Blau, Oct 15, 2025
  130. 20/49 builtin/repack.c: rename "struct generated_pack_data"Taylor Blau, Oct 15, 2025
  131. 21/49 builtin/repack.c: factor out "generated_pack_install"Taylor Blau, Oct 15, 2025
  132. 22/49 builtin/repack.c: pass "packtmp" to `generated_pack_populate()`Taylor Blau, Oct 15, 2025
  133. 23/49 builtin/repack.c: provide pack locations to `generated_pack_install()`Taylor Blau, Oct 15, 2025
  134. 24/49 repack: remove 'generated_pack' API from the builtinTaylor Blau, Oct 15, 2025
  135. 25/49 builtin/repack.c: pass "packtmp" to `repack_promisor_objects()`Taylor Blau, Oct 15, 2025
  136. 26/49 builtin/repack.c: remove "repack_promisor_objects()" from the builtinTaylor Blau, Oct 15, 2025
  137. 27/49 builtin/repack.c: rename various pack_geometry functionsTaylor Blau, Oct 15, 2025
  138. 28/49 builtin/repack.c: pass 'pack_kept_objects' to `pack_geometry_init()`Taylor Blau, Oct 15, 2025
  139. 29/49 builtin/repack.c: pass 'packdir' to `pack_geometry_remove_redundant()`Taylor Blau, Oct 15, 2025
  140. 30/49 repack: remove pack_geometry API from the builtinTaylor Blau, Oct 15, 2025
  141. 31/49 builtin/repack.c: remove ref snapshotting from builtinTaylor Blau, Oct 15, 2025
  142. 32/49 builtin/repack.c: extract opts struct for 'write_midx_included_packs()'Taylor Blau, Oct 15, 2025
  143. 33/49 builtin/repack.c: use a string_list for 'midx_pack_names'Taylor Blau, Oct 15, 2025
  144. 34/49 repack: keep track of MIDX pack names using existing_packsTaylor Blau, Oct 15, 2025
  145. 35/49 builtin/repack.c: reorder `remove_redundant_bitmaps()`Taylor Blau, Oct 15, 2025
  146. 36/49 builtin/repack.c: inline `remove_redundant_bitmaps()`Taylor Blau, Oct 15, 2025
  147. 37/49 builtin/repack.c: pass `repack_write_midx_opts` to `midx_included_packs`Taylor Blau, Oct 15, 2025
  148. 38/49 builtin/repack.c: inline packs within `write_midx_included_packs()`Taylor Blau, Oct 15, 2025
  149. 39/49 repack: 'write_midx_included_packs' API from the builtinTaylor Blau, Oct 15, 2025
  150. 40/49 builtin/repack.c: introduce `struct write_pack_opts`Taylor Blau, Oct 15, 2025
  151. 41/49 builtin/repack.c: use `write_pack_opts` within `write_cruft_pack()`Taylor Blau, Oct 15, 2025
  152. 42/49 repack: move `find_pack_prefix()` out of the builtinTaylor Blau, Oct 15, 2025
  153. 43/49 repack: extract `write_pack_opts_is_local()`Taylor Blau, Oct 15, 2025
  154. 44/49 builtin/repack.c: pass `write_pack_opts` to `finish_pack_objects_cmd()`Taylor Blau, Oct 15, 2025
  155. 45/49 repack: move `finish_pack_objects_cmd()` out of the builtinTaylor Blau, Oct 15, 2025
  156. 46/49 repack: move `pack_kept_objects` to `struct pack_objects_args`Taylor Blau, Oct 15, 2025
  157. 47/49 repack: move `write_filtered_pack()` out of the builtinTaylor Blau, Oct 15, 2025
  158. 48/49 repack: move `write_cruft_pack()` out of the builtinTaylor Blau, Oct 15, 2025
  159. 49/49 builtin/repack.c: clean up unused `#include`sTaylor Blau, Oct 15, 2025
  160. Patrick SteinhardtOct 16, 2025
  161. Junio C HamanoOct 16, 2025
  162. Taylor BlauOct 16, 2025
  163. Jeff KingOct 17, 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.