"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.
Perhaps that is unavoidable due to the code structure of the
production code.
> +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?
Other than that, looking very good.
