Re: [PATCH] midx: apply gitconfig to midx repack

From: Son Luong Ngoc <sluongng@gmail.com>
Date: May 5, 2020, 16:03 UTC
Message-ID: <74A7FE73-6B5F-4DCF-9A57-AD11306CFAF8@gmail.com>
In-Reply-To: <8bd91a14-75dc-76e2-31b4-54eff5bea8dd@gmail.com>

Hi Derrick,

Thanks for a swift and comprehensive review.

Show 11 quoted lines

> On May 5, 2020, at 15:50, Derrick Stolee <stolee@gmail.com> wrote:
> 
> In the scenario where there is a .keep pack _and_ it is small enough to get
> picked up by the batch size, the 'git multi-pack-index repack' command will
> create a new pack containing its objects (and objects from other packs) but
> the 'git multi-pack-index expire' command will not delete the pack with .keep.
> 
> The good news is that after the first repack, the objects in the pack are
> in a newer pack, so the multi-pack-index will not repack those objects from
> that pack multiple times. However, this may be unintended behavior for the
> user that specified the .keep pack.

Yup I experienced exactly this when trying to test midx repack/expire with biggest pack file marked with `.keep`. Luckily the storage size bump for duplicated objects was not noticeable in my case. You worded the situation precisely.

> I think the right thing to do to respect "repack.packKeptObjects = false" is
> to ignore the packs when selecting the batch of objects. Instead of asking
> you to do this, I added a patch below. Please take it into your v2, if you
> don't mind.

Gladly. This should help me a lot for re-rolling V2.

Show 6 quoted lines

>> +static int delta_base_offset = 1;
>> +static int write_bitmaps = -1;
>> +static int use_delta_islands;
>> +
> 
> Why not make these local to the midx_repack method?

No practical reason except me shamelessly lifted those from builtin/repack.c. I was a bit confused how `git repack` houses these logic in the builtin file, while midx was having these logic in the midx.c instead of builtin/multi-pack-index.c.

I make them local in V2.

Show 13 quoted lines

>> int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)
>> {
>> 	int result = 0;
>> @@ -1381,12 +1385,25 @@ 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;
>> 
>> +  git_config_get_bool("repack.usedeltabaseoffset", &delta_base_offset);
>> +  git_config_get_bool("repack.writebitmaps", &write_bitmaps);
>> +  git_config_get_bool("repack.usedeltaislands", &use_delta_islands);
>> +
> 
> It looks like you have some spacing issues here. Perhaps use tabs?

Rookie mistake on my part. Will fix it in V2

Show 15 quoted lines

>> +	if (write_bitmaps > 0)
>> +		argv_array_push(&cmd.args, "--write-bitmap-index");
>> +	else if (write_bitmaps < 0)
>> +		argv_array_push(&cmd.args, "--write-bitmap-index-quiet");
> 
> These make less sense. Unless --batch-size=0 and there are no .keep
> packs (with the patch below) I'm not sure we _can_ write bitmap indexes
> here. The pack-file is not necessarily closed under reachability. Or,
> will supplying these arguments to 'git pack-objects' actually do that
> closure?
> 
> I would be happy to special-case these options to the "--batch-size=0"
> situation and otherwise ignore them. This then gets into enough
> complication that we should update the documentation as in the patch
> below.

You make a great point here. I completely missed this as I have been largely testing with repacking only 2 packs, effectively with --batch-size=0.

I think having the bitmaps index is highly desirable in `--batch-size=0` case. I will try to include that in V2 (with Documentation).

> At minimum, it would be good to have some tests that exercise these
> code paths so we know they are behaving correctly.

I will do some readings with the current tests for repack and midx. Hopefully I will have something for V2. (^_^ !)

> Thanks,
> -Stolee

Cheers, Son Luong