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

From: Jeff King <peff@peff.net>
Date: Sep 7, 2023, 08:58 UTC
Message-ID: <20230907085828.GG1260718@coredump.intra.peff.net>
In-Reply-To: <ZPi1c98o2fKB/U+e@nand.local>

On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:

Show 14 quoted lines

> --- 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.

I do think the boolean is syntactically a little nicer than the bit-set, just because of the casting we have to with the void pointer). After reading the earlier parts, I was hoping the culmination of this series would be dropping the use of util entirely (presumably in favor of separate lists). But maybe that would be too disruptive; I didn't try it.

Show 11 quoted lines

> -#define DELETE_PACK 1
> +#define DELETE_PACK ((void*)(uintptr_t)1)
> [...]
> @@ -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;
>  	}
>  }

I do like the use of the macro here to make the meaning of the boolean more plain, since the name "util" is totally meaningless (but we are stuck with it).

But on the other side, things get more mysterious:

Show 7 quoted lines

> @@ -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;

This is syntactically much nicer, but the meaning of "util" as a boolean for "we should delete this" is lost.

So I dunno. The end result of this patch is more readable syntactically, but arguably less so semantically. Unless we have a good reason to ditch the bit-set entirely, I wonder if we could have the best of both with some macro helpers. Or even a set of matching helper functions like:

  void pack_mark_for_deletion(struct string_list_item *item)
  {
	(uintptr_t)item->util = 1;
  }

  int pack_should_deleted(const struct string_list_item *item)
  {
	return item->util;
  }

which could be a bool or a bit-set; the callers no longer need to care and get to use human-readable names.

I dunno. It is a file-local convention and there aren't that many spots, so maybe it is not worth worrying too much about. I'm pretty sure that I got confused by the use of "util" here when looking at the code before, as it did not have the DELETE_PACK name until your 72263ffc32 (builtin/repack.c: use named flags for existing_packs, 2022-05-20). But maybe the comment that you added is sufficient.

If we had generic pointer-bitset macros, then perhaps other string-list users could benefit, too. I thought maybe fast-export could use this for its mark_to_ptr() stuff, but that is storing a whole 32-bit value, not a bitset. So maybe this is just a weird localized thing.

A more radical idea is that we don't care very much about the data structure as long as it is ordered. So we could just do:

  struct existing_pack {
          struct list_head list;
          int to_delete;
          char name[FLEX_ARRAY];
  };

and ditch string-list entirely. That lets us use "to_delete" in a natural way. Though I suspect it makes all the _other_ code unreadable, as we have to allocate them, deal with list_entry(), and so on.

So I guess I'm fine with any path (including the one in this patch).

-Peff