Git development
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Muhammed Dilshad A <dilsheddilu123@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 2/3] mergesort: move sorting tests to the unit-test framework
Date: Fri, 9 Oct 2026 13:52:38 +0200	[thread overview]
Message-ID: <asjVhlTj3MqHcGm4@pks.im> (raw)
In-Reply-To: <0429552774367ddcc3c2fda78e09a83650ccfa02.1791365181.git.dilsheddilu123@gmail.com>

On Wed, Oct 07, 2026 at 07:20:24PM +0530, Muhammed Dilshad A wrote:
> The mergesort certification checks exercise C code directly, so they do
> not need a shell test and test-tool command. Move their distributions and
> transformations to Clar, retaining the sorted-value, stability and list
> length checks. Add small cases for both list sort macros and debug hooks.
> 
> Keep node storage available to the cleanup fixture and bound validation
> so a failed assertion can release it without walking a broken list.

Wat...? I have no idea what this means.

I would appreciate it if you would read through the AI generated
messages and ask yourself whether a normal human being would understand
what was being generated. In general, we ask you to fully vet all of the
stuff that is being generated, understand it and convert it into a form
that normal human beings understand.

A commit message is _your_ chance to demonstrate that you understand
what you're contributing. If it's this obviously AI generated it raises
a huge red flag as I will immediately assume that you haven't read any
of the code it wrote.

> diff --git a/t/unit-tests/u-mergesort.c b/t/unit-tests/u-mergesort.c
> new file mode 100644
> index 0000000000..e621c9ec21
> --- /dev/null
> +++ b/t/unit-tests/u-mergesort.c
> @@ -0,0 +1,369 @@
> +#include "unit-test.h"
> +#include "mergesort.h"
> +
> +static uint32_t minstd_rand(uint32_t *state)

All of these distributions are kinda cute. But is it really required to
test the merge sort with half a dozen different distributions? I dunno,
color me sceptical.

That being said, you just retain the old status quo, so okay.

[snip]
> +#define DIST(name) { #name, dist_##name }
> +
> +static struct dist {
> +	const char *name;
> +	void (*fn)(int *arr, int n, int m);
> +} dist[] = {
> +	DIST(sawtooth),
> +	DIST(rand),
> +	DIST(stagger),
> +	DIST(plateau),
> +	DIST(shuffle),
> +};

This also feels quite overengineered now for the unit test infra.

[snip]
> +#define MODE(name) { #name, mode_##name }
> +
> +static struct mode {
> +	const char *name;
> +	void (*fn)(int *arr, int n);
> +} mode[] = {
> +	MODE(copy),
> +	MODE(reverse),
> +	MODE(reverse_1st_half),
> +	MODE(reverse_2nd_half),
> +	MODE(sort),
> +	MODE(dither),
> +	MODE(unriffle),
> +	MODE(unriffle_skewed),
> +};

Same. All of this is way too overengineered. It probably was useful at
one point in time to show performance with these different modes and
distributions. But even with the performance test we don't use those at
all anymore, so it just feels needlessly complex by now.

Patrick

  reply	other threads:[~2026-10-09 11:52 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-07  3:42 [PATCH] test-mergesort: plug memory leaks in sort_stdin() Muhammed Dilshad A
2026-10-07  6:13 ` Patrick Steinhardt
2026-10-07 17:27   ` Junio C Hamano
2026-10-07 13:50 ` [PATCH v2 0/3] mergesort: move tests to Clar and retire the helper Muhammed Dilshad A
2026-10-07 13:50   ` [PATCH v2 1/3] test-mergesort: plug memory leaks in sort_stdin() Muhammed Dilshad A
2026-10-07 13:50   ` [PATCH v2 2/3] mergesort: move sorting tests to the unit-test framework Muhammed Dilshad A
2026-10-09 11:52     ` Patrick Steinhardt [this message]
2026-10-09 13:55       ` Muhammed Dilshad A
2026-10-07 13:50   ` [PATCH v2 3/3] t: retire the sorting benchmark and mergesort helper Muhammed Dilshad A
2026-10-09 11:52   ` [PATCH v2 0/3] mergesort: move tests to Clar and retire the helper Patrick Steinhardt
2026-10-09 13:56     ` Muhammed Dilshad A
2026-10-09 15:08   ` [PATCH v3 0/4] mergesort: move tests to Clar and remove " Muhammed Dilshad A
2026-10-09 15:08     ` [PATCH v3 1/4] mergesort: move sorting tests to Clar Muhammed Dilshad A
2026-10-09 15:08     ` [PATCH v3 2/4] mergesort: simplify the unit tests Muhammed Dilshad A
2026-10-09 15:08     ` [PATCH v3 3/4] mergesort: cover empty and small lists Muhammed Dilshad A
2026-10-09 15:08     ` [PATCH v3 4/4] t: retire the sorting benchmark and mergesort helper Muhammed Dilshad A

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=asjVhlTj3MqHcGm4@pks.im \
    --to=ps@pks.im \
    --cc=dilsheddilu123@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox