From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 23 Sep 2025 09:44:42 GMT
Subject: Re: [PATCH v2 3/4] string-list: change "string_list_find_insert_index" return type to "size_t"
Message-ID: <CAOLa=ZSGb_vvcneJ-b7ahL8J46a-FFKzupXaO0eKcyNehwTF+w@mail.gmail.com>
In-Reply-To: <aMp9Qs2AHBax62LL@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

> As "string_list_find_insert_index" is a simple wrapper of
> "get_entry_index" and the return type of "get_entry_index" is already
> "size_t", we could simply change its return type to "size_t".
>
> Update all callers to use size_t variables for storing the return value.
> The tricky fix is the loop condition in "mailmap.c" to properly handle
> "size_t" underflow by changing from `0 <= --i` to `i--`.
>
> Remove "DISABLE_SIGN_COMPARE_WARNINGS" from "mailmap.c" as it's no
> longer needed with the proper unsigned types.
>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  add-interactive.c | 2 +-
>  mailmap.c         | 5 ++---
>  refs.c            | 4 +---
>  string-list.c     | 4 ++--
>  string-list.h     | 4 ++--
>  5 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 7c0fd3d218..19def3168a 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -222,7 +222,7 @@ static void find_unique_prefixes(struct prefix_item_list *list)
>  static ssize_t find_unique(const char *string, struct prefix_item_list *list)
>  {
>  	bool exact_match;
> -	int index = string_list_find_insert_index(&list->sorted, string, &exact_match);
> +	size_t index = string_list_find_insert_index(&list->sorted, string, &exact_match);
>  	struct string_list_item *item;
>
>  	if (list->items.nr != list->sorted.nr)
> diff --git a/mailmap.c b/mailmap.c
> index 58a4484963..37fd158a51 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -1,5 +1,4 @@
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
>  #include "git-compat-util.h"
>  #include "environment.h"
> @@ -244,7 +243,7 @@ static struct string_list_item *lookup_prefix(struct string_list *map,
>  					      const char *string, size_t len)
>  {
>  	bool exact_match;
> -	int i = string_list_find_insert_index(map, string, &exact_match);
> +	size_t i = string_list_find_insert_index(map, string, &exact_match);
>  	if (exact_match) {
>  		if (!string[len])
>  			return &map->items[i];
> @@ -266,7 +265,7 @@ static struct string_list_item *lookup_prefix(struct string_list *map,
>  	 * overlong key would be inserted, which must come after the
>  	 * real location of the key if one exists.
>  	 */
> -	while (0 <= --i && i < map->nr) {
> +	while (i-- && i < map->nr) {
>  		int cmp = strncasecmp(map->items[i].string, string, len);
>  		if (cmp < 0)

So earlier, if `i = 0`, we'd have a negative number and then the loop
would exit. But with `size_t`, i can never be negative. So by using
`i--`, we exit if `i = 0`. Okay

>  			/*
> diff --git a/refs.c b/refs.c
> index f1ff5bf846..a8f06b9a0a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1688,8 +1688,6 @@ const char *find_descendant_ref(const char *dirname,
>  				const struct string_list *extras,
>  				const struct string_list *skip)
>  {
> -	int pos;
> -
>  	if (!extras)
>  		return NULL;
>
> @@ -1699,7 +1697,7 @@ const char *find_descendant_ref(const char *dirname,
>  	 * with dirname (remember, dirname includes the trailing
>  	 * slash) and is not in skip, then we have a conflict.
>  	 */
> -	for (pos = string_list_find_insert_index(extras, dirname, NULL);
> +	for (size_t pos = string_list_find_insert_index(extras, dirname, NULL);
>  	     pos < extras->nr; pos++) {
>  		const char *extra_refname = extras->items[pos].string;
>
> diff --git a/string-list.c b/string-list.c
> index c589ab5a2c..08dc00984c 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -91,8 +91,8 @@ bool string_list_has_string(const struct string_list *list, const char *string)
>  	return exact_match;
>  }
>
> -int string_list_find_insert_index(const struct string_list *list, const char *string,
> -				  bool *exact_match)
> +size_t string_list_find_insert_index(const struct string_list *list, const char *string,
> +				     bool *exact_match)
>  {
>  	return get_entry_index(list, string, exact_match);
>  }
> diff --git a/string-list.h b/string-list.h
> index 8830ce671d..6b0a2f4752 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -173,8 +173,8 @@ void string_list_remove_empty_items(struct string_list *list, int free_util);
>
>  /** Determine if the string_list has a given string or not. */
>  bool string_list_has_string(const struct string_list *list, const char *string);
> -int string_list_find_insert_index(const struct string_list *list, const char *string,
> -				  bool *exact_match);
> +size_t string_list_find_insert_index(const struct string_list *list, const char *string,
> +				     bool *exact_match);
>

Super nit: can we also add documentation to this function while we're here.

>  /**
>   * Insert a new element to the string_list. The returned pointer can
> --
> 2.51.0

The patch looks good to me.