Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote:
> I didn't check the previous patches, but your sign-off should be the
> last line of the message. (You are singing off on all previous content,
> and any later content is not covered by your sign-off.)
Ohhh, got it. I didn't know about it before.
> nit: This alignment should use four spaces at the end so the second phrase
> matches the start of the previous phrase. Like this:
>
> if (flags & BITMAP_OPT_LOOKUP_TABLE &&
> git_env_bool("GIT_TEST_READ_COMMIT_TABLE", 1)) {
>
> Perhaps it looked right in your editor because it renders tabs as 4 spaces
> instead of 8 spaces.
I don't know why but my editor sometimes do some weird things for alignments.
I generally use VS Code. But for alignment related problems, sometimes I have
to use vi editor.
> Here, we _do_ want to keep the st_mult(). Is the st_add() still necessary? It
> seems this is a leftover from the previous version that had the 4-byte flag
> data.
>
> We set table_size to zero above. We could drop that initialization and instead
> have this after the "size_t triplet_sz" definition:
>
> size_t table_size = st_mult(ntohl(header->entry_count),
> triplet_sz));
Yes, you're right. Will update.
> I expected something different: binary search on the triplets where the comparison is
> made by looking up the OID from the [multi-]pack-index and comparing that OID to the
> commit OID we are looking for.
>
> I'm not convinced that the binary search I had in mind is meaningfully faster than
> what you've implemented here, so I'm happy to leave it as you have it. We can investigate
> if that full search on the pack-index matters at all (it probably doesn't).
Good idea! Thanks!
> While there is potential that this is wasteful, it's probably not that huge,
> so we can start with the "maximum XOR depth" and then reconsider a smaller
> allocation in the future.
Ok.
> We should consider ensuring that also "size < bitmap_git->entry_count".
> Better yet, create an xor_positions_alloc variable that is initialized
> to the entry_count value.
>
> "size" should probably be xor_positions_nr.
>
> > + xor_positions[size++] = xor_pos;
> > + triplet = bitmap_get_triplet(bitmap_git, xor_pos);
> > + xor_pos = triplet_get_xor_pos(triplet);
> > + }
>
> (at this point, "if (xor_positions_nr >= xor_positions_alloc)", then error
> out since the file must be malformed with an XOR loop.)
Got it.
> Since we are storing the bitmap here as we "pop" the stack, should we be
> looking for a stored bitmap while pushing to the stack in the previous loop?
> That would save time when using multiple bitmaps with common XOR bases.
Yeah, I also am thinking about it. Will make a try.
Thanks :)
