[0/6] pack-bitmap: various pack-bitmap-write cleanups

Message

Taylor Blau May 14, 2024, 7:56 p.m. UTC

This topic was born out of tb/pseudo-merge-bitmaps, which has a few
quality-of-life improvements in the pack-bitmap machinery.

The main changes are to remove the static 'struct bitmap_writer', drop
one unused fields, and start using another unused one (see "move
commit_positions into commit_pos fields").

There are other smaller changes, too, like cleaning up the flag
allocation table in object.h, as well as introducing a new
bitmap_writer_free() function.

The next round of tb/pseudo-merge-bitmaps will be based on this branch.

Thanks in advance for your review!

Taylor Blau (6):
  object.h: add flags allocated by pack-bitmap.h
  pack-bitmap-write.c: move commit_positions into commit_pos fields
  pack-bitmap: avoid use of static `bitmap_writer`
  pack-bitmap: drop unused `max_bitmaps` parameter
  pack-bitmap-write.c: avoid uninitialized 'write_as' field
  pack-bitmap: introduce `bitmap_writer_free()`

 builtin/pack-objects.c |  19 +++-
 midx-write.c           |  17 ++-
 object.h               |   1 +
 pack-bitmap-write.c    | 248 +++++++++++++++++++++--------------------
 pack-bitmap.h          |  38 +++++--
 5 files changed, 185 insertions(+), 138 deletions(-)


base-commit: 83f1add914c6b4682de1e944ec0d1ac043d53d78

Comments

Jeff King May 15, 2024, 6:18 a.m. UTC | #1

On Tue, May 14, 2024 at 03:56:46PM -0400, Taylor Blau wrote:

> This topic was born out of tb/pseudo-merge-bitmaps, which has a few
> quality-of-life improvements in the pack-bitmap machinery.
> 
> The main changes are to remove the static 'struct bitmap_writer', drop
> one unused fields, and start using another unused one (see "move
> commit_positions into commit_pos fields").
> 
> There are other smaller changes, too, like cleaning up the flag
> allocation table in object.h, as well as introducing a new
> bitmap_writer_free() function.
> 
> The next round of tb/pseudo-merge-bitmaps will be based on this branch.

These all look good to me. And I think it is nice to pull them out of
the larger series, to keep the independent steps to a more digestible
size.

-Peff

Patrick Steinhardt May 15, 2024, 9:05 a.m. UTC | #2

On Tue, May 14, 2024 at 03:56:46PM -0400, Taylor Blau wrote:
> This topic was born out of tb/pseudo-merge-bitmaps, which has a few
> quality-of-life improvements in the pack-bitmap machinery.
> 
> The main changes are to remove the static 'struct bitmap_writer', drop
> one unused fields, and start using another unused one (see "move
> commit_positions into commit_pos fields").
> 
> There are other smaller changes, too, like cleaning up the flag
> allocation table in object.h, as well as introducing a new
> bitmap_writer_free() function.
> 
> The next round of tb/pseudo-merge-bitmaps will be based on this branch.
> 
> Thanks in advance for your review!

I've got some tiny nits, but overall this series looks good to me
already. I don't mind if it's merged in the current state if you don't
think any of the nits are worthy to be addressed.

Thanks for this cleanup!

Patrick

Taylor Blau May 15, 2024, 3:58 p.m. UTC | #3

On Wed, May 15, 2024 at 11:05:25AM +0200, Patrick Steinhardt wrote:
> On Tue, May 14, 2024 at 03:56:46PM -0400, Taylor Blau wrote:
> > This topic was born out of tb/pseudo-merge-bitmaps, which has a few
> > quality-of-life improvements in the pack-bitmap machinery.
> >
> > The main changes are to remove the static 'struct bitmap_writer', drop
> > one unused fields, and start using another unused one (see "move
> > commit_positions into commit_pos fields").
> >
> > There are other smaller changes, too, like cleaning up the flag
> > allocation table in object.h, as well as introducing a new
> > bitmap_writer_free() function.
> >
> > The next round of tb/pseudo-merge-bitmaps will be based on this branch.
> >
> > Thanks in advance for your review!
>
> I've got some tiny nits, but overall this series looks good to me
> already. I don't mind if it's merged in the current state if you don't
> think any of the nits are worthy to be addressed.

Thanks, both, for your review!

Thanks,
Taylor