Re: Crashes in t/t4058-diff-duplicates.sh

From: Alex Riesen <alexander.riesen@cetitec.com>
Date: May 6, 2022, 10:18 UTC
Message-ID: <YnT19KB2XkBrJOLQ@pflmari>
In-Reply-To: <YnSWgDdxgm+XWiLt@nand.local>

Taylor Blau, Fri, May 06, 2022 05:31:12 +0200:

Show 7 quoted lines

> On Thu, May 05, 2022 at 10:53:45AM +0200, Alex Riesen wrote:
> > Hi,
> >
> > the test t4058-diff-duplicates reliably dumps core here:
> 
> It was a little tricky to find out what part of t4058 you were referring
> to, but...

Very sorry! I forgot to include the output of the test itself!

Show 7 quoted lines

> > Core was generated by `/home/xxx/yyyyyyy/git/git merge update'.
> 
> ...helps us out ;-). The only match for "git merge update" is in
> t4058.16, which blames back to ac14de13b2 (t4058: explore duplicate tree
> entry handling in a bit more detail, 2020-12-11), which helpfully
> explains that this segfault is known (and furthermore they are
> long-lived and likely not even worth fixing, per ac14de13b2).

Thanks for finding the commit! Makes absolutely sense, but...

I have a little problem with the approach to have it crashing though. It crashes for every run of the tests: I have a crash core collecting program on the machine I use to build binaries of the tools I use. While it is not hard to isolate builds of Git (as a whole) with coredump collecting switched off I'd prefer to not do it: it's a special case (which gets forgotten) and with it I'll miss new crashes in Git (which I might have authored). It is inconvenient to crash regularly.

Is it reasonable to ask to replace the crash in case of this known breakage with an error()+exit(130)? (`exit(130)` because the test_expect_failure seems to require an exit code greater than 129, and I failed to find where it is).

Or, since the test-lib already has a notion of "expected failure" provide the *tests* with a way to reduce collateral effects of that failure? Like below with the GDB.

Regards, Alex

diff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh
index 54614b814d..b2f9ab07d1 100755
--- a/t/t4058-diff-duplicates.sh
+++ b/t/t4058-diff-duplicates.sh
@@ -132,22 +132,38 @@ test_expect_success 'create a few commits' '
 	rm commit_id up final
 '
 
+may_crash() {
+	local ret
+	if test -n "$GIT_DEBUGGER"
+	then
+		"$@"
+		ret=$?
+	else
+		GIT_DEBUGGER="gdb --batch --return-child-result --nh -ex run --args"
+		export GIT_DEBUGGER
+		"$@"
+		ret=$?
+		unset GIT_DEBUGGER
+	fi
+	return $ret
+}
+
 test_expect_failure 'git read-tree does not segfault' '
 	test_when_finished rm .git/index.lock &&
-	test_might_fail git read-tree --reset base
+	test_might_fail may_crash git read-tree --reset base
 '
 
 test_expect_failure 'reset --hard does not segfault' '
 	test_when_finished rm .git/index.lock &&
 	git checkout base &&
-	test_might_fail git reset --hard
+	test_might_fail may_crash git reset --hard
 '
 
 test_expect_failure 'git diff HEAD does not segfault' '
 	git checkout base &&
 	GIT_TEST_CHECK_CACHE_TREE=false &&
 	git reset --hard &&
-	test_might_fail git diff HEAD
+	test_might_fail may_crash git diff HEAD
 '
 
 test_expect_failure 'can switch to another branch when status is empty' '
@@ -183,7 +199,7 @@ test_expect_success 'switch to base branch and force status to be clean' '
 '
 
 test_expect_failure 'fast-forward from duplicate entries to non-duplicate' '
-	git merge update
+	may_crash git merge update
 '
 
 test_done