| Message ID | 23d41343-54fd-46c6-9d78-369e8009fa0b@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d0b38a27c6f824679e017dce5da6c58269ceac63 |
| Headers | show |
| Series | t0613: mark as leak-free | expand |
On Sun, Jun 30, 2024 at 08:46:38AM +0200, Rubén Justo wrote: > We can mark t0613 as leak-free: > [...] > I'm not sure why this simple change has fallen through the cracks. > Therefore, it's possible that I'm missing something. > > I'd appreciate if someone could double-check. I'd noticed it, too, while doing recent leak fixes. But since Patrick has been working on leaks and is the go-to person for reftables, I assumed he had already seen it and there was something clever going on. ;) I also get a passing result from t0612 (and I do have JGit available, so it actually runs the tests). I also get funny results from t4255, but I think we can ignore them. It's known breakages vanishing, which I guess is just some sub-program returning failure due to a leak and changing the test results. So anyway, this patch looks good to me, but probably we could squash t0612 into it, as well. -Peff
On Sun, Jun 30, 2024 at 11:57:59PM -0400, Jeff King wrote: > On Sun, Jun 30, 2024 at 08:46:38AM +0200, Rubén Justo wrote: > > > We can mark t0613 as leak-free: > > [...] > > I'm not sure why this simple change has fallen through the cracks. > > Therefore, it's possible that I'm missing something. > > > > I'd appreciate if someone could double-check. > > I'd noticed it, too, while doing recent leak fixes. But since Patrick > has been working on leaks and is the go-to person for reftables, I > assumed he had already seen it and there was something clever going on. ;) > > I also get a passing result from t0612 (and I do have JGit available, so > it actually runs the tests). I have no idea how JGit works, and I didn't have it installed either. But after a quick test, I can confirm that t0612 can also be marked as leak-free. I'll respond to this message shortly with a patch to fix that. > > I also get funny results from t4255, but I think we can ignore them. > It's known breakages vanishing, which I guess is just some sub-program > returning failure due to a leak and changing the test results. > > So anyway, this patch looks good to me, but probably we could squash > t0612 into it, as well. > > -Peff Thank you!
On Sun, Jun 30, 2024 at 11:57:59PM -0400, Jeff King wrote: > On Sun, Jun 30, 2024 at 08:46:38AM +0200, Rubén Justo wrote: > > > We can mark t0613 as leak-free: > > [...] > > I'm not sure why this simple change has fallen through the cracks. > > Therefore, it's possible that I'm missing something. > > > > I'd appreciate if someone could double-check. > > I'd noticed it, too, while doing recent leak fixes. But since Patrick > has been working on leaks and is the go-to person for reftables, I > assumed he had already seen it and there was something clever going on. ;) Nah, you assumed too much :) I just forgot to mark this as leak-free and the topic crossed with my memory-leak-fix topics, so I didn't yet find the time to fix it. It does highlight an issue though: I think memory leak checks should be opt-out rather than opt-in by now. Most of our tests run just fine with the memory leak checker enabled, and that's also where we want to be headed. So making tests opt-out would likely raise more eyebrows when new tests are being added that explicitly opt out. The only reason I didn't send a patch like this yet is that it would of course create quite a bit of churn in our tests. I'm not sure whether that churn is really worth it, or whether we should instead just continue fixing tests until we can get rid of this marking altogether because all of our tests pass. Patrick
On Mon, Jul 22, 2024 at 11:02:24AM +0200, Patrick Steinhardt wrote: > > I'd noticed it, too, while doing recent leak fixes. But since Patrick > > has been working on leaks and is the go-to person for reftables, I > > assumed he had already seen it and there was something clever going on. ;) > > Nah, you assumed too much :) I just forgot to mark this as leak-free and > the topic crossed with my memory-leak-fix topics, so I didn't yet find > the time to fix it. Ah, OK. :) Then I think we did the right thing in your absence. > It does highlight an issue though: I think memory leak checks should be > opt-out rather than opt-in by now. Most of our tests run just fine with > the memory leak checker enabled, and that's also where we want to be > headed. So making tests opt-out would likely raise more eyebrows when > new tests are being added that explicitly opt out. > > The only reason I didn't send a patch like this yet is that it would of > course create quite a bit of churn in our tests. I'm not sure whether > that churn is really worth it, or whether we should instead just > continue fixing tests until we can get rid of this marking altogether > because all of our tests pass. I could see arguments in both directions. I'd worry that by switching the default to "assume leak free", it may end up with misalignment between who introduces the bug and who deals with the fallout. Right now, if I introduce a test that is leak free but don't mark it, somebody working on leaks later runs in check mode and says "yay, it passes. Let's mark it". It becomes their task to do, but it's an easy-ish task. If we go the other way, then a new test that _does_ leak means that either: 1. The original author notices the CI leaks job failing. a. They introduced the leak, and it was caught early. Yay! b. The leak is in some random part of Git that their test happened to trigger. Now they spend effort proving it was not their fault before they annotate the test with "does not pass leak". 2. The original author does not notice. Somebody notices later when doing leak-checking (or I guess just running their own CI, if we are hitting these by default). Now they are stuck with doing (1a) or (1b) themselves, even though they do not care about the original topic. So I dunno. If we think people are paying attention to CI on their topics, and we think that we are close enough to leak-free that (1b) won't come up a lot, it might make sense. I'm not quite sure we're there yet on the latter, but it's mostly gut feeling (and I know things have gotten a bit better recently, too). I guess the only way to know is to try it, but as you noted, it is a bit of churn to switch between the two states. -Peff
diff --git a/t/t0613-reftable-write-options.sh b/t/t0613-reftable-write-options.sh index e2708e11d5..b1c6c97524 100755 --- a/t/t0613-reftable-write-options.sh +++ b/t/t0613-reftable-write-options.sh @@ -16,6 +16,7 @@ export GIT_TEST_DEFAULT_HASH GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'default write options' '
We can mark t0613 as leak-free: $ make test SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true T=t0613-reftable-write-options.sh [...] *** t0613-reftable-write-options.sh *** in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true ok 1 - default write options ok 2 - disabled reflog writes no log blocks ok 3 - many refs results in multiple blocks ok 4 - tiny block size leads to error ok 5 - small block size leads to multiple ref blocks ok 6 - small block size fails with large reflog message ok 7 - block size exceeding maximum supported size ok 8 - restart interval at every single record ok 9 - restart interval exceeding maximum supported interval ok 10 - object index gets written by default with ref index ok 11 - object index can be disabled # passed all 11 test(s) 1..11 # faking up non-zero exit with --invert-exit-code make[2]: *** [Makefile:75: t0613-reftable-write-options.sh] Error 1 Do it. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- I'm not sure why this simple change has fallen through the cracks. Therefore, it's possible that I'm missing something. I'd appreciate if someone could double-check. Thanks. t/t0613-reftable-write-options.sh | 1 + 1 file changed, 1 insertion(+)