On 4/23/2025 3:26 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Derrick Stolee <stolee@xxxxxxxxx>
>>
>> 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.
>> +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
