Re: [PATCH 6/6] midx.c: include preferred pack correctly with existing MIDX

From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Date: Aug 20, 2022, 18:40 UTC
Message-ID: <CAPOJW5zmbQ966KXjaEvxk-oHu01BsxwszUTu3et4SYGFCAegCA@mail.gmail.com>
In-Reply-To: <4ddddc959b042faf7ae98a8e8eaa05e77f9ea23e.1660944574.git.me@ttaylorr.com>

On Sat, Aug 20, 2022 at 3:00 AM Taylor Blau <me@ttaylorr.com> wrote:

Show 6 quoted lines

>
> +               if (-1 < preferred_pack && preferred_pack < start_pack)
> +                       midx_fanout_add_pack_fanout(&fanout, info,
> +                                                   preferred_pack, 1,
> +                                                   cur_fanout);
> +

All the other changes make sense to me but I have a question about this particular change. Instead of adding all the preferred objects again (but in this case these are being added from preferred pack) in `fanout->entries`, will it be better if we call `midx_fanout_add_pack_fanout()` function from `midx_fanout_add_midx_fanout()` when above conditions are met? Something like this -

    static void midx_fanout_add_midx_fanout(struct midx_fanout *fanout,
                                        struct multi_pack_index *m,
                                        struct pack_info *info,
                                        uint32_t cur_pack,
                                        int preferred,
                                        uint32_t cur_fanout)
    {
     ...
          if (cur_fanout)
                start = ntohl(m->chunk_oid_fanout[cur_fanout - 1]);
          end = ntohl(m->chunk_oid_fanout[cur_fanout]);
          if (preferred) {
                midx_fanout_add_pack_fanout(&fanout, info, cur_pack,

preferred, cur_fanout);
                return;
          }

          for (.....) {
          ........
    }

It may reduce some extra code work but also could decrease readability of the function(may be). What's your thought here?

Thanks :)