Re: [PATCH v2 12/24] pack-bitmap-write: fill bitmap with commit history

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 23, 2020, 14:54 UTC
Message-ID: <f6d89171-f343-0d4c-7cf4-725c49615b99@gmail.com>
In-Reply-To: <xmqqy2itoyco.fsf@gitster.c.googlers.com>

On 11/22/2020 4:50 PM, Junio C Hamano wrote:

Show 15 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
> 
> Is it because somebody found a case where the assumption does not
> hold and the code with the assumption produces a wrong result?  Is
> it because we can get a better result without making the assumption
> the current code does?

The algorithm from "pack-bitmap-write: reimplement bitmap writing" that calls fill_bitmap_commit() satisfies this assumption, since it computes a reachability bitmap for every commit during the reverse walk. We will soon change that algorithm to "skip" commits, so we need this step in fill_bitmap_commit() to walk forward to fill the gaps.

> In other words, can we explain why we are making the change in the
> proposed log message?

I'm sure Taylor and I can work out a better wording to make this more clear.

Thanks, -Stolee