Re: [PATCH v2 06/23] pack-bitmap-write: support storing pseudo-merge commits
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 6, 2024, 18:48 UTC
- Message-ID
- <Zjkl6vMHCeDsJKdR@nand.local>
- In-Reply-To
- <ZjjEmGHYnw20wVBg@tanuki>
On Mon, May 06, 2024 at 01:52:56PM +0200, Patrick Steinhardt wrote:
Show 15 quoted lines
> 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!
Show 14 quoted lines
> [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