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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

>> 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.
>

Thanks for suggesting this message, obviously it's much better than
mine specially because removes the confusion that I made when writing
the message about the `pack-objects` and `prune-packed`.

I'll update patch's message on the next revision.

>> 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.
>

You're right, the second call only loosens the object and the deleting
comes after it, which makes my message misleading. 

The suggested paragraph explain better the situation, thanks for
suggesting it and will replace mine in the next revision.

>> @@ -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).
>

Indeed.

>> @@ -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.
>

I preferred to keep the braces simply because the
for_each_string_list_item() is macro instead of a normal for-loop
statement. 

>> +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).

Thanks for the reviewing and helping clarify the patch intentions. I'll
address this comments on the next revision.

-- 
Thanks
Rafael