Re: [PATCH 1/2] repack: respect kept objects with '--write-midx -b'

[Derrick Stolee <stolee@gmail.com>]

On 12/17/2021 12:24 PM, Jeff King wrote:
> On Fri, Dec 17, 2021 at 04:28:45PM +0000, Derrick Stolee via GitGitGadget wrote:

...

>> +test_expect_success '--write-midx -b packs non-kept objects' '
>> +	git init midx-kept &&
>> +	test_when_finished "rm -fr midx-kept" &&
>> +	(
>> +		cd midx-kept &&
>> +		test_commit_bulk 100 &&
>> +		GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
>> +			git repack --write-midx -a -b &&
>> +		cat trace.txt | \
>> +			grep \"event\":\"start\" | \
>> +			grep pack-objects | \
>> +			grep \"--honor-pack-keep\"
>> +	)
>> +'
> 
> This looks correct, though:
> 
>   - do we really need this separate repo directory? The test before it
>     uses one, but only because it needs a very specific set of commits.
>     There is a long-running "midx" directory we could use, though I
>     think just the regular test repo would be fine, too.
> 
>   - likewise, do we need 100 commits? They are not too expensive these
>     days with test_commit_bulk, but I think we don't even care about the
>     repo contents at all.
> 
>   - there is no actual .keep file in your test. That's OK, as we are
>     just checking the args passed to pack-objects.
> 
>   - useless use of cat. :) Also, you could probably do it all with one
>     grep. This is bikeshedding, of course, but it's nice to keep process
>     counts low in the test suite. Also, your middle grep is looser than
>     the others (it doesn't check for surrounding quotes).
> 
> So something like this would work:
> 
>   test_expect_success '--write-midx -b packs non-kept objects' '
>           GIT_TRACE2_EVENT="$(pwd)/midx-keep-bitmap.trace" \
>                   git -C midx repack --write-midx -a -b &&
>           grep "\"event\":\"start\".*\"pack-objects\".*\"--honor-pack-keep\"" \
>                   midx-keep-bitmap.trace
>   '
> 
> One could perhaps argue that the combined grep is less readable. If
> that's a concern, I'd suggest wrapping it in a function like:
> 
>   # usage: check_trace2_arg <trace_file> <cmd> <arg>
>   check_trace2_arg () {
> 	grep "\"event\":\"start\".*\"$2\".*\"$3\"" "$1"
>   }
> 
> All just suggestions, of course. I'd be happy enough to see the patch go
> in as-is.

Thanks for the suggestions. I decided to adapt the 'test_subcommand'
helper into a 'test_subcommand_inexact' helper. The existing helper
requires the full argument list in exact order, but the new one only
cares about the given arguments (in that relative order).

Thanks,
-Stolee