Re: [PATCH 13/49] repack: move 'delta_base_offset' to 'struct pack_objects_args'
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 10, 2025, 22:54 UTC
- Message-ID
- <aOmOusrJ5J29wpml@nand.local>
- In-Reply-To
- <20251010055431.GE1965904@coredump.intra.peff.net>
On Fri, Oct 10, 2025 at 01:54:31AM -0400, Jeff King wrote:
Show 28 quoted lines
> On Sun, Sep 28, 2025 at 06:08:00PM -0400, Taylor Blau wrote: > > > The static variable 'delta_base_offset' determines whether or not we > > pass the "--delta-base-offset" command-line argument when spawning > > pack-objects as a child process. Its introduction dates back to when > > repack was rewritten in C, all the way back in a1bbc6c017 (repack: > > rewrite the shell script in C, 2013-09-15). > > > > 'struct pack_objects_args' was introduced much later on in 4571324b99 > > (builtin/repack.c: allow configuring cruft pack generation, 2022-05-20), > > but did not move the 'delta_base_offset' variable. > > > > Since the 'delta_base_offset' is a property of an individual > > pack-objects command, re-introduce that variable as a member of 'struct > > pack_objects_args', which will enable further code movement in the > > subsequent commits. > > Hmm, OK. It is true that it is a property of a pack-objects invocation, > but would we ever want it to be different between the two? I'd think > not, and from what I can see: > > > + cruft_po_args.delta_base_offset = po_args.delta_base_offset; > > we'd always use the same value in both cases. It kind of makes me wonder > if it should be in the repack_ctx variable instead, then, with both > pack-objects invocations pulling from a common source. But I could also > believe that makes life harder later on, when you want to pass just the > pack_objects_args to prepare_pack_objects().
Yeah, we do want to use the same value here in both cases. My thinking when writing this patch was that whether or not we pass the --delta-base-offset command-line argument to pack-objects *is* a property of the pack-objects invocation.
But the fact that we want to use the same value when generating reachable/geometric packs and when generating cruft packs is more of an implementation detail of repack itself. I like that that detail is made explicit in the line above.
Thanks, Taylor