Hi Junio, Thanks for the feedbacks > On May 6, 2020, at 19:03, Junio C Hamano <gitster@xxxxxxxxx> wrote: ... > We write this part in imperative mood, as if > we are giving an order to the codebase to "become like so". We do > not say "I do X, I do Y". This is a great feedback. I will try to include all of your suggestions and edit the message before submitting V3. >> Note: >> - `repack.packKeptObjects` will be addressed by Derrick Stolee in >> the following patch > > This definitely does not belong to the commit log message. It would > make a helpful note meant for the reviewers if written below the > three-dash line, though. Duly noted. > Do we need to worry about the configuration variables understood by > the "git pack-objects" command to get in the way, by the way? > "pack.packsizelimit" may cause "git repack" to produce more than one > packfile, and if this codepath wants to avoid it (I do not know if > that is the case), it may have to override it from the command line, > for example. I dont think we want to avoid the packsizelimit here. The point of repacking with midx is to help end users consolidate multiple packfile in a non-disruptive way. If you wish to put a constraint (i.e. packsizelimit, packKeptObjects) on this process, you should be able to. >> Signed-off-by: Son Luong Ngoc <sluongng@xxxxxxxxx> >> --- >> midx.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/midx.c b/midx.c >> index 9a61d3b37d9..3348f8e569b 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; > > By default we use delta-base-offset, so if repo_config_get_bool() > did not see the repack.usedeltabaseoffset configuration defined in > any configuration file, we still want to see 1 after it returns. > >> + int use_delta_islands; > > What is the reason why it is safe to leave this uninitialized? Did > you mean > > int use_delta_islands = 0; > > here? I think I totally misread how repo_config_get_bool() supposed to work Your comment here made me re-read it and things got a lot clearer. Will set the default value to 0 in next version. > Thanks. Much appreciate, Son Luong.
