From: Junio C Hamano Date: Mon, 08 Jun 2026 14:52:28 GMT Subject: Re: [PATCH GSoC RFC v12 03/12] cat-file: add declaration of variable i inside its for loop Message-ID: In-Reply-To: <20260608-ps-eric-work-rebase-v12-3-5338b766e658@gmail.com> Pablo Sabater writes: > From: Eric Ju > Subject: Re: [PATCH GSoC RFC v12 03/12] cat-file: add declaration of variable i inside its for loop "add" sounds a bit strange, as the existing code wouldn't have compiled if the variable were never declared. What the patch did was to move (not add) the declaration of a function scope variable that is used to control for() loops. Would any of these work? Subject: [PATCH GSOC v12 03/12] cat-file: narrow scope of loop counter Subject: [PATCH GSOC v12 03/12] cat-file: declare loop counter inside for() > Some code used in this series declares variable i and only uses it > in a for loop, not in any other logic outside the loop. > > Change the declaration of i to be inside the for loop for readability. > While at it, we also change its type from "int" to "size_t" where the latter makes more sense. Curious single line that is overly long? > Helped-by: Christian Couder > Signed-off-by: Eric Ju > Signed-off-by: Pablo Sabater > --- > builtin/cat-file.c | 11 +++-------- > fetch-pack.c | 3 +-- > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index fa45f774d7..c060fd4800 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -726,12 +726,10 @@ static void dispatch_calls(struct batch_options *opt, > struct queued_cmd *cmd, > int nr) > { > - int i; > - > if (!opt->buffer_output) > die(_("flush is only for --buffer mode")); > > - for (i = 0; i < nr; i++) > + for (size_t i = 0; i < nr; i++) > cmd[i].fn(opt, cmd[i].line, output, data); The loop limit "nr" will not become as large as size_t because the caller passes a platform natural "int" to the function. Wouldn't a stupid compiler give us warning on comparing unsigned size_t with signed int here? > @@ -739,9 +737,7 @@ static void dispatch_calls(struct batch_options *opt, > > static void free_cmds(struct queued_cmd *cmd, size_t *nr) > { > - size_t i; > - > - for (i = 0; i < *nr; i++) > + for (size_t i = 0; i < *nr; i++) > FREE_AND_NULL(cmd[i].line); No type change, so the result is as safe as the original. > @@ -768,7 +764,6 @@ static void batch_objects_command(struct batch_options *opt, > size_t alloc = 0, nr = 0; > > while (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) { > - int i; > const struct parse_cmd *cmd = NULL; > const char *p = NULL, *cmd_end; > struct queued_cmd call = {0}; > @@ -778,7 +773,7 @@ static void batch_objects_command(struct batch_options *opt, > if (isspace(*input.buf)) > die(_("whitespace before command: '%s'"), input.buf); > > - for (i = 0; i < ARRAY_SIZE(commands); i++) { > + for (size_t i = 0; i < ARRAY_SIZE(commands); i++) { > if (!skip_prefix(input.buf, commands[i].name, &cmd_end)) > continue; ARRAY_SIZE() is some arithmetic over sizeof(*commands) and sizeof(commands), which is of type size_t, so this is better than the original. Use of size_t i of course is a natural way to index into commands[] array, so the result is just fine. > diff --git a/fetch-pack.c b/fetch-pack.c > index 120e01f3cf..f13951d154 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1388,9 +1388,8 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf, > if (advertise_sid && server_supports_v2("session-id")) > packet_buf_write(req_buf, "session-id=%s", trace2_session_id()); > if (server_options && server_options->nr) { > - int i; > ensure_server_supports_v2("server-option"); > - for (i = 0; i < server_options->nr; i++) > + for (size_t i = 0; i < server_options->nr; i++) > packet_buf_write(req_buf, "server-option=%s", > server_options->items[i].string); server_options is a string_list whose .nr member is of type size_t, so this comparison is perfectly fine. Ditto for ->items[i].string that is a natural way to index into an array. > } v