Re: [PATCH v2 3/8] string-list: return index directly when inserting an existing element

From: Jeff King <peff@peff.net>
Date: May 19, 2025, 07:58 UTC
Message-ID: <20250519075813.GF102701@coredump.intra.peff.net>
In-Reply-To: <aCoDW8CcWeq8T9hp@ArchLinux>

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

Show 12 quoted lines

> When inserting an existing element, "add_entry" would convert "index"
> value to "-1-index" to indicate the caller that this element is in the
> list already.
> 
> However, in "string_list_insert", we would simply convert this to the
> original positive index without any further action. Let's directly
> return the index as we don't care about whether the element is in the
> list by using "add_entry".
> 
> In the future, if we want to let "add_entry" tell the caller, we may add
> "int *exact_match" parameter to "add_entry" instead of converting the
> index to negative to indicate.

I assumed this was in the same boat as the previous change: something we used to use and now don't. But I don't think we ever did. The "-1-index" pattern goes all the way back to the beginning of the code.

It does match how other functions like string_list_find_insert_index() behave. But I think that pattern doesn't make much sense for add_entry(). After the function returns we know we've either found something or added it, so the positive index will always point to a matching entry.

So I think your patches are correct, but I was curious how we got to this state.

-Peff