repack: add --filter=

From: John Cai <[email protected]>

Currently, repack does not work with partial clones. When repack is run on a partially cloned repository, it grabs all missing objects from promisor remotes. This also means that when gc is run for repository maintenance on a partially cloned repository, it will end up getting missing objects, which is not what we want.

In order to make repack work with partial clone, teach repack a new option --filter, which takes a <filter-spec> argument. repack will skip any objects that are matched by <filter-spec> similar to how the clone command will skip fetching certain objects.

The final goal of this feature, is to be able to store objects on a server other than the regular git server itself.

There are several scripts added so we can test the process of using a remote helper to upload blobs to an http server:

- t/lib-httpd/list.sh lists blobs uploaded to the http server.
- t/lib-httpd/upload.sh uploads blobs to the http server.
- t/t0410/git-remote-testhttpgit a remote helper that can access blobs onto from an http server. Copied over from t/t5801/git-remote-testhttpgit and modified to upload blobs to an http server.
- t/t0410/lib-http-promisor.sh convenience functions for uploading blobs

builtin/repack.c +1 -0

+		}
 	}
 	fclose(out);
 	ret = finish_command(&cmd);

t/t7700-repack.sh +0 -0

index e489869dd94..78cc1858cb6 100755
@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
 	test_must_be_empty actual
 '
 

Quoted 7 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

Currently, repack does not work with partial clones. When repack is run on a partially cloned repository, it grabs all missing objects from promisor remotes. This also means that when gc is run for repository maintenance on a partially cloned repository, it will end up getting missing objects, which is not what we want.

This shouldn't be what is happening. Do you have a demonstration of this happening? repack_promisor_objects() should be avoiding following links outside of promisor packs so we can safely 'git gc' in a partial clone without downloading all reachable blobs.

Quoted 5 lines

In order to make repack work with partial clone, teach repack a new option --filter, which takes a <filter-spec> argument. repack will skip any objects that are matched by <filter-spec> similar to how the clone command will skip fetching certain objects.

This is a bit misleading, since 'git clone' doesn't "skip fetching", but instead requests a filter and the server can choose to write a pack-file using that filter. I'm not sure if it's worth how pedantic I'm being here.

The thing that I find confusing here is that you are adding an option that could be run on a _full_ repository. If I have a set of packs and none of them are promisor (I have every reachable object), then what is the end result after 'git repack -adf --filter=blob:none'? Those existing pack-files shouldn't be deleted because they have objects that are not in the newly-created pack-file.

I'd like to see some additional clarity on this before continuing to review this series.

Quoted 7 lines

The final goal of this feature, is to be able to store objects on a server other than the regular git server itself.

There are several scripts added so we can test the process of using a remote helper to upload blobs to an http server:

Quoted 7 lines

- t/lib-httpd/list.sh lists blobs uploaded to the http server.
- t/lib-httpd/upload.sh uploads blobs to the http server.
- t/t0410/git-remote-testhttpgit a remote helper that can access blobs onto from an http server. Copied over from t/t5801/git-remote-testhttpgit and modified to upload blobs to an http server.
- t/t0410/lib-http-promisor.sh convenience functions for uploading blobs

I think these changes to the tests should be extracted to a new patch where this can be discussed in more detail. I didn't look too closely at them because I want to focus on whether this --filter option is a good direction for 'git repack'.

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 830

Quoted 9 lines

builtin/repack.c +4 -0

 		OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),

@@ -819,6 +824,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only from pack-objects."));
 		string_list_append(&names, line.buf);
+		if (po_args.filter) {
+			char *promisor_name = mkpathdup("%s-%s.promisor", packtmp,
+							line.buf);
+			write_promisor_file(promisor_name, NULL, 0);

This code is duplicated in repack_promisor_objects(), so it would be good to extract that logic into a helper method called by both places.

Quoted 14 lines

builtin/repack.c +1 -0

+		}
 	}
 	fclose(out);
 	ret = finish_command(&cmd);

t/t7700-repack.sh +0 -0

index e489869dd94..78cc1858cb6 100755
@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
 	test_must_be_empty actual
 '
 

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 249

Quoted 10 lines

t/t7700-repack.sh +10 -0

+test_expect_success 'repack with filter does not fetch from remote' '
+	rm -rf server client &&
+	test_create_repo server &&
+	git -C server config uploadpack.allowFilter true &&
+	git -C server config uploadpack.allowAnySHA1InWant true &&
+	echo content1 >server/file1 &&
+	git -C server add file1 &&
+	git -C server commit -m initial_commit &&
+	expected="?$(git -C server rev-parse :file1)" &&
+	git clone --bare --no-local server client &&

You could use "file:://$(pwd)/server" here instead of "server".

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 251

Quoted 3 lines

t/t7700-repack.sh +2 -0

+	git -C client config remote.origin.promisor true &&
+	git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&

This isn't testing what you want it to test, because your initial clone doesn't use --filter=blob:none, so you already have all of the objects in the client. You would never trigger a need for a fetch from the remote.

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 255

Quoted 5 lines

t/t7700-repack.sh +4 -0

+	git -C client rev-list --objects --all --missing=print >objects &&
+	grep "$expected" objects &&
+	git -C client repack -a -d &&
+	expected="$(git -C server rev-parse :file1)" &&

This is signalling to me that you are looking for a remote fetch now that you are repacking everything, and that can only happen if you deleted objects from the client during your first repack. That seems incorrect.

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 258

Quoted 3 lines

t/t7700-repack.sh +3 -0

+	git -C client rev-list --objects --all --missing=print >objects &&
+	grep "$expected" objects
+'

Based on my current understanding, this patch seems unnecessary (repacks should already be doing the right thing when in the presence of a partial clone) and incorrect (we should not delete existing reachable objects when repacking with a filter).

I look forward to hearing more about your intended use of this feature so we can land on a better way to solve the problems you are having.

Thanks,
-Stolee

Apologies for the lack of clarity. Indeed, I can see why this is the most important detail of this patch to provide enough context on, as it involves deleting objects from a full repository as you said.

To back up a little, the goal is to be able to offload large blobs to a separate http server. Christian Couder has a demo [1] that shows this in detail.

If we had the following:
A. an http server to use as a generalized object store B. a server update hook that uploads large blobs to 1.

C. a git server
D. a regular job that runs git repack --filter to remove large blobs from C.

Clients would need to configure both C) and A) as promisor remotes to

be able to get everything. When they push new large blobs, they can still push them to C), as B) will upload them to A), and D) will regularly remove those large blobs from C).

This way with a little bit of client and server configuration, we can have a native way to support offloading large files without git LFS.
It would be more flexible as you can easily tweak which blobs are considered large files by tweaking B) and D).

Hi Stolee,

Thanks for taking the time to review this patch! I added some points of clarification down below.

Quoted 9 lines · Derrick Stolee

Quoted 7 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

Currently, repack does not work with partial clones. When repack is run on a partially cloned repository, it grabs all missing objects from promisor remotes. This also means that when gc is run for repository maintenance on a partially cloned repository, it will end up getting missing objects, which is not what we want.

Quoted 4 lines

This shouldn't be what is happening. Do you have a demonstration of this happening? repack_promisor_objects() should be avoiding following links outside of promisor packs so we can safely 'git gc' in a partial clone without downloading all reachable blobs.

You're right, sorry I was mistaken about this detail of how partial clones work.

Quoted 10 lines

Quoted 5 lines

In order to make repack work with partial clone, teach repack a new option --filter, which takes a <filter-spec> argument. repack will skip any objects that are matched by <filter-spec> similar to how the clone command will skip fetching certain objects.

This is a bit misleading, since 'git clone' doesn't "skip fetching", but instead requests a filter and the server can choose to write a pack-file using that filter. I'm not sure if it's worth how pedantic I'm being here.

Thanks for the more precise description of the mechanics of partial clone. I'll improve the wording in the next version of this patch series.

Quoted 10 lines

The thing that I find confusing here is that you are adding an option that could be run on a _full_ repository. If I have a set of packs and none of them are promisor (I have every reachable object), then what is the end result after 'git repack -adf --filter=blob:none'? Those existing pack-files shouldn't be deleted because they have objects that are not in the newly-created pack-file.

I'd like to see some additional clarity on this before continuing to review this series.

Apologies for the lack of clarity. Indeed, I can see why this is the most important detail of this patch to provide enough context on, as it involves deleting objects from a full repository as you said.

To back up a little, the goal is to be able to offload large blobs to a separate http server. Christian Couder has a demo [1] that shows this in detail.

If we had the following:
A. an http server to use as a generalized object store B. a server update hook that uploads large blobs to 1. C. a git server
D. a regular job that runs git repack --filter to remove large blobs from C.

Clients would need to configure both C) and A) as promisor remotes to be able to get everything. When they push new large blobs, they can still push them to C), as B) will upload them to A), and D) will regularly remove those large blobs from C).

This way with a little bit of client and server configuration, we can have a native way to support offloading large files without git LFS.
It would be more flexible as you can easily tweak which blobs are considered large files by tweaking B) and D).

Quoted 20 lines

Quoted 7 lines

The final goal of this feature, is to be able to store objects on a server other than the regular git server itself.

There are several scripts added so we can test the process of using a remote helper to upload blobs to an http server:

Quoted 7 lines

- t/lib-httpd/list.sh lists blobs uploaded to the http server.
- t/lib-httpd/upload.sh uploads blobs to the http server.
- t/t0410/git-remote-testhttpgit a remote helper that can access blobs onto from an http server. Copied over from t/t5801/git-remote-testhttpgit and modified to upload blobs to an http server.
- t/t0410/lib-http-promisor.sh convenience functions for uploading blobs

I think these changes to the tests should be extracted to a new patch where this can be discussed in more detail. I didn't look too closely at them because I want to focus on whether this --filter option is a good direction for 'git repack'.

Quoted 12 lines

builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 830

Quoted 9 lines

builtin/repack.c +4 -0

 		OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),

@@ -819,6 +824,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only from pack-objects."));
 		string_list_append(&names, line.buf);
+		if (po_args.filter) {
+			char *promisor_name = mkpathdup("%s-%s.promisor", packtmp,
+							line.buf);
+			write_promisor_file(promisor_name, NULL, 0);

This code is duplicated in repack_promisor_objects(), so it would be good to extract that logic into a helper method called by both places.

Thanks for pointing this out. I'll incorporate this into the next version.

Quoted 14 lines

Quoted 14 lines

builtin/repack.c +1 -0

+		}
 	}
 	fclose(out);
 	ret = finish_command(&cmd);

t/t7700-repack.sh +0 -0

index e489869dd94..78cc1858cb6 100755
@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
 	test_must_be_empty actual
 '
 

Quoted 12 lines

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 249

Quoted 10 lines

t/t7700-repack.sh +10 -0

+test_expect_success 'repack with filter does not fetch from remote' '
+	rm -rf server client &&
+	test_create_repo server &&
+	git -C server config uploadpack.allowFilter true &&
+	git -C server config uploadpack.allowAnySHA1InWant true &&
+	echo content1 >server/file1 &&
+	git -C server add file1 &&
+	git -C server commit -m initial_commit &&
+	expected="?$(git -C server rev-parse :file1)" &&
+	git clone --bare --no-local server client &&

You could use "file:://$(pwd)/server" here instead of "server".

good point, thanks

Quoted 7 lines

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 251

Quoted 3 lines

t/t7700-repack.sh +2 -0

+	git -C client config remote.origin.promisor true &&
+	git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&

This isn't testing what you want it to test, because your initial clone doesn't use --filter=blob:none, so you already have all of the objects in the client. You would never trigger a need for a fetch from the remote.

right, so this test is actually testing that repack --filter would shed objects to show that it can be used as D) as a regular cleanup job for git servers that utilize another http server to host large blobs.

Quoted 20 lines

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 255

Quoted 5 lines

t/t7700-repack.sh +4 -0

+	git -C client rev-list --objects --all --missing=print >objects &&
+	grep "$expected" objects &&
+	git -C client repack -a -d &&
+	expected="$(git -C server rev-parse :file1)" &&

This is signalling to me that you are looking for a remote fetch now that you are repacking everything, and that can only happen if you deleted objects from the client during your first repack. That seems incorrect.

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 258

Quoted 3 lines

t/t7700-repack.sh +3 -0

+	git -C client rev-list --objects --all --missing=print >objects &&
+	grep "$expected" objects
+'

Based on my current understanding, this patch seems unnecessary (repacks should already be doing the right thing when in the presence of a partial clone) and incorrect (we should not delete existing reachable objects when repacking with a filter).

Quoted 2 lines

I look forward to hearing more about your intended use of this feature so we can land on a better way to solve the problems you are having.

Thanks for the callouts on the big picture of this proposed change. Looking forward to getting your thoughts on this!

Quoted 3 lines

Thanks,
-Stolee

This patch series makes partial clone more useful by making it possible to run repack to remove objects from a repository (replacing it with promisor objects). This is useful when we want to offload large blobs from a git server onto another git server, or even use an http server through a remote helper.

In [A], a --refilter option on fetch and fetch-pack is being discussed where either a less restrictive or more restrictive filter can be used. In the more restrictive case, the objects that already exist will not be deleted. But, one can imagine that users might want the ability to delete objects when they apply a more restrictive filter in order to save space, and this patch series would also allow that.

There are a couple of things we need to adjust to make this possible. This patch has three parts.

1. Allow --filter in pack-objects without --stdout
2. Add a --filter flag for repack
3. Allow missing promisor objects in upload-pack
4. Tests that demonstrate the ability to offload objects onto an http remote

cc: Christian Couder [email protected] cc: Derrick Stolee [email protected] cc: Robert Coup [email protected]

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

John Cai (4):
pack-objects: allow --filter without --stdout
repack: add --filter=<filter-spec> option
upload-pack: allow missing promisor objects
tests for repack --filter mode

 Documentation/git-repack.txt   |   5 +
 builtin/pack-objects.c         |   2 -
 builtin/repack.c               |  22 +++--
 t/lib-httpd.sh                 |   2 +
 t/lib-httpd/apache.conf        |   8 ++
 t/lib-httpd/list.sh            |  43 +++++++++
 t/lib-httpd/upload.sh          |  46 +++++++++
 t/t0410-partial-clone.sh       |  81 ++++++++++++++++
 t/t0410/git-remote-testhttpgit | 170 +++++++++++++++++++++++++++++++++
 t/t7700-repack.sh              |  20 ++++
 upload-pack.c                  |   5 +
 11 files changed, 395 insertions(+), 9 deletions(-)
 create mode 100644 t/lib-httpd/list.sh
 create mode 100644 t/lib-httpd/upload.sh
 create mode 100755 t/t0410/git-remote-testhttpgit

base-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1206%2Fjohn-cai%2Fjc-repack-filter-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1206/john-cai/jc-repack-filter-v2
Pull-Request: https://github.com/git/git/pull/1206

Range-diff vs v1:

 1:  0eec9b117da = 1:  f43b76ca650 pack-objects: allow --filter without --stdout
 -:  ----------- > 2:  6e7c8410b8d repack: add --filter=<filter-spec> option
 -:  ----------- > 3:  40612b9663b upload-pack: allow missing promisor objects
 2:  a3166381572 ! 4:  d76faa1f16e repack: add --filter=<filter-spec> option

@@ Metadata
Author: John Cai <[email protected]>

## Commit message ##
- repack: add --filter=<filter-spec> option
+ tests for repack --filter mode

- Currently, repack does not work with partial clones. When repack is run
- on a partially cloned repository, it grabs all missing objects from
- promisor remotes. This also means that when gc is run for repository
- maintenance on a partially cloned repository, it will end up getting
- missing objects, which is not what we want.
-
- In order to make repack work with partial clone, teach repack a new
- option --filter, which takes a <filter-spec> argument. repack will skip
- any objects that are matched by <filter-spec> similar to how the clone
- command will skip fetching certain objects.
-
- The final goal of this feature, is to be able to store objects on a
- server other than the regular git server itself.
+ This patch adds tests to test both repack --filter functionality in
+ isolation (in t7700-repack.sh) as well as how it can be used to offload
+ large blobs (in t0410-partial-clone.sh)

There are several scripts added so we can test the process of using a
- remote helper to upload blobs to an http server:
+ remote helper to upload blobs to an http server.

- t/lib-httpd/list.sh lists blobs uploaded to the http server.
- t/lib-httpd/upload.sh uploads blobs to the http server.
@@ Commit message

Based-on-patch-by Christian Couder <[email protected]>

Signed-off-by John Cai <[email protected]>

- ## Documentation/git-repack.txt ##
-@@ Documentation/git-repack.txt: depth is 4095.
- a larger and slower repository; see the discussion in
- pack.packSizeLimit.
-
-+--filter=<filter-spec>::
-+ Omits certain objects (usually blobs) from the resulting
-+ packfile. See linkgit:git-rev-list[1] for valid
-+ <filter-spec> forms.
-+
- -b::
- --write-bitmap-index::
- Write a reachability bitmap index as part of the repack. This
-
- ## builtin/repack.c ##
-@@ builtin/repack.c: struct pack_objects_args {
- const char *depth;
- const char *threads;
- const char *max_pack_size;
-+ const char *filter;
- int no_reuse_delta;
- int no_reuse_object;
- int quiet;
-@@ builtin/repack.c: static void prepare_pack_objects(struct child_process *cmd,
- strvec_pushf(&cmd->args, "--threads=%s", args->threads);
- if (args->max_pack_size)
- strvec_pushf(&cmd->args, "--max-pack-size=%s", args->max_pack_size);
-+ if (args->filter)
-+ strvec_pushf(&cmd->args, "--filter=%s", args->filter);
- if (args->no_reuse_delta)
- strvec_pushf(&cmd->args, "--no-reuse-delta");
- if (args->no_reuse_object)
-@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
- N_("limits the maximum number of threads")),
- OPT_STRING(0, "max-pack-size", &po_args.max_pack_size, N_("bytes"),
- N_("maximum size of each packfile")),
-+ OPT_STRING(0, "filter", &po_args.filter, N_("args"),
-+ N_("object filtering")),
- OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
- N_("repack objects in packs marked with .keep")),
- OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
-@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
- if (line.len != the_hash_algo->hexsz)
- die(_("repack: Expecting full hex object ID lines only from pack-objects."));
- string_list_append(&names, line.buf);
-+ if (po_args.filter) {
-+ char *promisor_name = mkpathdup("%s-%s.promisor", packtmp,
-+ line.buf);
-+ write_promisor_file(promisor_name, NULL, 0);
-+ }
- }
- fclose(out);
- ret = finish_command(&cmd);
-
## t/lib-httpd.sh ##
@@ t/lib-httpd.sh: prepare_httpd() {
install_script error-smart-http.sh
@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects from
+ git -C server rev-list --objects --all --missing=print >objects &&
+ grep "$sha" objects
+'
++
++test_expect_success 'fetch does not cause server to fetch missing objects' '
++ rm -rf origin server client &&
++ test_create_repo origin &&
++ dd if=/dev/zero of=origin/file1 bs=801k count=1 &&
++ git -C origin add file1 &&
++ git -C origin commit -m "large blob" &&
++ sha="$(git -C origin rev-parse :file1)" &&
++ expected="?$(git -C origin rev-parse :file1)" &&
++ git clone --bare --no-local origin server &&
++ git -C server remote add httpremote "testhttpgit::${PWD}/server" &&
++ git -C server config remote.httpremote.promisor true &&
++ git -C server config --remove-section remote.origin &&
++ git -C server rev-list --all --objects --filter-print-omitted \
++ --filter=blob:limit=800k | perl -ne "print if s/^[~]//" \
++ >large_blobs.txt &&
++ upload_blobs_from_stdin server <large_blobs.txt &&
++ git -C server -c repack.writebitmaps=false repack -a -d \
++ --filter=blob:limit=800k &&
++ git -C server config uploadpack.allowmissingpromisor true &&
++ git clone -c remote.httpremote.url="testhttpgit::${PWD}/server" \
++ -c remote.httpremote.fetch='+refs/heads/*:refs/remotes/httpremote/*' \
++ -c remote.httpremote.promisor=true --bare --no-local \
++ --filter=blob:limit=800k server client &&
++ git -C client rev-list --objects --all --missing=print >client_objects &&
++ grep "$expected" client_objects &&
++ git -C server rev-list --objects --all --missing=print >server_objects &&
++ grep "$expected" server_objects
++'
+
# DO NOT add non-httpd-specific tests here, because the last part of this # test script is only executed when httpd is available and enabled.

Signature · 1 line

gitgitgadget

t/t7700-repack.sh +2 -0

index e489869dd94..78cc1858cb6 100755
@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
        test_must_be_empty actual
 '
 
+test_expect_success 'repack with filter does not fetch from remote' '
+       rm -rf server client &&

Hi John,

Minor, but should we use oid rather than sha1 in the list.sh/upload.sh scripts? wrt sha256 slowly coming along the pipe.

Quoted 9 lines

t/t7700-repack.sh +0 -0

index e489869dd94..78cc1858cb6 100755
@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '
        test_must_be_empty actual
 '
 

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 251

Quoted 12 lines

t/t7700-repack.sh +12 -0

+test_expect_success 'repack with filter does not fetch from remote' '
+       rm -rf server client &&
+       test_create_repo server &&
+       git -C server config uploadpack.allowFilter true &&
+       git -C server config uploadpack.allowAnySHA1InWant true &&
+       echo content1 >server/file1 &&
+       git -C server add file1 &&
+       git -C server commit -m initial_commit &&
+       expected="?$(git -C server rev-parse :file1)" &&
+       git clone --bare --no-local server client &&
+       git -C client config remote.origin.promisor true &&
+       git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&

Does writing bitmaps have any effect/interaction here?

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 253

Quoted 3 lines

t/t7700-repack.sh +2 -0

+       git -C client rev-list --objects --all --missing=print >objects &&
+       grep "$expected" objects &&

This is testing the object that was cloned initially is gone after the repack, ok.

t/t7700-repack.sh test_expect_success 'auto-bitmaps do not complain if unavailable' ' · line 257

Quoted 4 lines

t/t7700-repack.sh +4 -0

+       git -C client repack -a -d &&
+       expected="$(git -C server rev-parse :file1)" &&
+       git -C client rev-list --objects --all --missing=print >objects &&
+       grep "$expected" objects

But I'm not sure what you're testing here? A repack wouldn't fetch missing objects for a promisor pack anyway... and because there's no '^' in the pattern the grep will succeed regardless of whether the object is missing/present.

Rob :)

This patch series makes partial clone more useful by making it possible to run repack to remove objects from a repository (replacing it with promisor objects). This is useful when we want to offload large blobs from a git server onto another git server, or even use an http server through a remote helper.

In [A], a --refilter option on fetch and fetch-pack is being discussed where either a less restrictive or more restrictive filter can be used. In the more restrictive case, the objects that already exist will not be deleted. But, one can imagine that users might want the ability to delete objects when they apply a more restrictive filter in order to save space, and this patch series would also allow that.

Hi John,

Quoted 7 lines · John Cai via GitGitGadget

This patch series makes partial clone more useful by making it possible to run repack to remove objects from a repository (replacing it with promisor objects). This is useful when we want to offload large blobs from a git server onto another git server, or even use an http server through a remote helper.

Quoted 6 lines

In [A], a --refilter option on fetch and fetch-pack is being discussed where either a less restrictive or more restrictive filter can be used. In the more restrictive case, the objects that already exist will not be deleted. But, one can imagine that users might want the ability to delete objects when they apply a more restrictive filter in order to save space, and this patch series would also allow that.

This all makes sense to me, and the implementation is remarkably short - gluing together capabilities that are already there, and writing tests.

*But*, running repack --filter drops objects from the object db. That seems like
a capability Git shouldn't idly expose without people understanding the consequences - mostly that they really have another copy elsewhere or they will lose data, and it won't necessarily be obvious for a long time. Otherwise it is a footgun.

I don't know whether that is just around naming (--delete-filter / --drop-filter /
--expire-filter ?), and/or making the documentation very explicit that this isn't so
much "omitting certain objects from a packfile" as irretrievably deleting objects.

Rob :)

Quoted 6 lines

I don't know whether that is just around naming (--delete-filter / --drop-filter /
--expire-filter ?), and/or making the documentation very explicit that this isn't so
much "omitting certain objects from a packfile" as irretrievably deleting objects.

Yeah, making the name very clear (I kind of like --delete-filter) would certainly help. Also, to have more protection we can either

1. add a config value that needs to be set to true for repack to remove objects (repack.allowDestroyFilter).

2. --filter is dry-run by default and prints out objects that would have been removed, and it has to be combined with another flag --destroy in order for it to actually remove objects from the odb.

we would still be leaving repository
corruption on the table, just making it marginally more difficult to achieve.

Hi,

Quoted 4 lines · Taylor Blau

we would still be leaving repository
corruption on the table, just making it marginally more difficult to achieve.

While reviewing John's patch I initially wondered if a better approach might be something like git repack -a -d --exclude-stdin, passing a list of specific objects to exclude from the new pack (sourced from rev-list via a filter, etc). To me this seems like a less dangerous approach, but my concern was it doesn't use the existing filter capabilities of pack-objects, and we end up generating and passing around a huge list of oids. And of course any mistakes in the list generation aren't visible until it's too late.

I also wonder whether there's a race condition if the repository gets updated? If you're moving large objects out in advance, then filtering the remainder there's nothing to stop a new large object being pushed between those two steps and getting dropped.

My other idea, which is growing on me, is whether repack could generate two valid packs: one for the included objects via the filter (as John's change does now), and one containing the filtered-out objects. git repack -a -d --split-filter=<filter> Then a user could then move/extract the second packfile to object storage, but there'd be no way to *accidentally* corrupt the repository by using a bad option. With this approach the race condition above goes away too.

    $ git repack -a -d -q --split-filter=blob:limit=1m
    pack-37b7443e3123549a2ddee31f616ae272c51cae90
    pack-10789d94fcd99ffe1403b63b167c181a9df493cd

First pack identifier being the objects that match the filter (ie:
commits/trees/blobs <1m), and the second pack identifier being the objects that are excluded by the filter (blobs >1m).

An astute --i-know-what-im-doing reader could spot that you could just delete the second packfile and achieve the same outcome as the current proposed patch, subject to being confident the race condition hadn't happened to you.

Thanks,
Rob :)

Quoted 8 lines · John Cai

Quoted 6 lines

I don't know whether that is just around naming (--delete-filter / --drop-filter /
--expire-filter ?), and/or making the documentation very explicit that this isn't so
much "omitting certain objects from a packfile" as irretrievably deleting objects.

Yeah, making the name very clear (I kind of like --delete-filter) would certainly help.

Quoted 4 lines

Also, to have more protection we can either

1. add a config value that needs to be set to true for repack to remove objects (repack.allowDestroyFilter).

Quoted 3 lines

2. --filter is dry-run by default and prints out objects that would have been removed, and it has to be combined with another flag --destroy in order for it to actually remove objects from the odb.

I share the same concern as Robert and Stolee do. But I think this issue goes deeper than just naming.

Even if we called this git repack --delete-filter and only ran it with --i-know-what-im-doing flag, we would still be leaving repository corruption on the table, just making it marginally more difficult to achieve.

I'm not familiar enough with the proposal to comment authoritatively, but it seems like we should be verifying that there is a promisor remote which promises any objects that we are about to filter out of the repository.

But as it stands right now, I worry that this feature is too easily misused and could result in unintended repository corruption.

I think verifying that that any objects we're about to delete exist somewhere should make this safer to use, though even then, I think we're still open to a TOCTOU race whereby the promisor has the objects when we're about to delete them (convincing Git that deleting those objects is OK to do) but gets rid of them after objects have been deleted from the local copy (leaving no copies of the object around).

Quoted 4 lines

Quoted 4 lines

Also, to have more protection we can either

1. add a config value that needs to be set to true for repack to remove objects (repack.allowDestroyFilter).

I don't think it's of much value. We don't have such config values for other possibly destructive operations.

Quoted 3 lines

Quoted 3 lines

2. --filter is dry-run by default and prints out objects that would have been removed, and it has to be combined with another flag --destroy in order for it to actually remove objects from the odb.

I am not sure it's of much value either compared to naming it --filter-destroy. It's likely to just make things more difficult for users to understand.

Quoted 7 lines

I share the same concern as Robert and Stolee do. But I think this issue goes deeper than just naming.

Even if we called this git repack --delete-filter and only ran it with --i-know-what-im-doing flag, we would still be leaving repository corruption on the table, just making it marginally more difficult to achieve.

My opinion on this is that the promisor object mechanism assumes by design that some objects are outside a repo, and that this repo shouldn't care much about these objects possibly being corrupted.

I am not against a name and some docs that strongly state that users should be very careful when using such a command, but otherwise I think such a command is perfectly ok. We have other commands that by design could lead to some objects or data being lost.

Quoted 2 lines

But as it stands right now, I worry that this feature is too easily misused and could result in unintended repository corruption.

Are you worrying about the UI or about what it does?

I am ok with improving the UI, but I think what it does is reasonable.

Quoted 8 lines · Christian Couder

Quoted 4 lines

Quoted 4 lines

Also, to have more protection we can either

1. add a config value that needs to be set to true for repack to remove objects (repack.allowDestroyFilter).

I don't think it's of much value. We don't have such config values for other possibly destructive operations.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

2. --filter is dry-run by default and prints out objects that would have been removed, and it has to be combined with another flag --destroy in order for it to actually remove objects from the odb.

I am not sure it's of much value either compared to naming it --filter-destroy. It's likely to just make things more difficult for users to understand.

On this and the above, I agree with Christian.

Quoted 11 lines

Quoted 7 lines

I share the same concern as Robert and Stolee do. But I think this issue goes deeper than just naming.

Even if we called this git repack --delete-filter and only ran it with --i-know-what-im-doing flag, we would still be leaving repository corruption on the table, just making it marginally more difficult to achieve.

My opinion on this is that the promisor object mechanism assumes by design that some objects are outside a repo, and that this repo shouldn't care much about these objects possibly being corrupted.

For what it's worth, I am fine having a mode of repack which allows us to remove objects that we know are stored by a promisor remote. But this series doesn't do that, so users could easily run git repack -d --filter=... and find that they have irrecoverably corrupted their repository.

I think that there are some other reasonable directions, though. One which Robert and I discussed was making it possible to split a repository into two packs, one which holds objects that match some --filter criteria, and one which holds the objects that don't match that filter.

Another option would be to prune the repository according to objects that are already made available by a promisor remote.

An appealing quality about the above two directions is that the first doesn't actually remove any objects, just makes it easier to push a whole pack of unwanted objects off to a promsior remote. The second prunes the repository according to objects that are already made available by the promisor remote. (Yes, there is a TOCTOU race there, too, but it's the same prune-while-pushing race that Git already has today).

Quoted 4 lines

I am not against a name and some docs that strongly state that users should be very careful when using such a command, but otherwise I think such a command is perfectly ok. We have other commands that by design could lead to some objects or data being lost.

I can think of a handful of ways to remove objects which are unreachable from a repository, but I am not sure we have any ways to remove objects which are reachable.

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

I share the same concern as Robert and Stolee do. But I think this issue goes deeper than just naming.

Even if we called this git repack --delete-filter and only ran it with --i-know-what-im-doing flag, we would still be leaving repository corruption on the table, just making it marginally more difficult to achieve.

My opinion on this is that the promisor object mechanism assumes by design that some objects are outside a repo, and that this repo shouldn't care much about these objects possibly being corrupted.

Quoted 5 lines

For what it's worth, I am fine having a mode of repack which allows us to remove objects that we know are stored by a promisor remote. But this series doesn't do that, so users could easily run git repack -d --filter=... and find that they have irrecoverably corrupted their repository.

In some cases we just know the objects we are removing are stored by a promisor remote or are replicated on different physical machines or both, so you should be fine with this.

If you are not fine with this because sometimes a user might use it without knowing, then why are you ok with commands deleting refs not checking that there isn't a regular repack removing dangling objects?

In some cases we just know the objects we are removing are stored by a promisor remote or are replicated on different physical machines or both, so you should be fine with this.

Quoted 2 lines

Another option would be to prune the repository according to objects that are already made available by a promisor remote.

If the objects have just been properly transferred to the promisor remote, the check will just waste resources.

Quoted 5 lines

For what it's worth, I am fine having a mode of repack which allows us to remove objects that we know are stored by a promisor remote. But this series doesn't do that, so users could easily run git repack -d --filter=... and find that they have irrecoverably corrupted their repository.

In some cases we just know the objects we are removing are stored by a promisor remote or are replicated on different physical machines or both, so you should be fine with this.

If you are not fine with this because sometimes a user might use it without knowing, then why are you ok with commands deleting refs not checking that there isn't a regular repack removing dangling objects?

Quoted 9 lines · Christian Couder

Quoted 5 lines

For what it's worth, I am fine having a mode of repack which allows us to remove objects that we know are stored by a promisor remote. But this series doesn't do that, so users could easily run git repack -d --filter=... and find that they have irrecoverably corrupted their repository.

In some cases we just know the objects we are removing are stored by a promisor remote or are replicated on different physical machines or both, so you should be fine with this.

So, we need to decide if an object we have that is outside the narrowed filter definition was (and still is, but let's keep the assumption the whole lazy clone mechanism makes: promisor remotes will never shed objects that they once served) available at the promisor remote, but I suspect we have too little information to reliably do so. It is OK to assume that objects in existing packs taken from the promisor remotes and everything reachable from them (but missing from our object store) will be available to us from there. But if we see an object that is outside of _new_ filter spec (e.g. you fetched with "max 100MB", now you are refiltering with "max 50MB", narrowing the spec, and you need to decide for an object that weigh 70MB), can we tell if that can be retrieved from the promisor or is it unique to our repository until we push it out? I am not sure. For that matter, do we even have a way to compare if the new filter spec is a subset, a superset, or neither, of the original filter spec?

Quoted 4 lines

If you are not fine with this because sometimes a user might use it without knowing, then why are you ok with commands deleting refs not checking that there isn't a regular repack removing dangling objects?

Sorry, I do not follow this argument. Your user may do "branch -D" because the branch deleted is no longer needed, which may mean that objects only reachable from the deleted branch are no longer needed. I do not see what repack has anything to do with that. As long as the filter spec does not change (in other words, before this series is applied), the repack that discards objects that are known to be reachable from objects in packs retrieved from promisor remote, the objects that are no longer reachable may be removed and that will not lose objects that we do not know to be retrievable from there (which is different from objects that we know are unretrievable). But with filter spec changing after the fact, I am not sure if that is safe. IOW, "commands deleting refs" may have been OK without this series, but this series may be what makes it not OK, no?

Puzzled.

let me try to summarize (perhaps over simplify) the main concern folks have with this feature, so please correct me if I'm wrong!

As a user, if I apply a filter that ends up deleting objects that it turns out do not exist anywhere else, then I have irrecoverably corrupted my repository.

Before git allows me to delete objects from my repository, it should be pretty certain that I have path to recover those objects if I need to.

Is that correct? It seems to me that, put another way, we don't want to give users too much rope to hang themselves.

Quoted 7 lines · John Cai

let me try to summarize (perhaps over simplify) the main concern folks have with this feature, so please correct me if I'm wrong!

As a user, if I apply a filter that ends up deleting objects that it turns out do not exist anywhere else, then I have irrecoverably corrupted my repository.

Quoted 6 lines

Before git allows me to delete objects from my repository, it should be pretty certain that I have path to recover those objects if I need to.

Is that correct? It seems to me that, put another way, we don't want to give users too much rope to hang themselves.

I wrote about my concerns in some more detail in [1], but the thing I was most unclear on was how your demo script[2] was supposed to work.

Namely, I wasn't sure if you had intended to use two separate filters to "re-filter" a repository, one to filter objects to be uploaded to a content store, and another to filter objects to be expunged from the repository. I have major concerns with that approach, namely that if each of the filters is not exactly the inverse of the other, then we will either upload too few objects, or delete too many.

My other concern was around what guarantees we currently provide for a promisor remote. My understanding is that we expect an object which was received from the promisor remote to always be fetch-able later on. If that's the case, then I don't mind the idea of refiltering a repository, provided that you only need to specify a filter once.

So the suggestion about splitting a repository into two packs was a potential way to mediate the "two filter" problem, since the two packs you get exactly correspond to the set of objects that match the filter, and the set of objects that _don't_ match the filter.

In either case, I tried to use the patches in [1] and was able to corrupt my local repository (even when fetching from a remote that held onto the objects I had pruned locally).

Thanks,
Taylor

[1]: https://lore.kernel.org/git/YhUeUCIetu%[email protected]/
[2]: https://gitlab.com/chriscool/partial-clone-demo/-/blob/master/http-promisor/server_demo.txt#L47-52

Thanks for bringing this up again. I meant to write back regarding what you raised in the other part of this thread. I think this is a valid concern. To attain the goal of offloading certain blobs onto another server(B) and saving space on a git server(A), then there will essentially be two steps. One to upload objects to (B), and one to remove objects from (A). As you said, these two need to be the inverse of each other or else you might end up with missing objects.

Quoted 5 lines

My other concern was around what guarantees we currently provide for a promisor remote. My understanding is that we expect an object which was received from the promisor remote to always be fetch-able later on. If that's the case, then I don't mind the idea of refiltering a repository, provided that you only need to specify a filter once.

Could you clarify what you mean by re-filtering a repository? By that I assumed it meant specifying a filter eg: 100mb, and then narrowing it by specifying a 50mb filter.

Quoted 6 lines · John Cai

Thanks for bringing this up again. I meant to write back regarding what you raised in the other part of this thread. I think this is a valid concern. To attain the goal of offloading certain blobs onto another server(B) and saving space on a git server(A), then there will essentially be two steps. One to upload objects to (B), and one to remove objects from (A). As you said, these two need to be the inverse of each other or else you might end up with missing objects.

Do you mean that you want to offload objects both from a local clone of some repository, _and_ the original remote it was cloned from?

I don't understand what the role of "another server" is here. If this proposal was about making it easy to remove objects from a local copy of a repository based on a filter provided that there was a Git server elsewhere that could act as a promisor remote, than that makes sense to me.

But I think I'm not quite understanding the rest of what you're suggesting.

Quoted 10 lines

Quoted 6 lines

My other concern was around what guarantees we currently provide for a promisor remote. My understanding is that we expect an object which was received from the promisor remote to always be fetch-able later on. If that's the case, then I don't mind the idea of refiltering a repository, provided that you only need to specify a filter once.

Could you clarify what you mean by re-filtering a repository? By that I assumed it meant specifying a filter eg: 100mb, and then narrowing it by specifying a 50mb filter.

I meant: applying a filter to a local clone (either where there wasn't a filter before, or a filter which matched more objects) and then removing objects that don't match the filter.

But your response makes me think of another potential issue. What happens if I do the following:

    $ git repack -ad --filter=blob:limit=100k
    $ git repack -ad --filter=blob:limit=200k

What should the second invocation do? I would expect that it needs to do a fetch from the promisor remote to recover any blobs between (100, 200] KB in size, since they would be gone after the first repack.

This is a problem not just with two consecutive git repack --filters, I think, since you could cook up the same situation with:

    $ git clone --filter=blob:limit=100k [email protected]:git
    $ git -C git repack -ad --filter=blob:limit=200k

I don't think the existing patches handle this situation, so I'm curious whether it's something you have considered or not before.

(Unrelated to the above, but please feel free to trim any quoted parts of emails when responding if they get overly long.)

Thanks,
Taylor

Hi Taylor,

Quoted 10 lines · Taylor Blau

Quoted 6 lines · John Cai

Thanks for bringing this up again. I meant to write back regarding what you raised in the other part of this thread. I think this is a valid concern. To attain the goal of offloading certain blobs onto another server(B) and saving space on a git server(A), then there will essentially be two steps. One to upload objects to (B), and one to remove objects from (A). As you said, these two need to be the inverse of each other or else you might end up with missing objects.

Do you mean that you want to offload objects both from a local clone of some repository, _and_ the original remote it was cloned from?

yes, exactly. The "another server" would be something like an http server, OR another remote which hosts a subset of the objects (let's say the large blobs).

Quoted 7 lines

I don't understand what the role of "another server" is here. If this proposal was about making it easy to remove objects from a local copy of a repository based on a filter provided that there was a Git server elsewhere that could act as a promisor remote, than that makes sense to me.

Quoted 2 lines

But I think I'm not quite understanding the rest of what you're suggesting.

Sorry for the lack of clarity here. The goal is to make it easy for a remote to offload a subset of its objects to __another__ remote (either a Git server or an http server through a remote helper).

Quoted 8 lines · Christian Couder

Quoted 4 lines

Quoted 4 lines

Also, to have more protection we can either

1. add a config value that needs to be set to true for repack to remove objects (repack.allowDestroyFilter).

I don't think it's of much value. We don't have such config values for other possibly destructive operations.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

2. --filter is dry-run by default and prints out objects that would have been removed, and it has to be combined with another flag --destroy in order for it to actually remove objects from the odb.

I am not sure it's of much value either compared to naming it --filter-destroy. It's likely to just make things more difficult for users to understand.

On this and the above, I agree with Christian.

Quoted 11 lines

Quoted 7 lines

I share the same concern as Robert and Stolee do. But I think this issue goes deeper than just naming.

Even if we called this git repack --delete-filter and only ran it with --i-know-what-im-doing flag, we would still be leaving repository corruption on the table, just making it marginally more difficult to achieve.

My opinion on this is that the promisor object mechanism assumes by design that some objects are outside a repo, and that this repo shouldn't care much about these objects possibly being corrupted.

For what it's worth, I am fine having a mode of repack which allows us to remove objects that we know are stored by a promisor remote. But this series doesn't do that, so users could easily run git repack -d --filter=... and find that they have irrecoverably corrupted their repository.

I think that there are some other reasonable directions, though. One which Robert and I discussed was making it possible to split a repository into two packs, one which holds objects that match some --filter criteria, and one which holds the objects that don't match that filter.

Another option would be to prune the repository according to objects that are already made available by a promisor remote.

An appealing quality about the above two directions is that the first doesn't actually remove any objects, just makes it easier to push a whole pack of unwanted objects off to a promsior remote. The second prunes the repository according to objects that are already made available by the promisor remote. (Yes, there is a TOCTOU race there, too, but it's the same prune-while-pushing race that Git already has today).

Quoted 4 lines

I am not against a name and some docs that strongly state that users should be very careful when using such a command, but otherwise I think such a command is perfectly ok. We have other commands that by design could lead to some objects or data being lost.

I can think of a handful of ways to remove objects which are unreachable from a repository, but I am not sure we have any ways to remove objects which are reachable.

Quoted 6 lines

Quoted 2 lines

But as it stands right now, I worry that this feature is too easily misused and could result in unintended repository corruption.

Are you worrying about the UI or about what it does?

I am ok with improving the UI, but I think what it does is reasonable.

I am more worried about the proposal's functionality than its UI, hopefully my concerns there are summarized above.

Thanks,
Taylor

Quoted 2 lines

Another option would be to prune the repository according to objects that are already made available by a promisor remote.

Thanks for the discussion around the two packfile idea. Definitely interesting. However, I'm leaning towards the second option here where we ensure that objects that are about to be deleted can be retrieved via a promisor remote. That way we have an easy path to recovery.