Re: [PATCH 1/3] test-tool: add pack-deltas helper

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 23, 2025, 19:32 UTC
Message-ID: <460cd77b-2b49-4159-bac5-0fd4fb655f84@gmail.com>
In-Reply-To: <xmqqmsc6y911.fsf@gitster.g>

On 4/23/2025 3:26 PM, Junio C Hamano wrote:

Show 33 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> When trying to demonstrate certain behavior in tests, it can be helpful
>> to create packfiles that have specific delta structures. 'git
>> pack-objects' uses various algorithms to select deltas based on their
>> compression rates, but that does not always demonstrate all possible
>> packfile shapes. This becomes especially important when wanting to test
>> 'git index-pack' and its ability to parse certain pack shapes.
>>
>> We have prior art in t/lib-pack.sh, where certain delta structures are
>> produced by manually writing certain opaque pack contents. However,
>> producing these script updates is cumbersome and difficult to do as a
>> contributor.
>>
>> Instead, create a new test-tool, 'test-tool pack-deltas', that reads a
>> list of instructions for which objects to include in a packfile and how
>> those objects should be written in delta form.
>>
>> At the moment, this only supports REF_DELTAs as those are the kinds of
>> deltas needed to exercise a bug in 'git index-pack'.
> 
> Wonderful writing.  I agree with the destination where this effort
> wants to go, including the decision that starting with ref-delta
> only is a good enough first step.
> 
> As to the implementation, I was a tiny little bit bummed to see
> that, even though it does share the code with the real pack-objects
> code paths to compute delta data by calling diff_delta(), and to
> write per-object header by calling encode_in_pack_object_header(),
> it has its own compression loop that does not even do an error
> checking after calling into zlib deflate machinery.

I could strengthen these options to help folks more quickly understand potential failures as being part of the pack write instead of them failing during the later pack read.

> Perhaps that is unavoidable due to the code structure of the
> production code.

I briefly considered extracting some code out of builtin/pack-objects.c but it relies heavily on globals and context that I won't have in this helper. I'm open to suggestions for how I can safely share more code, but my initial attempt required too much refactoring to be worth it.

I am grateful for the amount of code from pack-write.c that I _was_ able to reuse.

Show 23 quoted lines

>> +static const char usage_str[] = "test-tool pack-deltas <n>";
>> ...
>> +int cmd__pack_deltas(int argc, const char **argv)
>> +{
>> +	int N;
>> +	struct hashfile *f;
>> +	struct strbuf line = STRBUF_INIT;
>> +
>> +	if (argc != 2) {
>> +		usage(usage_str);
>> +		return -1;
>> +	}
>> +
>> +	N = atoi(argv[1]);
> 
> It somewhat looks strange to see an uppercase N used as a variable
> name.  Together with the usage string, how about renaming "N" and
> "n" after "number of objects", e.g.
> 
> 	test-tool pack-deltas <num-objects>
> 	int num_objects;
> 
> or something?

I definitely should have used a better name here. Thanks.

-Stolee