Re: [PATCH v2 06/23] pack-bitmap-write: support storing pseudo-merge commits

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2425 bytes --]

On Mon, May 06, 2024 at 02:48:10PM -0400, Taylor Blau wrote:
> 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!

Yup, that's clearer, thanks.

> > [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:

Yeah, no idea. I must've been seeing ghosts here.

>   - 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.

Okay, thanks for the explanation!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]