Re: [PATCH v2 4/6] midx-write: use uint32_t for preferred_pack_idx
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 3, 2025, 10:15 UTC
- Message-ID
- <aLgVOa96grWC3G0Q@pks.im>
- In-Reply-To
- <2290e27ded03866cb816be5fb6e08b83a75a6f07.1756589007.git.gitgitgadget@gmail.com>
On Sat, Aug 30, 2025 at 09:23:25PM +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.
Tiny nit: the last sentence reads a bit funny. Maybe something like this?
Furthermore, with this change we end up extending the range from
2^31 possible packs to 2^32-1.Show 23 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 cb0211289d..1822268ce2 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -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))) {
> /*
> * Objects from preferred packs are addedNeither of these braces around comparisons are really needed, but feel free to ignore as you simply piggy-back on existing style.
Show 11 quoted lines
> @@ -1040,7 +1042,9 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
> struct hashfile *f = NULL;
> struct lock_file lk;
> struct tempfile *incr;
> - struct write_midx_context ctx = { 0 };
> + struct write_midx_context ctx = {
> + .preferred_pack_idx = NO_PREFERRED_PACK,
> + };
> int bitmapped_packs_concat_len = 0;
> int pack_name_concat_len = 0;
> int dropped_packs = 0;Why is this change needed? We didn't previously initialize `.preferred_pack_idx = -1` either.
Patrick