Re: [PATCH 2/2] repack: add --filter=<filter-spec> option

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 27, 2022, 15:03 UTC
Message-ID: <a62a007f-7c61-68eb-c0e6-548dc9b6f671@gmail.com>
In-Reply-To: <a3166381572481f2ed159740eb8a1d88d4f9dc0f.1643248180.git.gitgitgadget@gmail.com>

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