Re: [PATCH v2 21/21] config: fix sign comparison warnings
From: Junio C Hamano <hidden>
Date: 2025-07-23 15:29:11
Patrick Steinhardt [off-list ref] writes:
quoted hunk
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.
quoted hunk
@@ -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.
quoted hunk
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.
quoted hunk
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.
quoted hunk
@@ -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.
quoted hunk
@@ -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.