Hi, sorry for the late review, as I am pointed here indirectly via https://public-inbox.org/git/xmqqy3iebpsw.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/ On Fri, Mar 16, 2018 at 11:33 AM Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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. > +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. > + 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, > + const struct object_entry *e) > +{ > + return pack->in_pack[e->in_pack_idx]; > + > +} extra new line after return? > +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.
