[05/15] run-command tests: use "return", not "exit"

Commit Message

Ævar Arnfjörð Bjarmason Sept. 30, 2022, 11:28 a.m. UTC

Change the "run-command" test helper to "return" instead of calling
"exit", see 338abb0f045 (builtins + test helpers: use return instead of exit() in cmd_*, 2021-06-08)

Because we'd previously gotten past the SANITIZE=leak check by using
exit() here we need to move to "goto cleanup" pattern. See
fdc8f79f1f1 (leak tests: run various "test-tool" tests in t00*.sh
SANITIZE=leak, 2021-10-12) for prior art. for when this code was opted
into the "linux-leaks" job.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 t/helper/test-run-command.c | 44 +++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)

Comments

Phillip Wood Oct. 7, 2022, 9:24 a.m. UTC | #1

Hi Ævar

On 30/09/2022 12:28, Ævar Arnfjörð Bjarmason wrote:
> Change the "run-command" test helper to "return" instead of calling
> "exit", see 338abb0f045 (builtins + test helpers: use return instead of exit() in cmd_*, 2021-06-08)
> 
> Because we'd previously gotten past the SANITIZE=leak check by using
> exit() here we need to move to "goto cleanup" pattern. See
> fdc8f79f1f1 (leak tests: run various "test-tool" tests in t00*.sh
> SANITIZE=leak, 2021-10-12) for prior art. for when this code was opted
> into the "linux-leaks" job.

That commit just adds some TEST_PASSES_SANITIZE_LEAK=true lines, it's 
not nothing to do with "goto cleanup", I don't think we need to 
reference any prior art, just explain why we need to add the cleanup 
which you already do.

>   	if (!strcmp(argv[1], "run-command-parallel")) {
> -		exit(run_processes_parallel(jobs, parallel_next,
> -					    NULL, NULL, &proc));
> +		run_processes_parallel(jobs, parallel_next, NULL, NULL,
> +				       &proc);

There is no explanation of why it is safe to discard the return value 
here. The answer is in the next commit message, but needs to be 
mentioned here as well.

Best Wishes

Phillip

Patch

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 390fa4fb724..ebda2203408 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -381,13 +381,14 @@  int cmd__run_command(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
+	int ret;
 
 	if (argc > 1 && !strcmp(argv[1], "testsuite"))
-		exit(testsuite(argc - 1, argv + 1));
+		return testsuite(argc - 1, argv + 1);
 	if (!strcmp(argv[1], "inherited-handle"))
-		exit(inherit_handle(argv[0]));
+		return inherit_handle(argv[0]);
 	if (!strcmp(argv[1], "inherited-handle-child"))
-		exit(inherit_handle_child());
+		return inherit_handle_child();
 
 	if (argc >= 2 && !strcmp(argv[1], "quote-stress-test"))
 		return !!quote_stress_test(argc - 1, argv + 1);
@@ -404,18 +405,24 @@  int cmd__run_command(int argc, const char **argv)
 		argv += 2;
 		argc -= 2;
 	}
-	if (argc < 3)
-		return 1;
+	if (argc < 3) {
+		ret = 1;
+		goto cleanup;
+	}
 	strvec_pushv(&proc.args, (const char **)argv + 2);
 
 	if (!strcmp(argv[1], "start-command-ENOENT")) {
-		if (start_command(&proc) < 0 && errno == ENOENT)
-			return 0;
+		if (start_command(&proc) < 0 && errno == ENOENT) {
+			ret = 0;
+			goto cleanup;
+		}
 		fprintf(stderr, "FAIL %s\n", argv[1]);
 		return 1;
 	}
-	if (!strcmp(argv[1], "run-command"))
-		exit(run_command(&proc));
+	if (!strcmp(argv[1], "run-command")) {
+		ret = run_command(&proc);
+		goto cleanup;
+	}
 
 	if (!strcmp(argv[1], "--ungroup")) {
 		argv += 1;
@@ -428,16 +435,21 @@  int cmd__run_command(int argc, const char **argv)
 	strvec_pushv(&proc.args, (const char **)argv + 3);
 
 	if (!strcmp(argv[1], "run-command-parallel")) {
-		exit(run_processes_parallel(jobs, parallel_next,
-					    NULL, NULL, &proc));
+		run_processes_parallel(jobs, parallel_next, NULL, NULL,
+				       &proc);
 	} else if (!strcmp(argv[1], "run-command-abort")) {
-		exit(run_processes_parallel(jobs, parallel_next,
-					    NULL, task_finished, &proc));
+		run_processes_parallel(jobs, parallel_next, NULL,
+				       task_finished, &proc);
 	} else if (!strcmp(argv[1], "run-command-no-jobs")) {
-		exit(run_processes_parallel(jobs, no_job,
-					    NULL, task_finished, &proc));
+		run_processes_parallel(jobs, no_job, NULL, task_finished,
+				       &proc);
 	} else {
+		ret = 1;
 		fprintf(stderr, "check usage\n");
-		return 1;
+		goto cleanup;
 	}
+	ret = 0;
+cleanup:
+	child_process_clear(&proc);
+	return ret;
 }