Re: [PATCH v2 21/21] config: fix sign comparison warnings

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

>  static int prepare_include_condition_pattern(const struct key_value_info *kvi,
> -					     struct strbuf *pat)
> +					     struct strbuf *pat,
> +					     size_t *out)
>  {
>  	struct strbuf path = STRBUF_INIT;
>  	char *expanded;
> -	int prefix = 0;
> +	size_t prefix = 0;
>  
>  	expanded = interpolate_path(pat->buf, 1);
>  	if (expanded) {
> @@ -229,8 +228,10 @@ static int prepare_include_condition_pattern(const struct key_value_info *kvi,
>  
>  	add_trailing_starstar_for_dir(pat);
>  
> +	*out = prefix;
> +
>  	strbuf_release(&path);
> -	return prefix;
> +	return 0;
>  }

OK, so unlike the previous attempt, this one uses an out parameter
to return the length of the prefix it counted, so the caller should
use its return value solely for noticing an error.

The first iteration would have given the caller (size_t)-1 and the
"ah, we got a non-zero prefix, let's see if it matches the string we
have" code that follows in the caller safely jumps to the "done"
label without doing any harm, so in that sense, the breakage there
was also merely theoretical ;-), but this certainly is more correct.

> @@ -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;

And this is how the caller adjusts to the calling convention.

>  	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;

Same.  Negative return value is checked for early return upon an
error, and the value we received in prefix is used as before.

>  	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;

Variable "i" of type ul looks confusing, but it is a counter to
count up to "cout" that is a result from strtoul(), so everything is
now consistent.  Good.

Not that we would seriously expect that we can export more than 2
billion environment variables sensibly, so this is all theoretical
exercise ;-)

> -			strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
> +			strbuf_addf(&envvar, "GIT_CONFIG_KEY_%lu", i);

Good.

> @@ -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

The loop in the body of this function makes references to an array
with the index (i-1), but i counts downwards and never reaches 0, so
this should be OK.  The next hunk is a change that adjust the caller
to this change.

> @@ -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)

Looks good.  Thanks.