On 3/23/2022 7:10 PM, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > >> On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote: >>>> So, perhaps #3 ;-)? >>> >>> I'll default to #3 (do nothing), but if this shows up again >>> I'll plan on adding a comment to the helper to be clear on >>> how "inexact" the helper really is. >> >> I wonder if we could sidestep the whole issue with >> test_subcommand_inexact by testing this behavior by looking at the >> contents of the packs themselves. >> >> If we have a kept pack, and then add some new objects, and run "git >> repack --write-midx -adb", the new pack should not contain any of the >> objects found in the old (kept) pack. And that's the case after this >> patch, but was broken before it. > > Sounds quite sensible. > > Instead of saying "we are happy as long as we internally run this > command, as that _should_ give us the desired outcome", we check the > resulting packs ourselves, and we do not really care how the > "repack" command gave us that desired outcome. Sounds good. It's all about a balance: using test_subcommand[_inexact] gives us a way to check "Did we trigger this other command that we trust works correctly from other tests?" without the more complicated work of doing a full post-condition check. It's a bit more of a unit- level check than most Git tests. The full post-condition check requires more test code, but that's not really a problem. The problem comes in if that test is now too rigid to future changes in that subcommand. What if the post-conditions change in a subtle way because of the subcommand does something differently, but in a way that is not of importance to the top command? In this specific case, the test name says that it "packs non-kept objects", so we can do more here to validate that post-condition that we care about. As I'm looking at Taylor's test case example, the one thing I notice is that there is only one pack-file before the repack. It would be good to have a non-kept packfile get repacked in the process, not just the loose objects added by the test_commit. I'll take a look at what can be done here. Thanks, -Stolee
