Re: [PATCH] test-mergesort: plug memory leaks in sort_stdin()

[Patrick Steinhardt <ps@pks.im>]

On Wed, Oct 07, 2026 at 09:12:05AM +0530, Muhammed Dilshad A wrote:
> The sort_stdin() helper allocates an input buffer and a memory pool for
> the list of lines, but returns without releasing either. Discard the
> pool and release the strbuf after printing the sorted lines.

Makes sense.

> Add a test for the sort subcommand to t0071. The existing test only
> exercises the test subcommand, leaving these leaks undetected by the
> regular leak-sanitized test suite.

I was briefly wondering whether we could get rid of t0071 altogether in
favor of converting the tests into a unit test, and then drop the test
helper. And that's certainly doable, and I'd argue it would also be the
right thing to do. But unfortunately it wouldn't allow us to get rid of
the test helper completely as the "mergesort sort" subcommand is used as
part of our performance tests.

I would claim that the benchmark itself is of dubious value. It was nice
enough to have some numbers when we were working on the implementation
of the mergesort, but carrying it with us nowadays feels like a bit of a
waste as chances for regression are somewhat slim here. And if we ever
wanted to iterate further on the merge sort implementation we could
still introduce a new benchmark, that's easy enough to do.

But anyway, that's of course a much bigger scope, and I'm fine to just
fix the bugs for now.

> diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c
> index 791e128793..3b8c428b14 100644
> --- a/t/helper/test-mergesort.c
> +++ b/t/helper/test-mergesort.c
> @@ -61,6 +61,8 @@ static int sort_stdin(void)
>  		puts(lines->text);
>  		lines = lines->next;
>  	}
> +	mem_pool_discard(&lines_pool, 0);
> +	strbuf_release(&sb);
>  	return 0;
>  }

The fix is obviously correct.

> diff --git a/t/t0071-sort.sh b/t/t0071-sort.sh
> index 2236a7e956..97890da29f 100755
> --- a/t/t0071-sort.sh
> +++ b/t/t0071-sort.sh
> @@ -8,4 +8,11 @@ test_expect_success 'DEFINE_LIST_SORT_DEBUG' '
>  	test-tool mergesort test
>  '
>  
> +test_expect_success 'sort stdin' '
> +	printf "%s\n" c a b >input &&
> +	printf "%s\n" a b c >expect &&
> +	test-tool mergesort sort <input >actual &&
> +	test_cmp expect actual
> +'

And having a test makes sense, I guess.

I noticed that there's another "generate" subcommand here that is
entirely unused. Do we maybe want to also remove it while at it? The
test suite passes with the below diff.

Thanks!

Patrick

diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c
index 791e128793..9200c4bb4a 100644
--- a/t/helper/test-mergesort.c
+++ b/t/helper/test-mergesort.c
@@ -114,16 +114,6 @@ static struct dist {
 	DIST(shuffle),
 };
 
-static const struct dist *get_dist_by_name(const char *name)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(dist); i++) {
-	       if (!strcmp(dist[i].name, name))
-		       return &dist[i];
-	}
-	return NULL;
-}
-
 static void mode_copy(int *arr UNUSED, int n UNUSED)
 {
 	/* nothing */
@@ -237,41 +227,6 @@ static struct mode {
 	MODE(unriffle_skewed),
 };
 
-static const struct mode *get_mode_by_name(const char *name)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(mode); i++) {
-	       if (!strcmp(mode[i].name, name))
-		       return &mode[i];
-	}
-	return NULL;
-}
-
-static int generate(int argc, const char **argv)
-{
-	const struct dist *dist = NULL;
-	const struct mode *mode = NULL;
-	int i, n, m, *arr;
-
-	if (argc != 4)
-		return 1;
-
-	dist = get_dist_by_name(argv[0]);
-	mode = get_mode_by_name(argv[1]);
-	n = strtol(argv[2], NULL, 10);
-	m = strtol(argv[3], NULL, 10);
-	if (!dist || !mode)
-		return 1;
-
-	ALLOC_ARRAY(arr, n);
-	dist->fn(arr, n, m);
-	mode->fn(arr, n);
-	for (i = 0; i < n; i++)
-		printf("%08x\n", arr[i]);
-	free(arr);
-	return 0;
-}
-
 static struct stats {
 	int get_next, set_next, compare;
 } stats;
@@ -388,14 +343,11 @@ int cmd__mergesort(int argc, const char **argv)
 	int i;
 	const char *sep;
 
-	if (argc == 6 && !strcmp(argv[1], "generate"))
-		return generate(argc - 2, argv + 2);
 	if (argc == 2 && !strcmp(argv[1], "sort"))
 		return sort_stdin();
 	if (argc > 1 && !strcmp(argv[1], "test"))
 		return run_tests(argc - 2, argv + 2);
-	fprintf(stderr, "usage: test-tool mergesort generate <distribution> <mode> <n> <m>\n");
-	fprintf(stderr, "   or: test-tool mergesort sort\n");
+	fprintf(stderr, "usage: test-tool mergesort sort\n");
 	fprintf(stderr, "   or: test-tool mergesort test [<n>...]\n");
 	fprintf(stderr, "\n");
 	for (i = 0, sep = "distributions: "; i < ARRAY_SIZE(dist); i++, sep = ", ")