Subject: Re: [PATCH 8/9] repack: implement `--filter-to` for storing filtered out objects

From: Junio C Hamano <[email-masked]>
Date: Thu, 15 Jun 2023 19:21:56 -0700

Christian Couder <[email-masked]> writes:

> A previous commit has implemented `git repack --filter=<filter-spec>` to
> allow users to filter out some objects from the main pack and move them
> into a new different pack.
>
> It would be nice if this new different pack could be created in a
> different directory than the regular pack. This would make it possible
> to move large blobs into a pack on a different kind of storage, for
> example cheaper storage. Even in a different directory this pack can be
> accessible if, for example, the Git alternates mechanism is used to
> point to it.

Makes sense, I guess, for "in other usecases" scenario.  I am not
sure how this would be useful for the originally stated goal of
unbloating a bloated repository with promisor remote(s), though. 

> If users want to remove a pack that contains filtered out objects after
> checking that they are all already on a promisor remote, creating the
> pack in a different directory makes it easier to do so.

Care to elaborate?  I do not see how a separate directory would make
it easier.  After separating the potential cruft into a packfile,
you'd walk its .idx and see if there are any objects that are not
available (yet) at the promisor remotes to check if it is safe to
remove.  That can be done regardless of the location of the packfile
that is suspected to be now removable.

> Signed-off-by: Christian Couder <[email-masked]>
> ---
>  Documentation/git-repack.txt |  6 ++++++
>  builtin/repack.c             | 17 ++++++++++++-----
>  t/t7700-repack.sh            | 27 +++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
> index aa29c7e648..070dd22610 100644
> --- a/Documentation/git-repack.txt
> +++ b/Documentation/git-repack.txt
> @@ -148,6 +148,12 @@ depth is 4095.
>  	resulting packfile and put them into a separate packfile. See
>  	linkgit:git-rev-list[1] for valid `<filter-spec>` forms.
>  
> +--filter-to=<dir>::
> +	Write the pack containing filtered out objects to the
> +	directory `<dir>`. This can be used for putting the pack on a
> +	separate object directory that is accessed through the Git
> +	alternates mechanism. Only useful with `--filter`.
> +
>  -b::
>  --write-bitmap-index::
>  	Write a reachability bitmap index as part of the repack. This
> diff --git a/builtin/repack.c b/builtin/repack.c
> index b13d7196de..8c71e8fd51 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -838,7 +838,8 @@ static void prepare_pack_filtered_cmd(struct child_process *cmd,
>  }
>  
>  static void finish_pack_filtered_cmd(struct child_process *cmd,
> -				     struct string_list *names)
> +				     struct string_list *names,
> +				     const char *destination)
>  {
>  	if (cmd->in == -1) {
>  		/* No packed objects; cmd was never started */
> @@ -848,7 +849,7 @@ static void finish_pack_filtered_cmd(struct child_process *cmd,
>  
>  	close(cmd->in);
>  
> -	if (finish_pack_objects_cmd(cmd, names, NULL, NULL))
> +	if (finish_pack_objects_cmd(cmd, names, destination, NULL))
>  		die(_("could not finish pack-objects to pack filtered objects"));
>  }
>  
> @@ -877,6 +878,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	const char *cruft_expiration = NULL;
>  	const char *expire_to = NULL;
>  	struct child_process pack_filtered_cmd = CHILD_PROCESS_INIT;
> +	const char *filter_to = NULL;
>  
>  	struct option builtin_repack_options[] = {
>  		OPT_BIT('a', NULL, &pack_everything,
> @@ -930,6 +932,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  			   N_("write a multi-pack index of the resulting packs")),
>  		OPT_STRING(0, "expire-to", &expire_to, N_("dir"),
>  			   N_("pack prefix to store a pack containing pruned objects")),
> +		OPT_STRING(0, "filter-to", &filter_to, N_("dir"),
> +			   N_("pack prefix to store a pack containing filtered out objects")),
>  		OPT_END()
>  	};
>  
> @@ -1073,8 +1077,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		strvec_push(&cmd.args, "--incremental");
>  	}
>  
> -	if (po_args.filter)
> -		prepare_pack_filtered_cmd(&pack_filtered_cmd, &po_args, packtmp);
> +	if (po_args.filter) {
> +		if (!filter_to)
> +			filter_to = packtmp;
> +		prepare_pack_filtered_cmd(&pack_filtered_cmd, &po_args, filter_to);
> +	}
>  
>  	if (geometry)
>  		cmd.in = -1;
> @@ -1169,7 +1176,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (po_args.filter)
> -		finish_pack_filtered_cmd(&pack_filtered_cmd, &names);
> +		finish_pack_filtered_cmd(&pack_filtered_cmd, &names, filter_to);
>  
>  	string_list_sort(&names);
>  
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 9e7654090f..898f8a01b4 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -286,6 +286,33 @@ test_expect_success 'repacking with a filter works' '
>  	test "$blob_pack2" = "$blob_pack"
>  '
>  
> +test_expect_success '--filter-to stores filtered out objects' '
> +	git -C bare.git repack -a -d &&
> +	test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
> +
> +	git init --bare filtered.git &&
> +	git -C bare.git -c repack.writebitmaps=false repack -a -d \
> +		--filter=blob:none \
> +		--filter-to=../filtered.git/objects/pack/pack &&
> +	test_stdout_line_count = 1 ls bare.git/objects/pack/pack-*.pack &&
> +	test_stdout_line_count = 1 ls filtered.git/objects/pack/pack-*.pack &&
> +
> +	commit_pack=$(test-tool -C bare.git find-pack HEAD) &&
> +	test -n "$commit_pack" &&
> +	blob_pack=$(test-tool -C bare.git find-pack HEAD:file1) &&
> +	test -z "$blob_pack" &&
> +	blob_hash=$(git -C bare.git rev-parse HEAD:file1) &&
> +	test -n "$blob_hash" &&
> +	blob_pack=$(test-tool -C filtered.git find-pack $blob_hash) &&
> +	test -n "$blob_pack" &&
> +
> +	echo $(pwd)/filtered.git/objects >bare.git/objects/info/alternates &&
> +	blob_pack=$(test-tool -C bare.git find-pack HEAD:file1) &&
> +	test -n "$blob_pack" &&
> +	blob_content=$(git -C bare.git show $blob_hash) &&
> +	test "$blob_content" = "content1"
> +'
> +
>  objdir=.git/objects
>  midx=$objdir/pack/multi-pack-index