From: Pablo Sabater Date: Tue, 09 Jun 2026 17:55:08 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: El lun, 8 jun 2026 a las 16:52, Junio C Hamano () escribió: > > 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() > Hi! True, it's better to write "move" and about the title, works as well, I'll change them for the next version. > > 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? True, I'll wrap it up correctly in the next version. > > > 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? Yes, I'll change "i" to be int. > > > @@ -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 Thanks, Pablo.