midx: apply gitconfig to midx repack

From: Son Luong Ngoc <[email protected]>

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.

Note: I left out repack.packKeptObjects intentionally as I dont think its relevant to midx repack use case.

---
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.

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(+)

index 9a61d3b37d9..88f16594268 100644
@@ -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;
+
 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);
+
 	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 (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");
+
 	if (flags & MIDX_PROGRESS)
 		argv_array_push(&cmd.args, "--progress");
 	else

base-commit: b34789c0b0d3b137f0bb516b417bd8d75e0cb306

gitgitgadget

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.

I think it would be good to add this, but in a different way.

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.

---
 Documentation/git-multi-pack-index.txt |  3 +++
 midx.c                                 | 26 +++++++++++++++++++++-----
 t/t5319-multi-pack-index.sh            | 26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)

index 642d9ac5b7..0c6619493c 100644
@@ -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

index 1527e464a7..d055bf3cd3 100644
@@ -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");

index 43a7a66c9d..b2fece5d3d 100755
@@ -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

Hi Derrick,

Thanks for a swift and comprehensive review.

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.

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

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.

Rookie mistake on my part. Will fix it in V2

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).

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

Cheers,
Son Luong

Hi,

Hmm, I just realized that there is a check for --all is being passed on pack-objects side.

	if (batch_size == 0) {
		argv_array_push(&cmd.args, "--all");
		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");
	}

If I do something like this, the midx repack will become tremendously slow as I think pack-objects needs to scan for all revs (fed from midx) and all refs.
Perhaps special exception needed to be made on pack-objects side to trust that midx is feeding it everything there is?

I think adding write_bitmaps support would be a bit out of my hand for now, so I will settle with the delta configs and Derrick's patch for V2. (sending it later today)

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.

14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack
9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack

Latest 2.26.2 (without delta_base_offset)

9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack

With delta_base_offset

git version 2.26.2.672.g232c24e857.dirty

9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack

In this patch, I intentionally leaving out repack.writeBitmaps as I see that it might need some update on pack-objects to improve the performance

Derrick Stolee following patch with address repack. packKeptObjects support.

Derrick Stolee (1):
multi-pack-index: respect repack.packKeptObjects=false

Son Luong Ngoc (1):
midx: apply gitconfig to midx repack

 Documentation/git-multi-pack-index.txt |  3 +++
 midx.c                                 | 36 ++++++++++++++++++++++----
 t/t5319-multi-pack-index.sh            | 26 +++++++++++++++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

base-commit: b34789c0b0d3b137f0bb516b417bd8d75e0cb306
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/626

Range-diff vs v1:

1: 215c882a503 ! 1: 21c648cc486 midx: apply gitconfig to midx repack
@@ Commit message
In this patch, I applies those flags into git multi-pack-index repack so that it respect the repack.* config series.

- Note: I left out repack.packKeptObjects intentionally as I dont think
- its relevant to midx repack use case.
+ Note:
+ - repack.packKeptObjects will be addressed by Derrick Stolee in
+ the following patch
+ - repack.writeBitmaps when --batch-size=0 was NOT adopted here as it
+ requires --all to be passed onto git pack-objects, which is very
+ slow. I think it would be nice to have this in a future patch.

## midx.c ##
-@@ midx.c: static int fill_included_packs_batch(struct repository *r,
- return 0;
- }
+@@ midx.c: 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;

-+static int delta_base_offset = 1;
-+static int write_bitmaps = -1;
-+static int use_delta_islands;
-+
- int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)
- {
- int result = 0;
+ if (!m)
+ return 0;
@@ midx.c: 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);
++ 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");

@@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat
+ argv_array_push(&cmd.args, "--delta-base-offset");
+ if (use_delta_islands)
+ argv_array_push(&cmd.args, "--delta-islands");
-+ 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");
+
if (flags & MIDX_PROGRESS)
argv_array_push(&cmd.args, "--progress");
-: ----------- > 2: 3d7b334f5c6 multi-pack-index: respect repack.packKeptObjects=false

gitgitgadget

From: Son Luong Ngoc <[email protected]>

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.

Note:
- repack.packKeptObjects will be addressed by Derrick Stolee in the following patch
- repack.writeBitmaps when --batch-size=0 was NOT adopted here as it requires --all to be passed onto git pack-objects, which is very slow. I think it would be nice to have this in a future patch.

---
 midx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 9a61d3b37d9..3348f8e569b 100644
@@ -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;
 
 	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

gitgitgadget

...

Just my two cents here: the reachability bitmaps are really tied to the idea of a single pack right now. To create bitmaps, I would currently suggest using the 'git repack' builtin with the proper options. That command deletes the multi-pack-index, unfortunately, but it also produces a single pack and deletes the others (when creating bitmaps).

You are right that the --all option required to pack-objects is not appropriate to add inside git multi-pack-index repack as that changes the pattern. It requires loading all reachable objects, even if they are not already in packs covered by the multi-pack-index. This at minimum violates expectations with the --batch-size argument.

Integrating reachability bitmaps more closely with the multi-pack-index is certainly on our radar, but is a large endeavor.

This new patch looks good to me.

Thanks,
-Stolee

It may be clear to you who wrote the patch, but it is quite unclear to readers how repack gets into the picture. The first sentence talks about what "git multi-pack-index repack" subcommand. Unless you mention that that "git multi-pack-index repack" subcommand calls "git repack" under the hood in order to create a new packfile, the second paragraph can be read as if you are pointing out a problem if the user did

	$ git multi-pack-index repack
	$ git repack

and the explicit "repack" initiated by the user may create a packfile that is somehow incompatible with what the previous repack wanted to do, or something like that.

And this does not help to clear the possible confusion, either.

I think all of the above is clearer if you rewrite the above (including the title) like so:

midx: teach "git multi-pack-index repack" honor "git repack" configuration

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.

Now the problem description is behind us, let's see the description of proposed solution. 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".

Check the configuration variables used by "git repack" ourselves and pass the corresponding options to underlying "git pack-objects" in this codepath.

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.

The phrasing makes it hard to grok. Do you want to say that the repack.writeBitmaps configuration variable is ignored?

I think Derrick gave you the reason why bitmaps is not compatible with midx in general, and that would be a better rationale to record why the configuration is ignored. Perhaps like

Note that repack.writeBitmaps configuration is ignored, as the pack bitmap faciility is useful only with a single packfile.

or something like that?

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.

	int use_delta_islands = 0;

Hi Junio,

Thanks for the feedbacks

...

This is a great feedback.
I will try to include all of your suggestions and edit the message before submitting V3.

Duly noted.

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.

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.

Much appreciate,
Son Luong.

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/git-multi-pack-index.txt |  3 +++
 midx.c                                 | 26 +++++++++++++++++++++-----
 t/t5319-multi-pack-index.sh            | 26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)

index 642d9ac5b72..0c6619493c1 100644
@@ -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

index 3348f8e569b..b8a52740832 100644
@@ -1293,15 +1293,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,

@@ -1312,6 +1323,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;

@@ -1338,6 +1352,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;
 

@@ -1380,7 +1396,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;
 
 	repo_config_get_bool(r, "repack.usedeltabaseoffset", &delta_base_offset);

index 030a7222b2a..67afe1bb8d9 100755
@@ -538,6 +538,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 &&

gitgitgadget

Thanks for pointing this out. This line is repeated in the test after this one. That should be fixed, too.

Thanks,
-Stolee

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.

14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack
9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack

Latest 2.26.2 (without delta_base_offset)

9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack

With delta_base_offset

git version 2.26.2.672.g232c24e857.dirty

9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack

Note that repack.writeBitmaps configuration is ignored, as the pack bitmap facility is useful only with a single packfile.

Derrick Stolee's following patch will address repack.packKeptObjects support.

Derrick Stolee (1):
multi-pack-index: respect repack.packKeptObjects=false

Son Luong Ngoc (2):
midx: teach "git multi-pack-index repack" honor "git repack"
configurations
Ensured t5319 follows arith expansion guideline

 Documentation/git-multi-pack-index.txt |  3 ++
 midx.c                                 | 36 ++++++++++++---
 t/t5319-multi-pack-index.sh            | 62 ++++++++++++++++++--------
 3 files changed, 78 insertions(+), 23 deletions(-)

base-commit: b994622632154fc3b17fb40a38819ad954a5fb88
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/626

Range-diff vs v2:

1: 21c648cc486 ! 1: a925307d4c5 midx: apply gitconfig to midx repack
@@ Metadata
Author: Son Luong Ngoc <[email protected]>

## Commit message ##
- midx: apply gitconfig to midx repack
+ midx: teach "git multi-pack-index repack" honor "git repack" configurations

- 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.
+ 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.

- This is because with git repack, there are configuration that would
- enable different flags to be passed down to git pack-objects plumbing.
+ 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.

- In this patch, I applies those flags into git multi-pack-index repack
- so that it respect the repack.* config series.
-
- Note:
- - repack.packKeptObjects will be addressed by Derrick Stolee in
- the following patch
- - repack.writeBitmaps when --batch-size=0 was NOT adopted here as it
- requires --all to be passed onto git pack-objects, which is very
- slow. I think it would be nice to have this in a future patch.
+ Note that repack.writeBitmaps configuration is ignored, as the
+ pack bitmap facility is useful only with a single packfile.

@@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat
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;
++ int use_delta_islands = 0;

if (!m)
return 0;

 2:  3d7b334f5c6 = 2:  988697dd512 multi-pack-index: respect repack.packKeptObjects=false
 -:  ----------- > 3:  efeb3d7d132 Ensured t5319 follows arith expansion guideline

gitgitgadget

From: Son Luong Ngoc <[email protected]>

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.

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.

Note that repack.writeBitmaps configuration is ignored, as the pack bitmap facility is useful only with a single packfile.

---
 midx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 9a61d3b37d9..1e76be56826 100644
@@ -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

gitgitgadget

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".

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".

Good.

    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 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;

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.

Will add the comments in next version.

Cheers,
Son Luong

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/git-multi-pack-index.txt |  3 +++
 midx.c                                 | 26 +++++++++++++++++++++-----
 t/t5319-multi-pack-index.sh            | 26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)

index 642d9ac5b72..0c6619493c1 100644
@@ -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

index 1e76be56826..9b14d915db1 100644
@@ -1293,15 +1293,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,

@@ -1312,6 +1323,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;

@@ -1338,6 +1352,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;
 

@@ -1380,7 +1396,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;
 
 	repo_config_get_bool(r, "repack.usedeltabaseoffset", &delta_base_offset);

index 030a7222b2a..67afe1bb8d9 100755
@@ -538,6 +538,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 &&

gitgitgadget

When there is no file whose name ends with idx, what happens?

    $ ls *idx && echo OK
    ls: cannot access '*idx': No such file or directory
    $ echo *idx && echo OK
    *idx
    OK

Likewise.

Good suggestion, but doesn't .keep file record why the pack is kept in real life (i.e. not an empty file)?

"sed -n 3p" is the only valid way to write it ;-)

Yes, but I think the next line is checking for the number of lines. This is better to fail faster.

(My suggestion was wrong anyway, it should be "printf "%s\\n" *idx)

Yes, in real life, we usually provide a reason in this .keep file. But, we also allow empty file with git-index-pack --keep I think simple touch is fine for this test.

Missing piece for my previous command:
if keep-list is empty, we may want to fail fast, touch with empty list will error out (at least in my system).

Danh

Hi,

Thanks Danh and Junio for the testing improvement suggestions. I think these are the points I will adopt into next version:

- Remove the 3rd patch and keep the removal of dollar sign locally inside repack respects repack.packKeptObjects=false.

- Change head -n -3 | tail -n -1 to sed -n 3p

- Apply test_line_count on keep-list for failing fast (before touch)

Cheers,
Son Luong.

From: Son Luong Ngoc <[email protected]>

As the old versions of dash is deprecated, dollar-sign inside artihmetic expansion is no longer needed.
This ensures t5319 follows the coding guideline updated
in 'jk/arith-expansion-coding-guidelines' 6d4bf5813cd2c1a3b93fd4f0b231733f82133cce.

---
 t/t5319-multi-pack-index.sh | 38 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

index 67afe1bb8d9..065f48747f3 100755
@@ -62,8 +62,8 @@ generate_objects () {
 	} >wide_delta_$iii &&
 	{
 		test-tool genrandom "foo"$i 100 &&
-		test-tool genrandom "foo"$(( $i + 1 )) 100 &&
-		test-tool genrandom "foo"$(( $i + 2 )) 100
+		test-tool genrandom "foo"$(( i + 1 )) 100 &&
+		test-tool genrandom "foo"$(( i + 2 )) 100
 	} >deep_delta_$iii &&
 	{
 		echo $iii &&

@@ -251,21 +251,21 @@ MIDX_BYTE_OID_VERSION=5
 MIDX_BYTE_CHUNK_COUNT=6
 MIDX_HEADER_SIZE=12
 MIDX_BYTE_CHUNK_ID=$MIDX_HEADER_SIZE
-MIDX_BYTE_CHUNK_OFFSET=$(($MIDX_HEADER_SIZE + 4))
+MIDX_BYTE_CHUNK_OFFSET=$((MIDX_HEADER_SIZE + 4))
 MIDX_NUM_CHUNKS=5
 MIDX_CHUNK_LOOKUP_WIDTH=12
-MIDX_OFFSET_PACKNAMES=$(($MIDX_HEADER_SIZE + \
-			 $MIDX_NUM_CHUNKS * $MIDX_CHUNK_LOOKUP_WIDTH))
-MIDX_BYTE_PACKNAME_ORDER=$(($MIDX_OFFSET_PACKNAMES + 2))
-MIDX_OFFSET_OID_FANOUT=$(($MIDX_OFFSET_PACKNAMES + $(test_oid packnameoff)))
+MIDX_OFFSET_PACKNAMES=$((MIDX_HEADER_SIZE + \
+			 MIDX_NUM_CHUNKS * MIDX_CHUNK_LOOKUP_WIDTH))
+MIDX_BYTE_PACKNAME_ORDER=$((MIDX_OFFSET_PACKNAMES + 2))
+MIDX_OFFSET_OID_FANOUT=$((MIDX_OFFSET_PACKNAMES + $(test_oid packnameoff)))
 MIDX_OID_FANOUT_WIDTH=4
-MIDX_BYTE_OID_FANOUT_ORDER=$((MIDX_OFFSET_OID_FANOUT + 250 * $MIDX_OID_FANOUT_WIDTH + $(test_oid fanoutoff)))
-MIDX_OFFSET_OID_LOOKUP=$(($MIDX_OFFSET_OID_FANOUT + 256 * $MIDX_OID_FANOUT_WIDTH))
-MIDX_BYTE_OID_LOOKUP=$(($MIDX_OFFSET_OID_LOOKUP + 16 * $HASH_LEN))
-MIDX_OFFSET_OBJECT_OFFSETS=$(($MIDX_OFFSET_OID_LOOKUP + $NUM_OBJECTS * $HASH_LEN))
+MIDX_BYTE_OID_FANOUT_ORDER=$((MIDX_OFFSET_OID_FANOUT + 250 * MIDX_OID_FANOUT_WIDTH + $(test_oid fanoutoff)))
+MIDX_OFFSET_OID_LOOKUP=$((MIDX_OFFSET_OID_FANOUT + 256 * MIDX_OID_FANOUT_WIDTH))
+MIDX_BYTE_OID_LOOKUP=$((MIDX_OFFSET_OID_LOOKUP + 16 * HASH_LEN))
+MIDX_OFFSET_OBJECT_OFFSETS=$((MIDX_OFFSET_OID_LOOKUP + NUM_OBJECTS * HASH_LEN))
 MIDX_OFFSET_WIDTH=8
-MIDX_BYTE_PACK_INT_ID=$(($MIDX_OFFSET_OBJECT_OFFSETS + 16 * $MIDX_OFFSET_WIDTH + 2))
-MIDX_BYTE_OFFSET=$(($MIDX_OFFSET_OBJECT_OFFSETS + 16 * $MIDX_OFFSET_WIDTH + 6))
+MIDX_BYTE_PACK_INT_ID=$((MIDX_OFFSET_OBJECT_OFFSETS + 16 * MIDX_OFFSET_WIDTH + 2))
+MIDX_BYTE_OFFSET=$((MIDX_OFFSET_OBJECT_OFFSETS + 16 * MIDX_OFFSET_WIDTH + 6))
 
 test_expect_success 'verify bad version' '
 	corrupt_midx_and_verify $MIDX_BYTE_VERSION "\00" $objdir \

@@ -417,10 +417,10 @@ test_expect_success 'verify multi-pack-index with 64-bit offsets' '
 
 NUM_OBJECTS=63
 MIDX_OFFSET_OID_FANOUT=$((MIDX_OFFSET_PACKNAMES + 54))
-MIDX_OFFSET_OID_LOOKUP=$((MIDX_OFFSET_OID_FANOUT + 256 * $MIDX_OID_FANOUT_WIDTH))
-MIDX_OFFSET_OBJECT_OFFSETS=$(($MIDX_OFFSET_OID_LOOKUP + $NUM_OBJECTS * $HASH_LEN))
-MIDX_OFFSET_LARGE_OFFSETS=$(($MIDX_OFFSET_OBJECT_OFFSETS + $NUM_OBJECTS * $MIDX_OFFSET_WIDTH))
-MIDX_BYTE_LARGE_OFFSET=$(($MIDX_OFFSET_LARGE_OFFSETS + 3))
+MIDX_OFFSET_OID_LOOKUP=$((MIDX_OFFSET_OID_FANOUT + 256 * MIDX_OID_FANOUT_WIDTH))
+MIDX_OFFSET_OBJECT_OFFSETS=$((MIDX_OFFSET_OID_LOOKUP + NUM_OBJECTS * HASH_LEN))
+MIDX_OFFSET_LARGE_OFFSETS=$((MIDX_OFFSET_OBJECT_OFFSETS + NUM_OBJECTS * MIDX_OFFSET_WIDTH))
+MIDX_BYTE_LARGE_OFFSET=$((MIDX_OFFSET_LARGE_OFFSETS + 3))
 
 test_expect_success 'verify incorrect 64-bit offset' '
 	corrupt_midx_and_verify $MIDX_BYTE_LARGE_OFFSET "\07" objects64 \

@@ -555,7 +555,7 @@ test_expect_success 'repack respects repack.packKeptObjects=false' '
 		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)) &&
+		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 &&

@@ -570,7 +570,7 @@ test_expect_success 'repack creates a new pack' '
 		ls .git/objects/pack/*idx >idx-list &&
 		test_line_count = 5 idx-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)) &&
+		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 = 6 idx-list &&

gitgitgadget

That does not match my understanding of the guideline. By removing the "dollar required" rule and not adding a new "dollar forbidden" rule, we pretty much declared that "we do not care much either way" [*1*].

Even if we cared, "Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up." rule from the guidelines applies here.

Thanks.

[Reference]

*1* https://lore.kernel.org/git/[email protected]/

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.

14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack
9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack

Latest 2.26.2 (without delta_base_offset)

9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack

With delta_base_offset

git version 2.26.2.672.g232c24e857.dirty

9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack

Note that repack.writeBitmaps configuration is ignored, as the pack bitmap facility is useful only with a single packfile.

Derrick Stolee's following patch will address repack.packKeptObjects support.

Derrick Stolee (1):
multi-pack-index: respect repack.packKeptObjects=false

Son Luong Ngoc (1):
midx: teach "git multi-pack-index repack" honor "git repack"
configurations

 Documentation/git-multi-pack-index.txt |  3 ++
 midx.c                                 | 42 +++++++++++++++++++++++---
 t/t5319-multi-pack-index.sh            | 27 +++++++++++++++++
 3 files changed, 67 insertions(+), 5 deletions(-)

base-commit: b994622632154fc3b17fb40a38819ad954a5fb88
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/626

Range-diff vs v3:

1: a925307d4c5 ! 1: a8f75e34e5b midx: teach "git multi-pack-index repack" honor "git repack" configurations
@@ Metadata
## Commit message ##
midx: teach "git multi-pack-index repack" honor "git repack" configurations

- 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 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.

- 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.
+ 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.
@@ Commit message

## midx.c ##
@@ midx.c: 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);
+
++ /*
++ * 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;
-
++
if (!m)
return 0;
+
@@ midx.c: 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;
2: 988697dd512 ! 2: 192fc785382 multi-pack-index: respect repack.packKeptObjects=false

     @@ t/t5319-multi-pack-index.sh: test_expect_success 'repack with minimum size does
      +		ls .git/objects/pack/*idx >idx-list &&
      +		test_line_count = 5 idx-list &&
      +		ls .git/objects/pack/*.pack | sed "s/\.pack/.keep/" >keep-list &&
     ++		test_line_count = 5 keep-list &&
      +		for keep in $(cat keep-list)
      +		do
      +			touch $keep || return 1
     @@ t/t5319-multi-pack-index.sh: test_expect_success 'repack with minimum size does
      +		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)) &&
     ++		THIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | sed -n 3p) &&
     ++		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 &&

3: efeb3d7d132 < -: ----------- Ensured t5319 follows arith expansion guideline

gitgitgadget

From: Son Luong Ngoc <[email protected]>

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.

---
 midx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index 9a61d3b37d9..d2a43bd1a38 100644
@@ -1370,6 +1370,14 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 	struct strbuf base_name = STRBUF_INIT;
 	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
 
+	/*
+	 * 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;
+
 	if (!m)
 		return 0;
 

@@ -1381,12 +1389,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

gitgitgadget

From: Derrick Stolee <[email protected]>

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.

---
 Documentation/git-multi-pack-index.txt |  3 +++
 midx.c                                 | 26 ++++++++++++++++++++-----
 t/t5319-multi-pack-index.sh            | 27 ++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 5 deletions(-)

index 642d9ac5b72..0c6619493c1 100644
@@ -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

index d2a43bd1a38..6d1584ca51d 100644
@@ -1293,15 +1293,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,

@@ -1312,6 +1323,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;

@@ -1338,6 +1352,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;
 

@@ -1386,7 +1402,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;
 
 	repo_config_get_bool(r, "repack.usedeltabaseoffset", &delta_base_offset);

index 030a7222b2a..7214cab36c0 100755
@@ -538,6 +538,33 @@ 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 &&
+		test_line_count = 5 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 | sed -n 3p) &&
+		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 &&

gitgitgadget