Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 7, 2023, 08:19 UTC
- Message-ID
- <ZPmHpqHNzXF0Jbu6@tanuki>
- In-Reply-To
- <ZPi1c98o2fKB/U+e@nand.local>
On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:
Show 16 quoted lines
> On Wed, Sep 06, 2023 at 10:05:37AM -0700, Junio C Hamano wrote: > > Taylor Blau <me@ttaylorr.com> writes: > > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > > --- > > > 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! :-)
Personally I think that the original version is still more readable. If you simply see `if (item->util)` you don't really have any indicator what this actually means, whereas previously the more verbose check for `if ((uintptr)item->util & DELETE_PACK)` gives the reader a nice hint what this may be about.
If the intent is to make this check a bit prettier, how about we instead introduce a helper function like the following:
```
static inline int pack_marked_for_deletion(const struct string_list_item *item)
{
return (uintptr) item->util & DELETE_PACK;
}
```Other than that the rest of this series looks good to me, thanks.
Patrick
Show 95 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.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> 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