Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 15, 2021, 01:04 UTC
Message-ID: <20210415010454.4077355-1-jonathantanmy@google.com>
In-Reply-To: <20210414191403.4387-3-rafaeloliveira.cs@gmail.com>

> When `-A` and `-d` are used together, besides packing all objects (-A)
> and removing redundant packs (-d), it also unpack all unreachable
> objects and deletes them by calling `git pruned-packed`.

I still think of these objects as not unreachable, even though I know that pack-objects calls them that (the argument is called --unpack-unreachable). So I would say "it also loosens all objects that were previously packed but did not go into the new pack", but perhaps this is OK too.

Show 5 quoted lines

> For a partial
> clone, that contains unreferenced objects, this results in unpacking
> all "promisor" objects and deleting them right after, which
> unnecessarily increases the `repack` execution time and disk usage
> during the unpacking of the objects.

I think that the commit message also needs to explain that we're deleting the promisor objects immediately because they happen to be in a promisor pack. So perhaps this whole part could be written as follows:

  When "git repack -Ad" is run in a partial clone, "pack-objects" is
  invoked twice: once to repack all promisor objects, and once to repack
  all non-promisor objects. The latter "pack-objects" invocation is with
  --exclude-promisor-objects and --unpack-unreachable, which loosens all
  unused objects. Unfortunately, this includes promisor objects.

  Because the "-d" argument to "git repack" subsequently deletes all
  loose objects also in packs, these just-loosened promisor objects will
  be immediately deleted. But this extra disk churn is unnecessary in
  the first place.

Show 11 quoted lines

> For instance, a partially cloned repository that filters all the blob
> objects (e.g. "--filter=blob:none"), `repack` ends up unpacking all
> blobs into the filesystem that, depending on the repo size, makes
> nearly impossible to repack the operation before running out of disk.
> 
> For a partial clone, `git repack` calls `git pack-objects` twice: (1)
> for handle the "promisor" objects and (2) for performing the repack
> with --exclude-promisor-objects option, that results in unpacking and
> deleting of the objects. Given that we actually should keep the
> promisor objects, let's teach `repack` to tell `pack-objects` to
> --keep the old "promisor" pack file.

It's not this call (2) that results in any deleting of the objects, but the later call to prune_packed_objects(). Also, promisor objects are kept regardless of what we pass to "pack-objects" here (the keeping is done separately). Maybe write (continuation from my suggestion above):

  In order to avoid this extra disk churn, pass the names of the
  promisor packfiles as "--keep-pack" arguments to this
  second invocation of "pack-objects". This informs "pack-objects" that
  the promisor objects are already in a safe packfile and, therefore, do
  not need to be loosened.

Show 10 quoted lines

> @@ -533,7 +533,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	packdir = mkpathdup("%s/pack", get_object_directory());
> -	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
> +	packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
> +	packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
>  
>  	sigchain_push_common(remove_pack_on_signal);
>

Normally I would be concerned that packtmp_name is not freed, but in this case, it's a static variable (same as packtmp).

Show 8 quoted lines

> @@ -576,6 +577,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		repack_promisor_objects(&po_args, &names);
>  
>  		if (existing_packs.nr && delete_redundant) {
> +			for_each_string_list_item(item, &names) {
> +				strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
> +					     packtmp_name, item->string);
> +			}

Git style is to not have braces for single-statement loops.

Show 8 quoted lines

> +test_expect_success 'repack does not loose all objects' '
> +	rm -rf client &&
> +	git clone --bare --filter=blob:none "file://$(pwd)/srv.bare" client &&
> +	test_when_finished "rm -rf client" &&
> +	git -C client repack -A -l -d --no-prune-packed &&
> +	git -C client count-objects -v >object-count &&
> +	grep "^prune-packable: 0" object-count
> +'

s/loose all objects/loosen promisor objects/

Also, add a comment describing why we have "--no-prune-packed" there (probably something about not pruning any loose objects that are already in packs, so that we can verify that no redundant loose objects are being created in the first place).