Re: [PATCH v2 22/24] pack-bitmap-write: use existing bitmaps

> > However, if we walk trees
> > from top to bottom, then we might be parsing trees that are actually
> > part of a re-used bitmap.
>
> Isn't the issue that we shouldn't walk trees at all before exhausting
> our commit search, not the direction that we walk the trees in (top to
> bottom or bottom to top or whatever)?

> > To avoid over-walking trees, add them to a
> > LIFO queue and walk them from bottom-to-top after exploring commits
> > completely.
>
> Just to clarify - would it work just as well with a FIFO queue (not LIFO
> queue)? It seems to me that the most important part is doing this after
> exploring commits completely.

> >  static void fill_bitmap_commit(struct bb_commit *ent,
> >  			       struct commit *commit,
> > -			       struct prio_queue *queue)
> > +			       struct prio_queue *queue,
> > +			       struct prio_queue *tree_queue,
> > +			       struct bitmap_index *old_bitmap,
> > +			       const uint32_t *mapping)
> >  {
> >  	if (!ent->bitmap)
> >  		ent->bitmap = bitmap_new();
> >
> > -	bitmap_set(ent->bitmap, find_object_pos(&commit->object.oid));
> >  	prio_queue_put(queue, commit);
> >
> >  	while (queue->nr) {
> >  		struct commit_list *p;
> >  		struct commit *c = prio_queue_get(queue);
> >
> > +		/*
> > +		 * If this commit has an old bitmap, then translate that
> > +		 * bitmap and add its bits to this one. No need to walk
> > +		 * parents or the tree for this commit.
> > +		 */
>
> This comment should be right before "if (old && ...", I think. Here, it
> is a bit misleading. It leads me to think that "this commit has an old
> bitmap" means old_bitmap != NULL, but it is actually old != NULL.

> > +		if (old_bitmap && mapping) {
>
> This is defensive in that if we somehow calculate old_bitmap without
> mapping (or the other way around) (which is a bug), things just slow
> down instead of breaking. I'm OK with this, but I still wanted to call
> it out.