From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 23 Sep 2025 09:35:39 GMT
Subject: Re: [PATCH v2 2/4] string-list: replace negative index encoding with "exact_match" parameter
Message-ID: <CAOLa=ZShms1D-cq=x04dtT2ULTVE3ZDo8DODFnJRP2wcJz0EgQ@mail.gmail.com>
In-Reply-To: <aMp9OtXLfRw7dEwA@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

> We would return negative index to indicate exact match by converting the
> original positive index to be "-1 - index" in
> "string_list_find_insert_index", which requires callers to decode this
> information. This approach has several limitations:
>

Nit: It would be nice to start by explaining what
"string_list_find_insert_index" does and then talking about the negative
index. Perhaps something like:

  The `string_list_find_insert_index()` function is used to determine
  the correct insertion index for a new string within the string list.
  The function also doubles up to convey if the string is already
  existing in the list, this is done by returning a negative index
  "-1 -index". Users are expected to decode this information.

> 1. It prevents us from using the full range of size_t, which is
>    necessary for large string list.
> 2. Using int for indices while other parts of the codebase use size_t
>    creates signed comparison warnings when these values are compared.
>
> To address these limitations, change the function to return size_t for
> the index value and use a separate bool parameter to indicate whether
> the index refers to an existing entry or an insertion point.
>
> In some cases, the callers of "string_list_find_insert_index" only need
> the index position and don't care whether an exact match is found.
> However, "get_entry_index" currently requires a non-NULL "exact_match"
> parameter, forcing these callers to declare unnecessary variables.
> Let's allow callers to pass NULL for the "exact_match" parameter when
> they don't need this information, reducing unnecessary variable
> declarations in calling code.
>

Makes sense, and much cleaner..

> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  add-interactive.c |  7 ++++---
>  mailmap.c         |  7 +++----
>  refs.c            |  2 +-
>  string-list.c     | 14 ++++++--------
>  string-list.h     |  2 +-
>  5 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 3e692b47ec..7c0fd3d218 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -221,7 +221,8 @@ static void find_unique_prefixes(struct prefix_item_list *list)
>
>  static ssize_t find_unique(const char *string, struct prefix_item_list *list)
>  {
> -	int index = string_list_find_insert_index(&list->sorted, string, 1);
> +	bool exact_match;
> +	int index = string_list_find_insert_index(&list->sorted, string, &exact_match);
>  	struct string_list_item *item;
>
>  	if (list->items.nr != list->sorted.nr)
> @@ -229,8 +230,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
>  		    " vs %"PRIuMAX")",
>  		    (uintmax_t)list->items.nr, (uintmax_t)list->sorted.nr);
>
> -	if (index < 0)
> -		item = list->sorted.items[-1 - index].util;

Thanks for this, this is so confusing to read if one doesn't know that
the incoming information is encoded with a special format.

Rest of the patch looks great.

[snip]