On Tue, Nov 24, 2020 at 05:14:09PM -0800, Jonathan Tan wrote:
> > From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> >
> > The fill_bitmap_commit() method assumes that every parent of the given
> > commit is already part of the current bitmap. Instead of making that
> > assumption, let's walk parents until we reach commits already part of
> > the bitmap. Set the value for that parent immediately after querying to
> > save time doing double calls to find_object_pos() and to avoid inserting
> > the parent into the queue multiple times.
>
> I see from the later patches that this has no effect until the part
> where we can skip commits, but as Junio says [1], it's worth mentioning
> it here. Maybe something like:
>
> The fill_bitmap_commit() method assumes that every parent of the given
> commit is already part of the current bitmap. This is currently
> correct, but a subsequent patch will change the nature of the edges of
> the graph from parent-child to ancestor-descendant. In preparation for
> that, let's walk parents...
Thanks. Stolee and I worked a little on revising this last week, and I
think that the current log message is more along these lines. Here's
what we wrote:
pack-bitmap-write: fill bitmap with commit history
The current implementation of bitmap_writer_build() creates a
reachability bitmap for every walked commit. After computing a bitmap
for a commit, those bits are pushed to an in-progress bitmap for its
children.
fill_bitmap_commit() assumes the bits corresponding to objects
reachable from the parents of a commit are already set. This means that
when visiting a new commit, we only have to walk the objects reachable
between it and any of its parents.
A future change to bitmap_writer_build() will relax this condition so
not all parents have their reachable objects set in the in-progress
bitmap. Prepare for that by having 'fill_bitmap_commit()' walk
parents until reaching commits whose bits are already set. Then, walk
the trees for these commits as well.
This has no functional change with the current implementation of
bitmap_writer_build().
> > static void fill_bitmap_commit(struct bb_commit *ent,
> > - struct commit *commit)
> > + struct commit *commit,
> > + struct prio_queue *queue)
>
> As far as I can see, this function expects an empty queue and always
> ends with the queue empty, and the only reason why we don't instantiate
> a new queue every time is so that we can save on the internal array
> allocation/deallocation. Maybe add a comment to that effect.
Sure. Would you find a comment like that more helpful above
'fill_bitmap_commit()', or above the declaration of 'queue' (in
'bitmap_writer_build()') itself?
Thanks,
Taylor
