Re: [PATCH 4/5] midx-write: use uint32_t for preferred_pack_idx
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Aug 29, 2025, 01:35 UTC
- Message-ID
- <aLEDzF3b6tax6Lnq@nand.local>
- In-Reply-To
- <bd97db26f7f789315134dc796403b1ab9976135b.1756402795.git.gitgitgadget@gmail.com>
On Thu, Aug 28, 2025 at 05:39:54PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 15 quoted lines
> From: Derrick Stolee <stolee@gmail.com> > > midx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a > few reasons, but the biggest one is the use of a signed > preferred_pack_idx member inside the write_midx_context struct. The code > currently uses -1 to indicate an unset preferred pack but pack int ids > are normally handled as uint32_t. There are also a few loops that search > for the preferred pack by name and those iterators will need updates to > uint32_t in the next change. > > For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an > equality check. The macro stores the max value of a uint32_t, so we > cannot store a preferred pack that appears last in a list of 2^32 total > packs, but that's expected to be unreasonable already. This improves the > range from 2^31 already.
Nice ;-).
Show 19 quoted lines
> There are some careful things to worry about with initializing the > preferred pack in the struct and using that value when searching for a > preferred pack that was already incorrect but accidentally working when > the index was initialized to zero. > > Signed-off-by: Derrick Stolee <stolee@gmail.com> > --- > midx-write.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/midx-write.c b/midx-write.c > index f2d9a990e6..ea1b3a199c 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -24,6 +24,7 @@ > #define BITMAP_POS_UNKNOWN (~((uint32_t)0)) > #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256) > #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t)) > +#define NO_PREFERRED_PACK (~((uint32_t)0))
I think that just (~(uint32_t)0) is necessary, but I don't mind the extra clarity.
Show 7 quoted lines
> @@ -274,7 +275,7 @@ static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,
> end = m->num_objects_in_base + ntohl(m->chunk_oid_fanout[cur_fanout]);
>
> for (cur_object = start; cur_object < end; cur_object++) {
> - if ((preferred_pack > -1) &&
> + if ((preferred_pack != NO_PREFERRED_PACK) &&
> (preferred_pack == nth_midxed_pack_int_id(m, cur_object))) {OK, here we should make sure that the preferred pack was provided.
Like you mentioned above, I guess that means we can't have the 2^32-1'st pack, but before we couldn't have any pack greater than 2^31-1 preferred, so this is a strict improvement ;-).
Show 9 quoted lines
> /* > * Objects from preferred packs are added > @@ -364,7 +365,8 @@ static void compute_sorted_entries(struct write_midx_context *ctx, > preferred, cur_fanout); > } > > - if (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack) > + if (ctx->preferred_pack_idx != NO_PREFERRED_PACK && > + ctx->preferred_pack_idx < start_pack)
Looks good. It's tempting to say that any value of preferred_pack_idx lower than start_pack is going to be OK, since start_pack is a uint32_t as well, but we need to make sure that the preferred pack was specified in the first place.
The rest all looks good as well, thanks.
Thanks, Taylor