Subject: Re: [PATCH 3/9] test-mergesort: add test subcommand

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 02 Oct 2021 10:35:40 +0200

On Fri, Oct 01 2021, Junio C Hamano wrote:

> René Scharfe <[email-masked]> writes:
>
>> +static void dist_rand(int *arr, int n, int m)
>> +{
>> +	int i;
>> +	for (i = 0; i < n; i++)
>> +		arr[i] = rand() % m;
>> +}
>> ...
>> +static void dist_shuffle(int *arr, int n, int m)
>> +{
>> +	int i, j, k;
>> +	for (i = j = 0, k = 1; i < n; i++)
>> +		arr[i] = (rand() % m) ? (j += 2) : (k += 2);
>> +}
>
> I briefly wondered if we want to seed the rand() in some way to make
> the tests reproducible, but we'd need to ship our own rand() if we
> wanted to go that route, which would probably be too much.

Wouldn't calling srand() with some constant value suffice on most
platforms? I'm aware of it being a NOOP and rand() always being randomly
seeded on (IIRC) OpenBSD, but that should work on e.g. glibc.

>>  int cmd__mergesort(int argc, const char **argv)
>>  {
>>  	if (argc == 2 && !strcmp(argv[1], "sort"))
>>  		return sort_stdin();
>> -	usage("test-tool mergesort sort");
>> +	if (argc > 1 && !strcmp(argv[1], "test"))
>> +		return run_tests(argc - 2, argv + 2);
>> +	fprintf(stderr, "usage: test-tool mergesort sort\n");
>> +	fprintf(stderr, "   or: test-tool mergesort test [<n>...]\n");
>> +	return 129;
>
> If you can live with OPT_CMDMODE to implement sort/test subcommands,
> you'd get to have parse_options() to do the usage for you, I think.
> I am not sure if it is worth it, as t/helper/ is not end-user
> facing.

Yeah I think the one thing that could improve here is this custom
getopts handling, in particular the manual formatting of "usage" and
"or" is a bit ugly, considering that you'll get it for free with the
parse_options() "usage" array.