From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 23 Jul 2025 14:48:37 GMT
Subject: Re: [PATCH v2 21/21] config: fix sign comparison warnings
Message-ID: <4ea2b50c-7277-432b-bcb6-912a0cbcb261@gmail.com>
In-Reply-To: <20250723-pks-config-wo-the-repository-v2-21-1502d60d3867@pks.im>

On 23/07/2025 15:08, Patrick Steinhardt wrote:
> There are a couple of -Wsign-compare warnings in "config.c":
> 
>    - `prepare_include_condition_pattern()` is returns a signed integer,
>      where it either returns a negative error code or the index of the
>      last dir separator in a path. That index will always be a
>      non-negative number, but we cannot just change the return type to a
>      `size_t` due to it being re-used as error code. This is fixed by
>      splitting up concerns: the return value is only used as error code,
>      and the prefix is now returned via an out-pointer. This fixes a sign
>      comparison warning when comparing `text.len < prefix`,
> 

Sounds good, let's see how the caller is affected ...

> @@ -239,7 +240,8 @@ static int include_by_gitdir(const struct key_value_info *kvi,
>   {
>   	struct strbuf text = STRBUF_INIT;
>   	struct strbuf pattern = STRBUF_INIT;
> -	int ret = 0, prefix;
> +	size_t prefix;
> +	int ret = 0;
>   	const char *git_dir;
>   	int already_tried_absolute = 0;
>   
> @@ -250,12 +252,11 @@ static int include_by_gitdir(const struct key_value_info *kvi,
>   
>   	strbuf_realpath(&text, git_dir, 1);
>   	strbuf_add(&pattern, cond, cond_len);
> -	prefix = prepare_include_condition_pattern(kvi, &pattern);
> -
> -again:
> -	if (prefix < 0)
> +	ret = prepare_include_condition_pattern(kvi, &pattern, &prefix);
> +	if (ret < 0)
>   		goto done;
>   
> +again:
prefix is not modified below this point so moving the label down below 
the error check is safe.

This looks good, thanks

Phillip

>   	if (prefix > 0) {
>   		/*
>   		 * perform literal matching on the prefix part so that
> @@ -724,7 +725,6 @@ int git_config_from_parameters(config_fn_t fn, void *data)
>   	if (env) {
>   		unsigned long count;
>   		char *endp;
> -		int i;
>   
>   		count = strtoul(env, &endp, 10);
>   		if (*endp) {
> @@ -736,10 +736,10 @@ int git_config_from_parameters(config_fn_t fn, void *data)
>   			goto out;
>   		}
>   
> -		for (i = 0; i < count; i++) {
> +		for (unsigned long i = 0; i < count; i++) {
>   			const char *key, *value;
>   
> -			strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
> +			strbuf_addf(&envvar, "GIT_CONFIG_KEY_%lu", i);
>   			key = getenv_safe(&to_free, envvar.buf);
>   			if (!key) {
>   				ret = error(_("missing config key %s"), envvar.buf);
> @@ -747,7 +747,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
>   			}
>   			strbuf_reset(&envvar);
>   
> -			strbuf_addf(&envvar, "GIT_CONFIG_VALUE_%d", i);
> +			strbuf_addf(&envvar, "GIT_CONFIG_VALUE_%lu", i);
>   			value = getenv_safe(&to_free, envvar.buf);
>   			if (!value) {
>   				ret = error(_("missing config value %s"), envvar.buf);
> @@ -1614,13 +1614,13 @@ int config_with_options(config_fn_t fn, void *data,
>   
>   static void configset_iter(struct config_set *set, config_fn_t fn, void *data)
>   {
> -	int i, value_index;
> +	int value_index;
>   	struct string_list *values;
>   	struct config_set_element *entry;
>   	struct configset_list *list = &set->list;
>   	struct config_context ctx = CONFIG_CONTEXT_INIT;
>   
> -	for (i = 0; i < list->nr; i++) {
> +	for (size_t i = 0; i < list->nr; i++) {
>   		entry = list->items[i].e;
>   		value_index = list->items[i].value_index;
>   		values = &entry->value_list;
> @@ -2470,10 +2470,11 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
>    */
>   static void maybe_remove_section(struct config_store_data *store,
>   				 size_t *begin_offset, size_t *end_offset,
> -				 int *seen_ptr)
> +				 unsigned *seen_ptr)
>   {
>   	size_t begin;
> -	int i, seen, section_seen = 0;
> +	int section_seen = 0;
> +	unsigned int i, seen;
>   
>   	/*
>   	 * First, ensure that this is the first key, and that there are no
> @@ -2716,7 +2717,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
>   	} else {
>   		struct stat st;
>   		size_t copy_begin, copy_end;
> -		int i, new_line = 0;
> +		unsigned i;
> +		int new_line = 0;
>   		struct config_options opts;
>   
>   		if (!value_pattern)
>