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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]

On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:
> 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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]