"Son Luong Ngoc via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Son Luong Ngoc <sluongng@xxxxxxxxx>
>
> Previously, when the "repack" subcommand of "git multi-pack-index" command
> creates new packfile(s), it does not call the "git repack" command but
> instead directly calls the "git pack-objects" command, and the
> configuration variables meant for the "git repack" command, like
> "repack.usedaeltabaseoffset", are ignored.
When we talk about the current state of the code (i.e. before
applying this patch), we do not say "previously". It's not like you
are complaining about a recent breakage, e.g. "previously X worked
like this but since change Y, it instead works like that, which
breaks Z".
> This patch ensured "git multi-pack-index" checks the configuration
> variables used by "git repack" and passes the corresponding options to
> the underlying "git pack-objects" command.
We write this part in imperative mood, as if we are giving an order
to the codebase to "become like so". We do not give an observation
about the patch or the author ("This patch does X, this patch also
does Y", "I do X, I do Y").
Taking these two together, perhaps like:
When the "repack" subcommand of "git multi-pack-index" command
creates new packfile(s), it does not call the "git repack"
command but instead directly calls the "git pack-objects"
command, and the configuration variables meant for the "git
repack" command, like "repack.usedaeltabaseoffset", are ignored.
Check the configuration variables used by "git repack" ourselves
in "git multi-index-pack" and pass the corresponding options to
underlying "git pack-objects".
> Note that `repack.writeBitmaps` configuration is ignored, as the
> pack bitmap facility is useful only with a single packfile.
Good.
> + int delta_base_offset = 1;
> + int use_delta_islands = 0;
These give the default values for two configurations and over there
builtin/repack.c has these lines:
17 static int delta_base_offset = 1;
18 static int pack_kept_objects = -1;
19 static int write_bitmaps = -1;
20 static int use_delta_islands;
21 static char *packdir, *packtmp;
When somebody is tempted to update these to change the default used
by "git repack", it should be easy to notice that such a change must
be accompanied by a matching change to the lines you are introducing
in this patch, or we'll be out of sync.
The easiest way to avoid such a problem may be to stop bypassing
"git repack" and calling "pack-objects" ourselves. That is the
reason why the configuration variables honored by "git repack" are
ignored in this codepath in the first place. But that is not the
approach we are taking, so we need a reasonable way to tell those
who update this file and builtin/repack.c to make matching changes.
At the very least, perhaps we should give a comment above these two
lines in this file, e.g.
/*
* when updating the default for these configuration
* variables in builtin/repack.c, these must be adjusted
* to match.
*/
int delta_base_offset = 1;
int use_delta_islands = 0;
or something like that.
With that, the rest of the patch makes sense.
Thanks.
