Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 14, 2023, 18:01 UTC
Message-ID: <ZQNKkn0YYLUyN5Ih@nand.local>
In-Reply-To: <CAP8UFD1rDb-iYf4LYb7n=K4KpQ-JR-JK4TkQpGJ-TCfTNFFbnA@mail.gmail.com>

On Thu, Sep 14, 2023 at 01:10:38PM +0200, Christian Couder wrote:

> Ok, I will try to review and merge this with
> cc/repack-sift-filtered-objects-to-separate-pack soon.

I took a look at how much/little effort was going to be required, and luckily the changes are isolated only to a single patch. It's just your f1ffa71e8f (repack: add `--filter=<filter-spec>` option, 2023-09-11), and in particular the `write_filtered_pack()` function.

I started messing around with it myself and generated the following fixup! which can be applied on top of your version of f1ffa71e8f. It's mostly straightforward, but there is a gotcha that the loop over non-kept packs has to change to:

    for_each_string_list_item(item, &existing->non_kept_packs)
            /* ... */
    for_each_string_list_item(item, &existing->cruft_packs)
            /* ... */

, instead of just the first loop over non_kept_packs, since cruft packs are stored in a separate list.

In any event, here's the fixup! I generated on top of that patch:

--- 8< ---
Subject: [PATCH] fixup! repack: add `--filter=<filter-spec>` option

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 120f4241c0..0d23323d05 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -834,15 +834,13 @@ static int finish_pack_objects_cmd(struct child_process *cmd,
 static int write_filtered_pack(const struct pack_objects_args *args,
 			       const char *destination,
 			       const char *pack_prefix,
-			       struct string_list *keep_pack_list,
-			       struct string_list *names,
-			       struct string_list *existing_packs,
-			       struct string_list *existing_kept_packs)
+			       struct existing_packs *existing,
+			       struct string_list *names)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct string_list_item *item;
 	FILE *in;
-	int ret, i;
+	int ret;
 	const char *caret;
 	const char *scratch;
 	int local = skip_prefix(destination, packdir, &scratch);
@@ -853,9 +851,8 @@ static int write_filtered_pack(const struct pack_objects_args *args,

 	if (!pack_kept_objects)
 		strvec_push(&cmd.args, "--honor-pack-keep");
-	for (i = 0; i < keep_pack_list->nr; i++)
-		strvec_pushf(&cmd.args, "--keep-pack=%s",
-			     keep_pack_list->items[i].string);
+	for_each_string_list_item(item, &existing->kept_packs)
+		strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);

 	cmd.in = -1;

@@ -872,10 +869,12 @@ static int write_filtered_pack(const struct pack_objects_args *args,
 	in = xfdopen(cmd.in, "w");
 	for_each_string_list_item(item, names)
 		fprintf(in, "^%s-%s.pack\n", pack_prefix, item->string);
-	for_each_string_list_item(item, existing_packs)
+	for_each_string_list_item(item, &existing->non_kept_packs)
+		fprintf(in, "%s.pack\n", item->string);
+	for_each_string_list_item(item, &existing->cruft_packs)
 		fprintf(in, "%s.pack\n", item->string);
 	caret = pack_kept_objects ? "" : "^";
-	for_each_string_list_item(item, existing_kept_packs)
+	for_each_string_list_item(item, &existing->kept_packs)
 		fprintf(in, "%s%s.pack\n", caret, item->string);
 	fclose(in);

@@ -1261,10 +1260,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		ret = write_filtered_pack(&po_args,
 					  packtmp,
 					  find_pack_prefix(packdir, packtmp),
-					  &keep_pack_list,
-					  &names,
-					  &existing_nonkept_packs,
-					  &existing_kept_packs);
+					  &existing,
+					  &names);
 		if (ret)
 			goto cleanup;
 	}
--
2.42.0.137.g6fe1dff026
--- >8 ---

Thanks,
Taylor