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

[Patrick Steinhardt <ps@pks.im>]

On Mon, May 26, 2025 at 10:01:57PM +0800, shejialuo wrote:
> On Mon, May 19, 2025 at 03:51:19AM -0400, Jeff King wrote:
> > 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).
> > 
> 
> Thanks for the hint. By seeing this commit, I totally understand the
> history. Because we delete `string_list_insert_at_index`, we simply call
> "add_entry" by specifying "auto" mode and somehow we don't delete the
> legacy check in "add_entry".
> 
> But I have one question: should I include the information in the commit
> message? I feel doing this would be chaty. But I somehow think we should
> do this.

I would recommend including such information in the commit message, yes.
It helps the reviewer to understand the context and makes it easier for
them to figure out why this seemingly nonsensical parameter exists in
the first place.

Patrick