On Mon, May 06, 2024 at 01:52:56PM +0200, Patrick Steinhardt wrote:
> On Mon, Apr 29, 2024 at 04:43:15PM -0400, Taylor Blau wrote:
> [snip]
> > @@ -46,6 +48,11 @@ struct bitmap_writer {
> >
> > static struct bitmap_writer writer;
> >
> > +static inline int bitmap_writer_selected_nr(void)
> > +{
> > + return writer.selected_nr - writer.pseudo_merges_nr;
> > +}
>
> This function may use a comment to explain what its meaning actually is.
> Like, `bitmap_writer_selected_nr()` is obviously not the same as the
> `selected_nr` of the `bitmap_writer`, which is quite confusing. So why
> do we subtract values and why are there two different `selected_nr`s?
selected_nr is the total number of bitmaps we are writing (including
pseudo-merges), and writer.pseudo_merges_nr is the number of those
bitmaps which are pseudo-merges.
I renamed this function to bitmap_writer_nr_selected_commits() which
should clarify things, let me know if that works!
> [snip]
> > diff --git a/pack-bitmap.h b/pack-bitmap.h
> > index dae2d68a338..ca9acd2f735 100644
> > --- a/pack-bitmap.h
> > +++ b/pack-bitmap.h
> > @@ -21,6 +21,7 @@ struct bitmap_disk_header {
> > unsigned char checksum[GIT_MAX_RAWSZ];
> > };
> >
> > +#define BITMAP_PSEUDO_MERGE (1u<<21)
> > #define NEEDS_BITMAP (1u<<22)
>
> This flag is already used by "builtin/pack-objects.c", which may be fine.
> But in any case, shouldn't we update "object.h" with both of these flags?
I can't see where in builtin/pack-objects.c this flag is used. The table
in object.h says that bit 21 is used in:
- list-objects-filter.c
- builtin/index-pack.c
- builtin/unpack-objects.c
But I think those are all fine. We don't call unpack-objects from the
bitmap writing paths, and the same is true of index-pack (since we're
writing the pack out directly).
list-objects-filter.c should also be OK, since I am 99% sure that these
two code paths do not collide, but even if they do, that field is only
set on tree objects from the list-objects-filter.c code path, and the
new bits in pack-bitmap.h are only set on commit objects.
Regardless, let me not forget to update the table in object.h! Thanks
for reminding me.
Thanks,
Taylor
