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
