Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro

[Taylor Blau <me@xxxxxxxxxxxx>]

On Wed, Sep 06, 2023 at 10:05:37AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
> > ---
> >  builtin/repack.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
>
> The reason being...?

Wow, I have no idea how this got sent out without a commit message! At
least it was signed off ;-).

Here's a replacement that I cooked up, with your Helped-by. Let me know
if you want to replace this patch manually, or if you'd prefer a reroll
of the series. Either is fine with me! :-)

--- 8< ---
Subject: [PATCH] builtin/repack.c: treat string_list_item util as booleans

The `->util` field corresponding to each string_list_item used to track
the existence of some pack at the beginning of a repack operation was
originally intended to be used as a bitfield.

This bitfield tracked:

  - (1 << 0): whether or not the pack should be deleted
  - (1 << 1): whether or not the pack is cruft

The previous commit removed the use of the second bit, meaning that we
can treat this field as a boolean instead of a bitset.

Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
 builtin/repack.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 478fab96c9..575e69b020 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -26,7 +26,7 @@
 #define LOOSEN_UNREACHABLE 2
 #define PACK_CRUFT 4

-#define DELETE_PACK 1
+#define DELETE_PACK ((void*)(uintptr_t)1)

 static int pack_everything;
 static int delta_base_offset = 1;
@@ -96,6 +96,10 @@ static int repack_config(const char *var, const char *value,

 struct existing_packs {
 	struct string_list kept_packs;
+	/*
+	 * for both non_kept_packs, and cruft_packs, a non-NULL
+	 * 'util' field indicates the pack should be deleted.
+	 */
 	struct string_list non_kept_packs;
 	struct string_list cruft_packs;
 };
@@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
 		 * (if `-d` was given).
 		 */
 		if (!string_list_has_string(names, sha1))
-			item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
+			item->util = DELETE_PACK;
 	}
 }

@@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)
 {
 	struct string_list_item *item;
 	for_each_string_list_item(item, packs) {
-		if (!((uintptr_t)item->util & DELETE_PACK))
+		if (!item->util)
 			continue;
 		remove_redundant_pack(packdir, item->string);
 	}
@@ -695,20 +699,20 @@ static void midx_included_packs(struct string_list *include,

 		for_each_string_list_item(item, &existing->cruft_packs) {
 			/*
-			 * no need to check DELETE_PACK, since we're not
-			 * doing an ALL_INTO_ONE repack
+			 * no need to check for deleted packs, since we're
+			 * not doing an ALL_INTO_ONE repack
 			 */
 			string_list_insert(include, xstrfmt("%s.idx", item->string));
 		}
 	} else {
 		for_each_string_list_item(item, &existing->non_kept_packs) {
-			if ((uintptr_t)item->util & DELETE_PACK)
+			if (item->util)
 				continue;
 			string_list_insert(include, xstrfmt("%s.idx", item->string));
 		}

 		for_each_string_list_item(item, &existing->cruft_packs) {
-			if ((uintptr_t)item->util & DELETE_PACK)
+			if (item->util)
 				continue;
 			string_list_insert(include, xstrfmt("%s.idx", item->string));
 		}
--
2.38.0.16.g393fd4c6db

--- >8 ---

Thanks,
Taylor