On Mon, May 19, 2025 at 03:58:13AM -0400, Jeff King wrote:

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

quoted

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.

It seems that we create this in a long time ago. In 8fd2cb4069 (Extract
helper bits from c-merge-recursive work, 2006-07-25), we introduce the
"path-list.c", at that time, we have the code already.

-Peff

Thanks,
Jialuo