Re: [PATCH v2 21/21] config: fix sign comparison warnings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 23, 2025, 15:29 UTC
- Message-ID
- <xmqqfrenori2.fsf@gitster.g>
- In-Reply-To
- <20250723-pks-config-wo-the-repository-v2-21-1502d60d3867@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 22 quoted lines
> 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.
Show 7 quoted lines
> @@ -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.
Show 14 quoted lines
> 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.
Show 14 quoted lines
> 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.
Show 14 quoted lines
> @@ -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 noThe 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.
Show 10 quoted lines
> @@ -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.