| Message ID | a925307d4c57506f5236e60dc1390998e186cf26.1589034270.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: apply gitconfig to midx repack | expand |
"Son Luong Ngoc via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Son Luong Ngoc <sluongng@gmail.com> > > 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.
On Sat, May 09, 2020 at 09:51:08AM -0700, Junio C Hamano wrote: > "Son Luong Ngoc via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Son Luong Ngoc <sluongng@gmail.com> > > > > 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". Thanks for this, it will take me a bit to adjust to this style of writing but I do find it to be a lot clearer and practical. Will update in next version. > > > 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. Will add the comments in next version. > > With that, the rest of the patch makes sense. > > Thanks. Cheers, Son Luong
diff --git a/midx.c b/midx.c index 9a61d3b37d9..1e76be56826 100644 --- a/midx.c +++ b/midx.c @@ -1369,6 +1369,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, struct child_process cmd = CHILD_PROCESS_INIT; struct strbuf base_name = STRBUF_INIT; struct multi_pack_index *m = load_multi_pack_index(object_dir, 1); + int delta_base_offset = 1; + int use_delta_islands = 0; if (!m) return 0; @@ -1381,12 +1383,20 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, } else if (fill_included_packs_all(m, include_pack)) goto cleanup; + repo_config_get_bool(r, "repack.usedeltabaseoffset", &delta_base_offset); + repo_config_get_bool(r, "repack.usedeltaislands", &use_delta_islands); + argv_array_push(&cmd.args, "pack-objects"); strbuf_addstr(&base_name, object_dir); strbuf_addstr(&base_name, "/pack/pack"); argv_array_push(&cmd.args, base_name.buf); + if (delta_base_offset) + argv_array_push(&cmd.args, "--delta-base-offset"); + if (use_delta_islands) + argv_array_push(&cmd.args, "--delta-islands"); + if (flags & MIDX_PROGRESS) argv_array_push(&cmd.args, "--progress"); else