Re: [PATCH] midx: apply gitconfig to midx repack
On 5/5/2020 9:06 AM, Son Luong Ngoc via GitGitGadget wrote:
Show 12 quoted lines
> From: Son Luong Ngoc <sluongng@gmail.com>
>
> Multi-Pack-Index repack is an incremental, repack solutions
> that allows user to consolidate multiple packfiles in a non-disruptive
> way. However the new packfile could be created without some of the
> capabilities of a packfile that is created by calling `git repack`.
>
> This is because with `git repack`, there are configuration that would
> enable different flags to be passed down to `git pack-objects` plumbing.
>
> In this patch, I applies those flags into `git multi-pack-index repack`
> so that it respect the `repack.*` config series.
This is a good idea! The fact that these are specified by 'git repack' and not 'git pack-objects' makes intervention here necessary.
However, I don't think that all of these will apply properly.
> Note: I left out `repack.packKeptObjects` intentionally as I dont think
> its relevant to midx repack use case.
I think it would be good to add this, but in a different way.
Show 49 quoted lines
> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
> ---
> midx: apply gitconfig to midx repack
>
> Midx repack has largely been used in Microsoft Scalar on the client side
> to optimize the repository multiple packs state. However when I tried to
> apply this onto the server-side, I realized that there are certain
> features that were lacking compare to git repack. Most of these features
> are highly desirable on the server-side to create the most optimized
> pack possible.
>
> One of the example is delta_base_offset, comparing an midx repack
> with/without delta_base_offset, we can observe significant size
> differences.
>
> > du objects/pack/*pack
> 14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack
> 9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack
>
> Latest 2.26.2 (without delta_base_offset)
> > git multi-pack-index write
> > git multi-pack-index repack
> > git multi-pack-index expire
> > du objects/pack/*pack
> 9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack
>
> With delta_base_offset
> > git version
> git version 2.26.2.672.g232c24e857.dirty
> > git multi-pack-index write
> > git multi-pack-index repack
> > git multi-pack-index expire
> > du objects/pack/*pack
> 9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack
>
> In this patch, I intentionally leaving out repack.packKeptObjects as I
> don't think its very relevant to midx repack use case:
>
> * One could always exclude biggest packs with --batch-size option
>
>
> * For non-biggest-packs exclusion use case, its rather rare (unless you
> want to have a special pack with only commits and trees being
> excluded from repack to serve partial clone better?)
>
>
>
> Please let me know if anyone think that we should include that option
> for the sake of completions.
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.
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.
Show 19 quoted lines
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/626
>
> midx.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/midx.c b/midx.c
> index 9a61d3b37d9..88f16594268 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1361,6 +1361,10 @@ static int fill_included_packs_batch(struct repository *r,
> return 0;
> }
>
> +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?
Show 11 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?
Show 10 quoted lines
> 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");
These two probably make sense.
> + 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.
At minimum, it would be good to have some tests that exercise these code paths so we know they are behaving correctly.
Thanks, -Stolee
-- >8 --
From 8a115191cbf21c553675a235c8c678affbca609b Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Tue, 5 May 2020 09:37:50 -0400
Subject: [PATCH] multi-pack-index: respect repack.packKeptObjects=false
When selecting a batch of pack-files to repack in the "git multi-pack-index repack" command, Git should respect the repack.packKeptObjects config option. When false, this option says that the pack-files with an associated ".keep" file should not be repacked. This config value is "false" by default.
There are two cases for selecting a batch of objects. The first is the case where the input batch-size is zero, which specifies "repack everything". The second is with a non-zero batch size, which selects pack-files using a greedy selection criteria. Both of these cases are updated and tested.
Reported-by: Son Luong Ngoc <sluongng@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-multi-pack-index.txt | 3 +++
midx.c | 26 +++++++++++++++++++++-----
t/t5319-multi-pack-index.sh | 26 ++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 5 deletions(-)
Show changes to 3 files +50 −5
Documentation/git-multi-pack-index.txt, midx.c, t/t5319-multi-pack-index.sh
diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
index 642d9ac5b7..0c6619493c 100644
--- a/Documentation/git-multi-pack-index.txt
+++ b/Documentation/git-multi-pack-index.txt
@@ -56,6 +56,9 @@ repack::
file is created, rewrite the multi-pack-index to reference the
new pack-file. A later run of 'git multi-pack-index expire' will
delete the pack-files that were part of this batch.
++
+If `repack.packKeptObjects` is `false`, then any pack-files with an
+associated `.keep` file will not be selected for the batch to repack.
EXAMPLES
diff --git a/midx.c b/midx.c
index 1527e464a7..d055bf3cd3 100644
--- a/midx.c
+++ b/midx.c
@@ -1280,15 +1280,26 @@ static int compare_by_mtime(const void *a_, const void *b_)
return 0;
}
-static int fill_included_packs_all(struct multi_pack_index *m,
+static int fill_included_packs_all(struct repository *r,
+ struct multi_pack_index *m,
unsigned char *include_pack)
{
- uint32_t i;
+ uint32_t i, count = 0;
+ int pack_kept_objects = 0;
+
+ repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects);
+
+ for (i = 0; i < m->num_packs; i++) {
+ if (prepare_midx_pack(r, m, i))
+ continue;
+ if (!pack_kept_objects && m->packs[i]->pack_keep)
+ continue;
- for (i = 0; i < m->num_packs; i++)
include_pack[i] = 1;
+ count++;
+ }
- return m->num_packs < 2;
+ return count < 2;
}
static int fill_included_packs_batch(struct repository *r,
@@ -1299,6 +1310,9 @@ static int fill_included_packs_batch(struct repository *r,
uint32_t i, packs_to_repack;
size_t total_size;
struct repack_info *pack_info = xcalloc(m->num_packs, sizeof(struct repack_info));
+ int pack_kept_objects = 0;
+
+ repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects);
for (i = 0; i < m->num_packs; i++) {
pack_info[i].pack_int_id = i;
@@ -1325,6 +1339,8 @@ static int fill_included_packs_batch(struct repository *r,
if (!p)
continue;
+ if (!pack_kept_objects && p->pack_keep)
+ continue;
if (open_pack_index(p) || !p->num_objects)
continue;
@@ -1365,7 +1381,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
if (batch_size) {
if (fill_included_packs_batch(r, m, include_pack, batch_size))
goto cleanup;
- } else if (fill_included_packs_all(m, include_pack))
+ } else if (fill_included_packs_all(r, m, include_pack))
goto cleanup;
argv_array_push(&cmd.args, "pack-objects");
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 43a7a66c9d..b2fece5d3d 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -533,6 +533,32 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
)
'
+test_expect_success 'repack respects repack.packKeptObjects=false' '
+ test_when_finished rm -f dup/.git/objects/pack/*keep &&
+ (
+ cd dup &&
+ ls .git/objects/pack/*idx >idx-list &&
+ test_line_count = 5 idx-list &&
+ ls .git/objects/pack/*.pack | sed "s/\.pack/.keep/" >keep-list &&
+ for keep in $(cat keep-list)
+ do
+ touch $keep || return 1
+ done &&
+ git multi-pack-index repack --batch-size=0 &&
+ ls .git/objects/pack/*idx >idx-list &&
+ test_line_count = 5 idx-list &&
+ test-tool read-midx .git/objects | grep idx >midx-list &&
+ test_line_count = 5 midx-list &&
+ THIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&
+ BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
+ git multi-pack-index repack --batch-size=$BATCH_SIZE &&
+ ls .git/objects/pack/*idx >idx-list &&
+ test_line_count = 5 idx-list &&
+ test-tool read-midx .git/objects | grep idx >midx-list &&
+ test_line_count = 5 midx-list
+ )
+'
+
test_expect_success 'repack creates a new pack' '
(
cd dup &&
--
2.26.2.vfs.1.2
Son Luong Ngoc via GitGitGadget
