Re: [PATCH 1/2] repack: teach --no-prune-packed to skip `git prune-packed`

From: Rafael Silva <rafaeloliveira.cs@gmail.com>
Date: Apr 18, 2021, 14:15 UTC
Message-ID: <gohp6kr1j8nmdv.fsf@cpm12071.fritz.box>
In-Reply-To: <20210414235027.4064035-1-jonathantanmy@google.com>

Thanks for reviewing this patch. really appreciate it.

However, in the v2, I decided to drop this in favor of just adding a trace2 event that emits all the loosened objects. This is to avoid adding user-visible option just for the sake of testing it.

Jonathan Tan <jonathantanmy@google.com> writes:

Show 11 quoted lines

>> The `git repack -d` command will remove any packfile that becomes
>> redundant after repacking, and then call  `git pruned-packed` for
>> pruning any unpacked objects.
>
> s/pruned-packed/prune-packed/ (note that there is no "d" after "prune")
> throughout this commit message.
>
> Also, if there are any objects pruned, they are packed objects, not
> unpacked objects. Maybe better to say "...for pruning any objects
> already in packs".
>

Thanks for catching the typo and the clarification.

Show 18 quoted lines

>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>> index 25b235c063..728a16ad97 100755
>> --- a/t/t7700-repack.sh
>> +++ b/t/t7700-repack.sh
>> @@ -127,12 +127,7 @@ test_expect_success 'packed unreachable obs in alternate ODB are not loosened' '
>>  	git reset --hard HEAD^ &&
>>  	test_tick &&
>>  	git reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&
>> -	# The pack-objects call on the next line is equivalent to
>> -	# git repack -A -d without the call to prune-packed
>> -	git pack-objects --honor-pack-keep --non-empty --all --reflog \
>> -	    --unpack-unreachable </dev/null pack &&
>> -	rm -f .git/objects/pack/* &&
>> -	mv pack-* .git/objects/pack/ &&
>> +	git repack -A -d --no-prune-packed &&
>
> This is great!
>

Unfortunately, with the drop of this patch in v2, we no longer refactor these tests. However, this still might be possible with the trace2 event information that is now-added in the v2. Of course, if this doesn't impact or weakens the test in anyway.

Show 11 quoted lines

>> +test_expect_success '-A -d and --no-prune-packed do not remove loose objects' '
>> +	test_create_repo repo &&
>> +	test_when_finished "rm -rf repo" &&
>> +	test_commit -C repo commit &&
>> +	git -C repo repack -A -d --no-prune-packed &&
>> +	git -C repo count-objects -v >out &&
>> +	grep "^prune-packable: 3" out
>> +'
>
> As for the test description, I would prefer "...do not remove loose
> objects already in packs".

Yes, this is an improvement of the test description.

-- 
Thanks
Rafael