Re: [PATCH v4 06/11] pack-objects: move in_pack out of struct object_entry

From: Stefan Beller <sbeller@google.com>
Date: Mar 26, 2018, 20:39 UTC
Message-ID: <CAGZ79kamsRrqZ09BtUZPdQEKXNn9X3CphsmUdoqsKv-T=kGSsg@mail.gmail.com>
In-Reply-To: <20180316183200.31014-7-pclouds@gmail.com>

Hi,

sorry for the late review, as I am pointed here indirectly via https://public-inbox.org/git/xmqqy3iebpsw.fsf@gitster-ct.c.googlers.com/

On Fri, Mar 16, 2018 at 11:33 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> +LIMITATIONS
> +-----------
> +
> +This command could only handle 16384 existing pack files at a time.

s/could/can/ ?

> @@ -3191,6 +3200,9 @@ int cmd_pack_objects(int argc, const char **argv,

const char *prefix)

>                  }
>          }

> +       /* make sure IN_PACK(0) return NULL */

I was confused for a while staring at this comment, /s/0/NULL/ would have helped me.

Show 9 quoted lines

> +static inline unsigned int oe_add_pack(struct packing_data *pack,
> +                                      struct packed_git *p)
> +{
> +       if (pack->in_pack_count >= (1 << OE_IN_PACK_BITS))
> +               die(_("too many packs to handle in one go. "
> +                     "Please add .keep files to exclude\n"
> +                     "some pack files and keep the number "
> +                     "of non-kept files below %d."),
> +                   1 << OE_IN_PACK_BITS);

The packs are indexed 0..N-1, so we can actually handle N packs I presume. But if we actually have N, then we'd run the

   /* make sure IN_PACK(0) return NULL */
   oe_add_pack(.., NULL);

as N+1, hence the user can only do N-1 ?

Oh wait! the code below makes me think we index from 1..N, treating index 0 special as uninitialized? So we actually can only store N-1 ?

> +       if (p) {
> +               if (p->index > 0)

s/>/!=/ ?

The new index variable is only used in these three inlined header functions, and in_pack_count is strictly positive, so index as well as in_pack_count could be made unsigned?

Given that oe_add_pack returns an unsigned, I would actually prefer to have in_pack_count an unsigned as well.

Show 8 quoted lines

> +                       die("BUG: this packed is already indexed");
> +               p->index = pack->in_pack_count;
> +       }
> +       pack->in_pack[pack->in_pack_count] = p;
> +       return pack->in_pack_count++;
> +}
> +
> +static inline struct packed_git *oe_in_pack(const struct packing_data

*pack,

Show 5 quoted lines

> +                                           const struct object_entry *e)
> +{
> +       return pack->in_pack[e->in_pack_idx];
> +
> +}

extra new line after return?

Show 6 quoted lines

> +static inline void oe_set_in_pack(struct object_entry *e,
> +                                 struct packed_git *p)
> +{
> +       if (p->index <= 0)
> +               die("BUG: found_pack should be NULL "
> +                   "instead of having non-positive index");

Do we also want to guard against
     p->index > (1 << OE_IN_PACK_BITS)
here? Also there is a BUG() macro, that would be better
as it reports line file/number, but we cannot use it here as
it is a header inline.