On Tue, Jun 14 2022, Derrick Stolee wrote: > On 6/13/2022 8:33 PM, Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Jun 13 2022, Derrick Stolee wrote: >> >>> On 6/8/2022 4:08 PM, Derrick Stolee via GitGitGadget wrote: > >>> While TEST_PASSES_SANITIZE_LEAK would be helpful to demonstrate the >>> leakage and prevent it in the future, t2407 uses helpers such as 'git >>> clone' that cause the test to fail under that mode. >> >> If you apply this: >> >> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh >> index 0760595337b..d41171acb83 100755 >> --- a/t/t2407-worktree-heads.sh >> +++ b/t/t2407-worktree-heads.sh >> @@ -10,16 +10,8 @@ test_expect_success 'setup' ' >> test_commit $i && >> git branch wt-$i && >> git worktree add wt-$i wt-$i || return 1 >> - done && >> - >> - # Create a server that updates each branch by one commit >> - git clone . server && >> - git remote add server ./server && >> - for i in 1 2 3 4 >> - do >> - git -C server checkout wt-$i && >> - test_commit -C server A-$i || return 1 >> done >> + >> ' >> >> test_expect_success 'refuse to overwrite: checked out in worktree' ' >> >> And compile with SANITIZE=leak then this will pass as: >> >> ./t2407-worktree-heads.sh --run=1,6 > > Of course this works for the tests that don't need the 'server' repo, > but it fails in the tests that _do_ need it. > > I'm able to make this work by creating the 'server' with init and > creating the wt-$i branches from scratch (they don't need to be > fast-forward updates). > > The linux-leaks tests still fail due to 'git fetch' and 'git bisect' > calls, but these can be avoided by carefully splitting the tests and > using the !SANITIZE_LEAK prereq. I mean copy/paste the existing file to a new one with this on top, then remove all tests that aren't your new one. That worked for me, doesn't it work for you? >> Normally I'd just say "let's leave it for later", but in this case the >> entire point of the commit and the relatively lengthy test is to deal >> with a memory leak, so just copy/pasting the few lines of setup you >> actually need to a new test & testing with SANITIZE=leak seems worth the >> effort in this case. > > Well, the point isn't to use automation to check for leaks, but instead > to fix leaks and add tests for the case where we previously had leaks. > The tests demonstrate that we aren't accidentally introducing a use-after- > free or double-free. I mean that the patch subject is "fix[...]leaks", and this gives it a CI'd regression test, which seems worth it. Of course it's also nice to check for use-after-fre etc., but we have other coverage for that. E.g. glibc in CI spots double-free issues with how we set it up, and we tend to segfault soon enough (or someone runs SANITIZE=address) on use-after-free.
