From: Jeff King <peff@peff.net>
Date: Sat, 22 Apr 2023 11:16:57 GMT
Subject: Re: [PATCH v2 3/6] t/helper/test-hashmap.c: avoid using `strtok()`
Message-ID: <20230422111657.GD2969939@coredump.intra.peff.net>
In-Reply-To: <0ae07dec3663d7cbb0f8662c47485c0667a879b9.1681845518.git.me@ttaylorr.com>

On Tue, Apr 18, 2023 at 03:18:49PM -0400, Taylor Blau wrote:

> @@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)
>  
>  	/* process commands from stdin */
>  	while (strbuf_getline(&line, stdin) != EOF) {
> -		char *cmd, *p1 = NULL, *p2 = NULL;
> +		char *cmd, *p1, *p2;
>  		unsigned int hash = 0;
>  		struct test_entry *entry;
>  
> +		/*
> +		 * Because we memdup() the arguments out of the
> +		 * string_list before inserting them into the hashmap,
> +		 * it's OK to set its length back to zero to avoid
> +		 * re-allocating the items array once per line.
> +		 *
> +		 * By doing so, we'll instead overwrite the existing
> +		 * entries and avoid re-allocating.
> +		 */
> +		string_list_setlen(&parts, 0);
>  		/* break line into command and up to two parameters */
> -		cmd = strtok(line.buf, DELIM);
> +		string_list_split_in_place_multi(&parts, line.buf, DELIM, 2);
> +

I'd argue we can drop this comment now. Having string_list_setlen()
makes it a blessed pattern, and I don't think there's anything special
about this caller that makes it more or less so. Obviously yes, the
string list items won't be valid as we enter a new loop iteration. But
that is always true of split_in_place(), not to mention strtok(),
because we are overwriting the buffer in each loop.

Ditto for the later commits which have similar (if shorter) comments.

-Peff