Re: [PATCH 2/2] repack: add --filter=<filter-spec> option
On 1/26/2022 8:49 PM, John Cai via GitGitGadget wrote:
Show 7 quoted lines
> From: John Cai <johncai86@gmail.com>
>
> 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.
> 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.
Show 13 quoted 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:
>
> - 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'.
Show 9 quoted lines
> 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.
> + }
> }
> fclose(out);
> ret = finish_command(&cmd);
Show 18 quoted lines
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index e489869dd94..78cc1858cb6 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -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 &&
> + 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".
> + 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.
> + 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.
> + 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
John Cai via GitGitGadget
