Re: [PATCH] repack: free geometry struct
From: Taylor Blau <hidden>
Date: 2023-08-08 20:34:13
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, Aug 08, 2023 at 02:50:23PM -0400, Jeff King wrote:
Another option would be to put the struct on the stack rather than the heap. However, this gets tricky, as we check the pointer against NULL in several places to decide whether we're in geometric mode.
The patch you have here looks good, but...
I did actually put together the "put it on the stack" patch, which swaps out: if (geometry) for: if (geometric_factor) to get around the NULL checks. But besides being noisy for little gain, it ends up with some subtle gotchas, because we pass "&geometry" into some functions which don't have access to "geometric_factor" (so now extra call-sites have to keep track of "is this struct valid enough to pass").
...I think that storing the geometric_factor value on the pack_geometry struct itself gets around most of these issues. This version is still somewhat noisy, but it's not too bad IMHO. I'm fine with either your approach or mine :-).
--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index aea5ca9d44..13e4f0094e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c@@ -303,6 +303,8 @@ struct pack_geometry { struct packed_git **pack; uint32_t pack_nr, pack_alloc; uint32_t split; + + int split_factor; }; static uint32_t geometry_pack_weight(struct packed_git *p)
@@ -324,17 +326,13 @@ static int geometry_cmp(const void *va, const void *vb) return 0; } -static void init_pack_geometry(struct pack_geometry **geometry_p, +static void init_pack_geometry(struct pack_geometry *geometry, struct string_list *existing_kept_packs, const struct pack_objects_args *args) { struct packed_git *p; - struct pack_geometry *geometry; struct strbuf buf = STRBUF_INIT; - *geometry_p = xcalloc(1, sizeof(struct pack_geometry)); - geometry = *geometry_p; - for (p = get_all_packs(the_repository); p; p = p->next) { if (args->local && !p->pack_local) /*
@@ -380,7 +378,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p, strbuf_release(&buf); } -static void split_pack_geometry(struct pack_geometry *geometry, int factor) +static void split_pack_geometry(struct pack_geometry *geometry) { uint32_t i; uint32_t split;
@@ -399,12 +397,14 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor) struct packed_git *ours = geometry->pack[i]; struct packed_git *prev = geometry->pack[i - 1]; - if (unsigned_mult_overflows(factor, geometry_pack_weight(prev))) + if (unsigned_mult_overflows(geometry->split_factor, + geometry_pack_weight(prev))) die(_("pack %s too large to consider in geometric " "progression"), prev->pack_name); - if (geometry_pack_weight(ours) < factor * geometry_pack_weight(prev)) + if (geometry_pack_weight(ours) < + geometry->split_factor * geometry_pack_weight(prev)) break; }
@@ -439,10 +439,12 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor) for (i = split; i < geometry->pack_nr; i++) { struct packed_git *ours = geometry->pack[i]; - if (unsigned_mult_overflows(factor, total_size)) + if (unsigned_mult_overflows(geometry->split_factor, + total_size)) die(_("pack %s too large to roll up"), ours->pack_name); - if (geometry_pack_weight(ours) < factor * total_size) { + if (geometry_pack_weight(ours) < + geometry->split_factor * total_size) { if (unsigned_add_overflows(total_size, geometry_pack_weight(ours))) die(_("pack %s too large to roll up"),
@@ -781,7 +783,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) struct string_list names = STRING_LIST_INIT_DUP; struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP; struct string_list existing_kept_packs = STRING_LIST_INIT_DUP; - struct pack_geometry *geometry = NULL; + struct pack_geometry geometry = { 0 }; struct strbuf line = STRBUF_INIT; struct tempfile *refs_snapshot = NULL; int i, ext, ret;
@@ -795,7 +797,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix) struct string_list keep_pack_list = STRING_LIST_INIT_NODUP; struct pack_objects_args po_args = {NULL}; struct pack_objects_args cruft_po_args = {NULL}; - int geometric_factor = 0; int write_midx = 0; const char *cruft_expiration = NULL; const char *expire_to = NULL;
@@ -844,7 +845,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) N_("repack objects in packs marked with .keep")), OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"), N_("do not repack this pack")), - OPT_INTEGER('g', "geometric", &geometric_factor, + OPT_INTEGER('g', "geometric", &geometry.split_factor, N_("find a geometric progression with factor <N>")), OPT_BOOL('m', "write-midx", &write_midx, N_("write a multi-pack index of the resulting packs")),
@@ -920,11 +921,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix) collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs, &keep_pack_list); - if (geometric_factor) { + if (geometry.split_factor) { if (pack_everything) die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a"); init_pack_geometry(&geometry, &existing_kept_packs, &po_args); - split_pack_geometry(geometry, geometric_factor); + split_pack_geometry(&geometry); } prepare_pack_objects(&cmd, &po_args, packtmp);
@@ -938,7 +939,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) strvec_pushf(&cmd.args, "--keep-pack=%s", keep_pack_list.items[i].string); strvec_push(&cmd.args, "--non-empty"); - if (!geometry) { + if (!geometry.split_factor) { /* * We need to grab all reachable objects, including those that * are reachable from reflogs and the index.
@@ -985,7 +986,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) strvec_push(&cmd.args, "--pack-loose-unreachable"); } } - } else if (geometry) { + } else if (geometry.split_factor) { strvec_push(&cmd.args, "--stdin-packs"); strvec_push(&cmd.args, "--unpacked"); } else {
@@ -993,7 +994,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) strvec_push(&cmd.args, "--incremental"); } - if (geometry) + if (geometry.split_factor) cmd.in = -1; else cmd.no_stdin = 1;
@@ -1002,17 +1003,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (ret) goto cleanup; - if (geometry) { + if (geometry.split_factor) { FILE *in = xfdopen(cmd.in, "w"); /* * The resulting pack should contain all objects in packs that * are going to be rolled up, but exclude objects in packs which * are being left alone. */ - for (i = 0; i < geometry->split; i++) - fprintf(in, "%s\n", pack_basename(geometry->pack[i])); - for (i = geometry->split; i < geometry->pack_nr; i++) - fprintf(in, "^%s\n", pack_basename(geometry->pack[i])); + for (i = 0; i < geometry.split; i++) + fprintf(in, "%s\n", pack_basename(geometry.pack[i])); + for (i = geometry.split; i < geometry.pack_nr; i++) + fprintf(in, "^%s\n", pack_basename(geometry.pack[i])); fclose(in); }
@@ -1155,9 +1156,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (write_midx) { struct string_list include = STRING_LIST_INIT_NODUP; midx_included_packs(&include, &existing_nonkept_packs, - &existing_kept_packs, &names, geometry); + &existing_kept_packs, &names, &geometry); - ret = write_midx_included_packs(&include, geometry, + ret = write_midx_included_packs(&include, &geometry, refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL, show_progress, write_bitmaps > 0);
@@ -1180,12 +1181,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix) remove_redundant_pack(packdir, item->string); } - if (geometry) { + if (geometry.split_factor) { struct strbuf buf = STRBUF_INIT; uint32_t i; - for (i = 0; i < geometry->split; i++) { - struct packed_git *p = geometry->pack[i]; + for (i = 0; i < geometry.split; i++) { + struct packed_git *p = geometry.pack[i]; if (string_list_has_string(&names, hash_to_hex(p->hash))) continue;
@@ -1228,7 +1229,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) string_list_clear(&names, 1); string_list_clear(&existing_nonkept_packs, 0); string_list_clear(&existing_kept_packs, 0); - clear_pack_geometry(geometry); + clear_pack_geometry(&geometry); return ret; } --- >8 ---
Thanks, Taylor