Re: [PATCH v2 2/8] string-list: remove unused "insert_at" parameter from add_entry

From: Jeff King <peff@peff.net>
Date: May 19, 2025, 07:51 UTC
Message-ID: <20250519075119.GE102701@coredump.intra.peff.net>
In-Reply-To: <aCoDU46MmoGPB60b@ArchLinux>

On Sun, May 18, 2025 at 11:57:07PM +0800, shejialuo wrote:

Show 11 quoted lines

> In "add_entry", we accept "insert_at" parameter which must be either -1
> (auto) or between 0 and `list->nr` inclusive. Any other value is
> invalid. When caller specify any invalid "insert_at" value, we won't
> check the range and move the element, which would definitely cause the
> trouble.
> 
> However, we only use "add_entry" in "string_list_insert" function and we
> always pass the "-1" for "insert_at" parameter. So, we never use this
> parameter to insert element in a user specified position. Let's delete
> this parameter. If there is any requirement later, we need to use a
> better way to do this.

We can see from looking at the code that removing this will not change the behavior. But that always makes me wonder why it was there in the first place, and whether we might ever want it.

The answer in this case is that we used to have another function, string_list_insert_at_index(), which used the extra insert_at parameter. The idea being that you could call string_list_find_insert_index(), decide whether there was something already there, and then insert without repeating the binary search.

But you can see in callers like 63226218ba (mailmap: use higher level string list functions, 2014-11-24) that this was not really that useful (in that commit we just try to insert and check the util pointer to see if we need to add the auxiliary structure).

So the function went away in f8c4ab611a (string_list: remove string_list_insert_at_index() from its API, 2014-11-24), and I suspect we won't need it again. (Also, I think these days we'd probably use a strmap instead anyway).

-Peff