{"thread":{"id":"63465","subject":"[PATCH 00/11] CodeQL-inspired fixes","startedAt":"2025-05-15T13:11:53Z","lastAt":"2025-05-22T06:03:39Z","messageCount":36,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"518123","messageId":"pull.1891.git.1747314709.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":null,"subject":"[PATCH 00/11] CodeQL-inspired fixes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:38Z","receivedAt":"2025-05-15T13:11:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"CodeQL [https://codeql.github.com/] pointed out a couple of issues, which\nare addressed in this patch series.\n\nJohannes Schindelin (11):\n commit: simplify code\n fetch: carefully clear local variable's address after use\n commit-graph: avoid malloc'ing a local variable\n upload-pack: rename `enum` to reflect the operation\n has_dir_name(): make code more obvious\n fetch: avoid unnecessary work when there is no current branch\n Avoid redundant conditions\n trace2: avoid \"futile conditional\"\n commit-graph: avoid using stale stack addresses\n bundle-uri: avoid using undefined output of `sscanf()`\n sequencer: stop pretending that an assignment is a condition\n\n builtin/commit.c | 2 +-\n builtin/fetch.c | 3 +-\n bundle-uri.c | 12 ++--\n commit-graph.c | 148 +++++++++++++++++++++++----------------------\n help.c | 2 +-\n read-cache.c | 55 ++++-------------\n sequencer.c | 9 ++-\n trace2/tr2_tmr.c | 24 ++------\n transport-helper.c | 2 +-\n upload-pack.c | 34 +++++------\n 10 files changed, 130 insertions(+), 161 deletions(-)\n\n\nbase-commit: 38af977b81bbf8ce8c0004d3f4046a823ecb30a1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1891%2Fdscho%2Fcodeql-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1891/dscho/codeql-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1891\n-- \ngitgitgadget\n"},{"id":"518124","messageId":"c66eaee64ad5484120f1905ce5501fd8b75b2266.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 02/11] fetch: carefully clear local variable's address after use","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:40Z","receivedAt":"2025-05-15T13:11:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs pointed out by CodeQL, it is a potentially dangerous practice to\nstore local variables' addresses in non-local structs. Yet this is\nexactly what happens with the `acked_commits` attribute that is used in\n`cmd_fetch()`: The pointer to a local variable is assigned to it.\n\nNow, it is Git's convention that `cmd_*()` functions are essentially\nonly returning just before exiting the process, therefore there is\nlittle danger that this attribute is used after the code flow returns\nfrom that function.\n\nHowever, code in `cmd_*()` function is often so useful that it gets\nlifted into a library function, at which point this issue could become a\nreal problem.\n\nLet's make sure to clear the `acked_commits` attribute out after it was\nused, and before the function returns (at which point the address would\ngo stale).\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fetch.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex cda6eaf1fd6e..c1a1434c7096 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2560,6 +2560,7 @@ int cmd_fetch(int argc,\n \t\tif (server_options.nr)\n \t\t\tgtransport->server_options = &server_options;\n \t\tresult = transport_fetch_refs(gtransport, NULL);\n+\t\tgtransport->smart_options->acked_commits = NULL;\n \n \t\toidset_iter_init(&acked_commits, &iter);\n \t\twhile ((oid = oidset_iter_next(&iter)))\n-- \ngitgitgadget\n\n"},{"id":"518125","messageId":"5a3a8880a68f8c69c2af39d2e32ebb56eb5fa483.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 03/11] commit-graph: avoid malloc'ing a local variable","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:41Z","receivedAt":"2025-05-15T13:11:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe do need a context to write the commit graph, but that context is only\nneeded during the life time of `commit_graph_write()`, therefore it can\neasily be a stack variable.\n\nThis also helps CodeQL recognize that it is safe to assign the address\nof other local variables to the context's fields.\n\nSigned-off-by: Johannes Schindelin \n---\n commit-graph.c | 141 ++++++++++++++++++++++++-------------------------\n 1 file changed, 69 insertions(+), 72 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6394752b0b08..9f0115dac9b5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2509,7 +2509,17 @@ int write_commit_graph(struct object_directory *odb,\n \t\t const struct commit_graph_opts *opts)\n {\n \tstruct repository *r = the_repository;\n-\tstruct write_commit_graph_context *ctx;\n+\tstruct write_commit_graph_context ctx = {\n+\t\t.r = r,\n+\t\t.odb = odb,\n+\t\t.append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0,\n+\t\t.report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0,\n+\t\t.split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0,\n+\t\t.opts = opts,\n+\t\t.total_bloom_filter_data_size = 0,\n+\t\t.write_generation_data = (get_configured_generation_version(r) == 2),\n+\t\t.num_generation_data_overflows = 0,\n+\t};\n \tuint32_t i;\n \tint res = 0;\n \tint replace = 0;\n@@ -2531,17 +2541,6 @@ int write_commit_graph(struct object_directory *odb,\n \t\treturn 0;\n \t}\n \n-\tCALLOC_ARRAY(ctx, 1);\n-\tctx->r = r;\n-\tctx->odb = odb;\n-\tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n-\tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n-\tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n-\tctx->opts = opts;\n-\tctx->total_bloom_filter_data_size = 0;\n-\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n-\tctx->num_generation_data_overflows = 0;\n-\n \tbloom_settings.hash_version = r->settings.commit_graph_changed_paths_version;\n \tbloom_settings.bits_per_entry = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY\",\n \t\t\t\t\t\t bloom_settings.bits_per_entry);\n@@ -2549,14 +2548,14 @@ int write_commit_graph(struct object_directory *odb,\n \t\t\t\t\t\t bloom_settings.num_hashes);\n \tbloom_settings.max_changed_paths = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS\",\n \t\t\t\t\t\t\t bloom_settings.max_changed_paths);\n-\tctx->bloom_settings = &bloom_settings;\n+\tctx.bloom_settings = &bloom_settings;\n \n \tinit_topo_level_slab(&topo_levels);\n-\tctx->topo_levels = &topo_levels;\n+\tctx.topo_levels = &topo_levels;\n \n-\tprepare_commit_graph(ctx->r);\n-\tif (ctx->r->objects->commit_graph) {\n-\t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n+\tprepare_commit_graph(ctx.r);\n+\tif (ctx.r->objects->commit_graph) {\n+\t\tstruct commit_graph *g = ctx.r->objects->commit_graph;\n \n \t\twhile (g) {\n \t\t\tg->topo_levels = &topo_levels;\n@@ -2565,15 +2564,15 @@ int write_commit_graph(struct object_directory *odb,\n \t}\n \n \tif (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS)\n-\t\tctx->changed_paths = 1;\n+\t\tctx.changed_paths = 1;\n \tif (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) {\n \t\tstruct commit_graph *g;\n \n-\t\tg = ctx->r->objects->commit_graph;\n+\t\tg = ctx.r->objects->commit_graph;\n \n \t\t/* We have changed-paths already. Keep them in the next graph */\n \t\tif (g && g->bloom_filter_settings) {\n-\t\t\tctx->changed_paths = 1;\n+\t\t\tctx.changed_paths = 1;\n \n \t\t\t/* don't propagate the hash_version unless unspecified */\n \t\t\tif (bloom_settings.hash_version == -1)\n@@ -2586,116 +2585,114 @@ int write_commit_graph(struct object_directory *odb,\n \n \tbloom_settings.hash_version = bloom_settings.hash_version == 2 ? 2 : 1;\n \n-\tif (ctx->split) {\n-\t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n+\tif (ctx.split) {\n+\t\tstruct commit_graph *g = ctx.r->objects->commit_graph;\n \n \t\twhile (g) {\n-\t\t\tctx->num_commit_graphs_before++;\n+\t\t\tctx.num_commit_graphs_before++;\n \t\t\tg = g->base_graph;\n \t\t}\n \n-\t\tif (ctx->num_commit_graphs_before) {\n-\t\t\tALLOC_ARRAY(ctx->commit_graph_filenames_before, ctx->num_commit_graphs_before);\n-\t\t\ti = ctx->num_commit_graphs_before;\n-\t\t\tg = ctx->r->objects->commit_graph;\n+\t\tif (ctx.num_commit_graphs_before) {\n+\t\t\tALLOC_ARRAY(ctx.commit_graph_filenames_before, ctx.num_commit_graphs_before);\n+\t\t\ti = ctx.num_commit_graphs_before;\n+\t\t\tg = ctx.r->objects->commit_graph;\n \n \t\t\twhile (g) {\n-\t\t\t\tctx->commit_graph_filenames_before[--i] = xstrdup(g->filename);\n+\t\t\t\tctx.commit_graph_filenames_before[--i] = xstrdup(g->filename);\n \t\t\t\tg = g->base_graph;\n \t\t\t}\n \t\t}\n \n-\t\tif (ctx->opts)\n-\t\t\treplace = ctx->opts->split_flags & COMMIT_GRAPH_SPLIT_REPLACE;\n+\t\tif (ctx.opts)\n+\t\t\treplace = ctx.opts->split_flags & COMMIT_GRAPH_SPLIT_REPLACE;\n \t}\n \n-\tctx->approx_nr_objects = repo_approximate_object_count(the_repository);\n+\tctx.approx_nr_objects = repo_approximate_object_count(the_repository);\n \n-\tif (ctx->append && ctx->r->objects->commit_graph) {\n-\t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n+\tif (ctx.append && ctx.r->objects->commit_graph) {\n+\t\tstruct commit_graph *g = ctx.r->objects->commit_graph;\n \t\tfor (i = 0; i < g->num_commits; i++) {\n \t\t\tstruct object_id oid;\n \t\t\toidread(&oid, g->chunk_oid_lookup + st_mult(g->hash_len, i),\n \t\t\t\tthe_repository->hash_algo);\n-\t\t\toid_array_append(&ctx->oids, &oid);\n+\t\t\toid_array_append(&ctx.oids, &oid);\n \t\t}\n \t}\n \n \tif (pack_indexes) {\n-\t\tctx->order_by_pack = 1;\n-\t\tif ((res = fill_oids_from_packs(ctx, pack_indexes)))\n+\t\tctx.order_by_pack = 1;\n+\t\tif ((res = fill_oids_from_packs(&ctx, pack_indexes)))\n \t\t\tgoto cleanup;\n \t}\n \n \tif (commits) {\n-\t\tif ((res = fill_oids_from_commits(ctx, commits)))\n+\t\tif ((res = fill_oids_from_commits(&ctx, commits)))\n \t\t\tgoto cleanup;\n \t}\n \n \tif (!pack_indexes && !commits) {\n-\t\tctx->order_by_pack = 1;\n-\t\tfill_oids_from_all_packs(ctx);\n+\t\tctx.order_by_pack = 1;\n+\t\tfill_oids_from_all_packs(&ctx);\n \t}\n \n-\tclose_reachable(ctx);\n+\tclose_reachable(&ctx);\n \n-\tcopy_oids_to_commits(ctx);\n+\tcopy_oids_to_commits(&ctx);\n \n-\tif (ctx->commits.nr >= GRAPH_EDGE_LAST_MASK) {\n+\tif (ctx.commits.nr >= GRAPH_EDGE_LAST_MASK) {\n \t\terror(_(\"too many commits to write graph\"));\n \t\tres = -1;\n \t\tgoto cleanup;\n \t}\n \n-\tif (!ctx->commits.nr && !replace)\n+\tif (!ctx.commits.nr && !replace)\n \t\tgoto cleanup;\n \n-\tif (ctx->split) {\n-\t\tsplit_graph_merge_strategy(ctx);\n+\tif (ctx.split) {\n+\t\tsplit_graph_merge_strategy(&ctx);\n \n \t\tif (!replace)\n-\t\t\tmerge_commit_graphs(ctx);\n+\t\t\tmerge_commit_graphs(&ctx);\n \t} else\n-\t\tctx->num_commit_graphs_after = 1;\n+\t\tctx.num_commit_graphs_after = 1;\n \n-\tctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph);\n+\tctx.trust_generation_numbers = validate_mixed_generation_chain(ctx.r->objects->commit_graph);\n \n-\tcompute_topological_levels(ctx);\n-\tif (ctx->write_generation_data)\n-\t\tcompute_generation_numbers(ctx);\n+\tcompute_topological_levels(&ctx);\n+\tif (ctx.write_generation_data)\n+\t\tcompute_generation_numbers(&ctx);\n \n-\tif (ctx->changed_paths)\n-\t\tcompute_bloom_filters(ctx);\n+\tif (ctx.changed_paths)\n+\t\tcompute_bloom_filters(&ctx);\n \n-\tres = write_commit_graph_file(ctx);\n+\tres = write_commit_graph_file(&ctx);\n \n-\tif (ctx->changed_paths)\n+\tif (ctx.changed_paths)\n \t\tdeinit_bloom_filters();\n \n-\tif (ctx->split)\n-\t\tmark_commit_graphs(ctx);\n+\tif (ctx.split)\n+\t\tmark_commit_graphs(&ctx);\n \n-\texpire_commit_graphs(ctx);\n+\texpire_commit_graphs(&ctx);\n \n cleanup:\n-\tfree(ctx->graph_name);\n-\tfree(ctx->base_graph_name);\n-\tfree(ctx->commits.list);\n-\toid_array_clear(&ctx->oids);\n+\tfree(ctx.graph_name);\n+\tfree(ctx.base_graph_name);\n+\tfree(ctx.commits.list);\n+\toid_array_clear(&ctx.oids);\n \tclear_topo_level_slab(&topo_levels);\n \n-\tfor (i = 0; i < ctx->num_commit_graphs_before; i++)\n-\t\tfree(ctx->commit_graph_filenames_before[i]);\n-\tfree(ctx->commit_graph_filenames_before);\n+\tfor (i = 0; i < ctx.num_commit_graphs_before; i++)\n+\t\tfree(ctx.commit_graph_filenames_before[i]);\n+\tfree(ctx.commit_graph_filenames_before);\n \n-\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n-\t\tfree(ctx->commit_graph_filenames_after[i]);\n-\t\tfree(ctx->commit_graph_hash_after[i]);\n+\tfor (i = 0; i < ctx.num_commit_graphs_after; i++) {\n+\t\tfree(ctx.commit_graph_filenames_after[i]);\n+\t\tfree(ctx.commit_graph_hash_after[i]);\n \t}\n-\tfree(ctx->commit_graph_filenames_after);\n-\tfree(ctx->commit_graph_hash_after);\n-\n-\tfree(ctx);\n+\tfree(ctx.commit_graph_filenames_after);\n+\tfree(ctx.commit_graph_hash_after);\n \n \treturn res;\n }\n-- \ngitgitgadget\n\n"},{"id":"518126","messageId":"8d712a0ebc8d5161bdc80bdd7f4d05a967c66a09.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 04/11] upload-pack: rename `enum` to reflect the operation","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:42Z","receivedAt":"2025-05-15T13:11:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhile 3145ea957d (upload-pack: introduce fetch server command,\n2018-03-15) added support for the `fetch` command, from the server's\npoint of view it is an upload, and hence the `enum` should really be\ncalled `upload_state` instead of `fetch_state`. Likewise, rename its\nvalues.\n\nThis also helps unconfuse CodeQL which would otherwise be at sixes or\nsevens about having _two_ non-local definitions of the same `enum` with\nthe same values.\n\nSigned-off-by: Johannes Schindelin \n---\n upload-pack.c | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 956da5b061a0..26f29b85b551 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1780,16 +1780,16 @@ static void send_shallow_info(struct upload_pack_data *data)\n \tpacket_delim(1);\n }\n \n-enum fetch_state {\n-\tFETCH_PROCESS_ARGS = 0,\n-\tFETCH_SEND_ACKS,\n-\tFETCH_SEND_PACK,\n-\tFETCH_DONE,\n+enum upload_state {\n+\tUPLOAD_PROCESS_ARGS = 0,\n+\tUPLOAD_SEND_ACKS,\n+\tUPLOAD_SEND_PACK,\n+\tUPLOAD_DONE,\n };\n \n int upload_pack_v2(struct repository *r, struct packet_reader *request)\n {\n-\tenum fetch_state state = FETCH_PROCESS_ARGS;\n+\tenum upload_state state = UPLOAD_PROCESS_ARGS;\n \tstruct upload_pack_data data;\n \n \tclear_object_flags(the_repository, ALL_FLAGS);\n@@ -1798,9 +1798,9 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)\n \tdata.use_sideband = LARGE_PACKET_MAX;\n \tget_upload_pack_config(r, &data);\n \n-\twhile (state != FETCH_DONE) {\n+\twhile (state != UPLOAD_DONE) {\n \t\tswitch (state) {\n-\t\tcase FETCH_PROCESS_ARGS:\n+\t\tcase UPLOAD_PROCESS_ARGS:\n \t\t\tprocess_args(request, &data);\n \n \t\t\tif (!data.want_obj.nr && !data.wait_for_done) {\n@@ -1811,27 +1811,27 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)\n \t\t\t\t * to just send 'have's without 'want's); guess\n \t\t\t\t * they didn't want anything.\n \t\t\t\t */\n-\t\t\t\tstate = FETCH_DONE;\n+\t\t\t\tstate = UPLOAD_DONE;\n \t\t\t} else if (data.seen_haves) {\n \t\t\t\t/*\n \t\t\t\t * Request had 'have' lines, so lets ACK them.\n \t\t\t\t */\n-\t\t\t\tstate = FETCH_SEND_ACKS;\n+\t\t\t\tstate = UPLOAD_SEND_ACKS;\n \t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * Request had 'want's but no 'have's so we can\n \t\t\t\t * immediately go to construct and send a pack.\n \t\t\t\t */\n-\t\t\t\tstate = FETCH_SEND_PACK;\n+\t\t\t\tstate = UPLOAD_SEND_PACK;\n \t\t\t}\n \t\t\tbreak;\n-\t\tcase FETCH_SEND_ACKS:\n+\t\tcase UPLOAD_SEND_ACKS:\n \t\t\tif (process_haves_and_send_acks(&data))\n-\t\t\t\tstate = FETCH_SEND_PACK;\n+\t\t\t\tstate = UPLOAD_SEND_PACK;\n \t\t\telse\n-\t\t\t\tstate = FETCH_DONE;\n+\t\t\t\tstate = UPLOAD_DONE;\n \t\t\tbreak;\n-\t\tcase FETCH_SEND_PACK:\n+\t\tcase UPLOAD_SEND_PACK:\n \t\t\tsend_wanted_ref_info(&data);\n \t\t\tsend_shallow_info(&data);\n \n@@ -1841,9 +1841,9 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)\n \t\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n \t\t\t\tcreate_pack_file(&data, NULL);\n \t\t\t}\n-\t\t\tstate = FETCH_DONE;\n+\t\t\tstate = UPLOAD_DONE;\n \t\t\tbreak;\n-\t\tcase FETCH_DONE:\n+\t\tcase UPLOAD_DONE:\n \t\t\tcontinue;\n \t\t}\n \t}\n-- \ngitgitgadget\n\n"},{"id":"518127","messageId":"80422a5770ded04993c73c657b363ddad45e2f4a.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 05/11] has_dir_name(): make code more obvious","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:43Z","receivedAt":"2025-05-15T13:11:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOne thing that might be non-obvious to readers (or to analyzers like\nCodeQL) is that the function essentially does nothing when the Git index\nis empty, and in particular that it does not look at the value of\n`len_eq_last` (which would be uninitialized at that point).\n\nLet's make this much easier to understand, by returning early if the Git\nindex is empty, and by avoiding empty `else` blocks.\n\nThis commit changes indentation and is hence best viewed using\n`--ignore-space-change`.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 55 +++++++++++++---------------------------------------\n 1 file changed, 13 insertions(+), 42 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 73f83a7e7a11..c0bb760ad473 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1117,48 +1117,19 @@ static int has_dir_name(struct index_state *istate,\n \t *\n \t * Compare the entry's full path with the last path in the index.\n \t */\n-\tif (istate->cache_nr > 0) {\n-\t\tcmp_last = strcmp_offset(name,\n-\t\t\tistate->cache[istate->cache_nr - 1]->name,\n-\t\t\t&len_eq_last);\n-\t\tif (cmp_last > 0) {\n-\t\t\tif (name[len_eq_last] != '/') {\n-\t\t\t\t/*\n-\t\t\t\t * The entry sorts AFTER the last one in the\n-\t\t\t\t * index.\n-\t\t\t\t *\n-\t\t\t\t * If there were a conflict with \"file\", then our\n-\t\t\t\t * name would start with \"file/\" and the last index\n-\t\t\t\t * entry would start with \"file\" but not \"file/\".\n-\t\t\t\t *\n-\t\t\t\t * The next character after common prefix is\n-\t\t\t\t * not '/', so there can be no conflict.\n-\t\t\t\t */\n-\t\t\t\treturn retval;\n-\t\t\t} else {\n-\t\t\t\t/*\n-\t\t\t\t * The entry sorts AFTER the last one in the\n-\t\t\t\t * index, and the next character after common\n-\t\t\t\t * prefix is '/'.\n-\t\t\t\t *\n-\t\t\t\t * Either the last index entry is a file in\n-\t\t\t\t * conflict with this entry, or it has a name\n-\t\t\t\t * which sorts between this entry and the\n-\t\t\t\t * potential conflicting file.\n-\t\t\t\t *\n-\t\t\t\t * In both cases, we fall through to the loop\n-\t\t\t\t * below and let the regular search code handle it.\n-\t\t\t\t */\n-\t\t\t}\n-\t\t} else if (cmp_last == 0) {\n-\t\t\t/*\n-\t\t\t * The entry exactly matches the last one in the\n-\t\t\t * index, but because of multiple stage and CE_REMOVE\n-\t\t\t * items, we fall through and let the regular search\n-\t\t\t * code handle it.\n-\t\t\t */\n-\t\t}\n-\t}\n+\tif (!istate->cache_nr)\n+\t\treturn 0;\n+\n+\tcmp_last = strcmp_offset(name,\n+\t\t\t\t istate->cache[istate->cache_nr - 1]->name,\n+\t\t\t\t &len_eq_last);\n+\tif (cmp_last > 0 && name[len_eq_last] != '/')\n+\t\t/*\n+\t\t * The entry sorts AFTER the last one in the\n+\t\t * index and their paths have no common prefix,\n+\t\t * so there cannot be a F/D conflict.\n+\t\t */\n+\t\treturn 0;\n \n \tfor (;;) {\n \t\tsize_t len;\n-- \ngitgitgadget\n\n"},{"id":"518128","messageId":"dff0a3ec886cdccf199bdc3881094ec0ab8b3bca.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 06/11] fetch: avoid unnecessary work when there is no current branch","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:44Z","receivedAt":"2025-05-15T13:11:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAs pointed out by CodeQL, `branch_get()` may return `NULL`, in which\ncase `branch_has_merge_config()` would return early, but we can even\navoid enumerating the refs prefixes in that case, saving even more CPU\ncycles.\n\nTechnically, we should enclose these two statements in an `if (branch)\n{...}` block, but the indentation is already quite deep, therefore I\nrefrained from doing that.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fetch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c1a1434c7096..40a0e8d24434 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1728,7 +1728,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tif (transport->remote->follow_remote_head != FOLLOW_REMOTE_NEVER)\n \t\t\t\tdo_set_head = 1;\n \t\t}\n-\t\tif (branch_has_merge_config(branch) &&\n+\t\tif (branch && branch_has_merge_config(branch) &&\n \t\t !strcmp(branch->remote_name, transport->remote->name)) {\n \t\t\tint i;\n \t\t\tfor (i = 0; i < branch->merge_nr; i++) {\n-- \ngitgitgadget\n\n"},{"id":"518129","messageId":"7d92e08b0c06a546ffd937c333b97d2fb6cd9817.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 07/11] Avoid redundant conditions","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:45Z","receivedAt":"2025-05-15T13:11:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhile `if (i <= 0) ... else if (i > 0) ...` is technically equivalent to\n`if (i <= 0) ... else ...`, the latter is vastly easier to read because\nit avoids writing out a condition that is unnecessary. Let's drop such\nunnecessary conditions.\n\nPointed out by CodeQL.\n\nSigned-off-by: Johannes Schindelin \n---\n help.c | 2 +-\n transport-helper.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 6ef90838f128..21b778707a6a 100644\n--- a/help.c\n+++ b/help.c\n@@ -214,7 +214,7 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n \t\telse if (cmp == 0) {\n \t\t\tei++;\n \t\t\tfree(cmds->names[ci++]);\n-\t\t} else if (cmp > 0)\n+\t\t} else\n \t\t\tei++;\n \t}\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 69391ee7d28e..0789e5bca532 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1437,7 +1437,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n \t\ttransfer_debug(\"%s EOF (with %i bytes in buffer)\",\n \t\t\tt->src_name, (int)t->bufuse);\n \t\tt->state = SSTATE_FLUSHING;\n-\t} else if (bytes > 0) {\n+\t} else {\n \t\tt->bufuse += bytes;\n \t\ttransfer_debug(\"Read %i bytes from %s (buffer now at %i)\",\n \t\t\t(int)bytes, t->src_name, (int)t->bufuse);\n-- \ngitgitgadget\n\n"},{"id":"518130","messageId":"a3f60183633eb17fb300309cf1a629234588418f.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 08/11] trace2: avoid \"futile conditional\"","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:46Z","receivedAt":"2025-05-15T13:12:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nCodeQL reports empty `if` blocks that only contain a comment as \"futile\nconditional\". The comment talks about potential plans to turn this into\na warning, but that seems not to have been necessary. Replace the entire\nconstruct with a concise comment.\n\nSigned-off-by: Johannes Schindelin \n---\n trace2/tr2_tmr.c | 24 +++++-------------------\n 1 file changed, 5 insertions(+), 19 deletions(-)\n\ndiff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c\nindex 51f564b07a40..038181ad9be0 100644\n--- a/trace2/tr2_tmr.c\n+++ b/trace2/tr2_tmr.c\n@@ -102,25 +102,11 @@ void tr2_update_final_timers(void)\n \t\tstruct tr2_timer *t_final = &final_timer_block.timer[tid];\n \t\tstruct tr2_timer *t = &ctx->timer_block.timer[tid];\n \n-\t\tif (t->recursion_count) {\n-\t\t\t/*\n-\t\t\t * The current thread is exiting with\n-\t\t\t * timer[tid] still running.\n-\t\t\t *\n-\t\t\t * Technically, this is a bug, but I'm going\n-\t\t\t * to ignore it.\n-\t\t\t *\n-\t\t\t * I don't think it is worth calling die()\n-\t\t\t * for. I don't think it is worth killing the\n-\t\t\t * process for this bookkeeping error. We\n-\t\t\t * might want to call warning(), but I'm going\n-\t\t\t * to wait on that.\n-\t\t\t *\n-\t\t\t * The downside here is that total_ns won't\n-\t\t\t * include the current open interval (now -\n-\t\t\t * start_ns). I can live with that.\n-\t\t\t */\n-\t\t}\n+\t\t/*\n+\t\t * `t->recursion_count` could technically be non-zero, which\n+\t\t * would constitute a bug. Reporting the bug would potentially\n+\t\t * cause an infinite recursion, though, so let's ignore it.\n+\t\t */\n \n \t\tif (!t->interval_count)\n \t\t\tcontinue; /* this timer was not used by this thread */\n-- \ngitgitgadget\n\n"},{"id":"518131","messageId":"077bcab206f5bfc9fc10a28ad7b726a6ec16c2bb.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 09/11] commit-graph: avoid using stale stack addresses","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:47Z","receivedAt":"2025-05-15T13:12:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe code is a bit too hard to reason about to fully assess whether the\n`fill_commit_graph_info()` function is called at all after\n`write_commit_graph()` returns (and hence the stack variable\n`topo_levels` goes out of context).\n\nLet's simply make sure that the stack address is no longer used at that\nstage, thereby making the code quite a bit easier to reason about.\n\nSigned-off-by: Johannes Schindelin \n---\n commit-graph.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9f0115dac9b5..d052c1bf15c5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2683,6 +2683,15 @@ cleanup:\n \toid_array_clear(&ctx.oids);\n \tclear_topo_level_slab(&topo_levels);\n \n+\tif (ctx.r->objects->commit_graph) {\n+\t\tstruct commit_graph *g = ctx.r->objects->commit_graph;\n+\n+\t\twhile (g) {\n+\t\t\tg->topo_levels = NULL;\n+\t\t\tg = g->base_graph;\n+\t\t}\n+\t}\n+\n \tfor (i = 0; i < ctx.num_commit_graphs_before; i++)\n \t\tfree(ctx.commit_graph_filenames_before[i]);\n \tfree(ctx.commit_graph_filenames_before);\n-- \ngitgitgadget\n\n"},{"id":"518132","messageId":"4dc3e2335afb42e5006ead7b9b18d33bdae7238f.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:48Z","receivedAt":"2025-05-15T13:12:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31)\ncode was introduced that assumes that an `sscanf()` call leaves its\noutput variables unchanged unless the return value indicates success.\n\nHowever, the POSIX documentation makes no such guarantee:\nhttps://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html\n\nSo let's make sure that the output variable `maxCreationToken` is\nalways well-defined.\n\nSigned-off-by: Johannes Schindelin \n---\n bundle-uri.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 96d2ba726d99..13a42f92387e 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -532,11 +532,13 @@ static int fetch_bundles_by_token(struct repository *r,\n \t */\n \tif (!repo_config_get_value(r,\n \t\t\t\t \"fetch.bundlecreationtoken\",\n-\t\t\t\t &creationTokenStr) &&\n-\t sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) == 1 &&\n-\t bundles.items[0]->creationToken <= maxCreationToken) {\n-\t\tfree(bundles.items);\n-\t\treturn 0;\n+\t\t\t\t &creationTokenStr)) {\n+\t\tif (sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) != 1)\n+\t\t\tmaxCreationToken = 0;\n+\t\tif (bundles.items[0]->creationToken <= maxCreationToken) {\n+\t\t\tfree(bundles.items);\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n-- \ngitgitgadget\n\n"},{"id":"518133","messageId":"7a54005bd26ac17cb6d99a2e18932f97575d4aca.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 11/11] sequencer: stop pretending that an assignment is a condition","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:49Z","receivedAt":"2025-05-15T13:12:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 3e81bccdf3 (sequencer: factor out todo command name parsing,\n2019-06-27), a `return` statement was introduced that basically was a\nlong sequence of conditions, combined with `&&`, except for the last\ncondition which is not really a condition but an assignment.\n\nThe point of this construct was to return 1 (i.e. `true`) from the\nfunction if all of those conditions held true, and also assign the `bol`\npointer to the end of the parsed command.\n\nSome static analyzers are really unhappy about such constructs. And\nhuman readers are at least puzzled, if not confused, by seeing a single\n`=` inside a chain of conditions where they would have expected to see\n`==` instead and, based on experience, immediately suspect a typo.\n\nLet's help all of this by turning this into the more verbose, more\nreadable form of an `if` construct that both assigns the pointer as well\nas returns 1 if all of the conditions hold true.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b5c4043757e9..e5e3bc6fa5ea 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2600,9 +2600,12 @@ static int is_command(enum todo_command command, const char **bol)\n \tconst char nick = todo_command_info[command].c;\n \tconst char *p = *bol;\n \n-\treturn (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n-\t\t(*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n-\t\t(*bol = p);\n+\tif ((skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n+\t (*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p)) {\n+\t\t*bol = p;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n }\n \n static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n-- \ngitgitgadget\n"},{"id":"518134","messageId":"767b1e74fc70d78c7e3c84a2dc5eb7b607252f4f.1747314709.git.gitgitgadget@gmail.com","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"[PATCH 01/11] commit: simplify code","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-15T13:11:39Z","receivedAt":"2025-05-15T13:12:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe difference of two unsigned integers is defined to be unsigned, and\ntherefore it is misleading to check whether it is greater than zero\n(instead, the more natural way would be to check whether the difference\nis zero or not).\n\nLet's instead avoid the subtraction altogether, and compare the two\noperands directly, which makes the code more obvious as a side effect.\n\nPointed out by CodeQL's rule with the ID\n`cpp/unsigned-difference-expression-compared-zero`.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66bd91fd523d..fba0dded64a7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1022,7 +1022,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\tfor (i = 0; i < the_repository->index->cache_nr; i++)\n \t\t\t\tif (ce_intent_to_add(the_repository->index->cache[i]))\n \t\t\t\t\tita_nr++;\n-\t\t\tcommittable = the_repository->index->cache_nr - ita_nr > 0;\n+\t\t\tcommittable = the_repository->index->cache_nr > ita_nr;\n \t\t} else {\n \t\t\t/*\n \t\t\t * Unless the user did explicitly request a submodule\n-- \ngitgitgadget\n\n"},{"id":"518161","messageId":"xmqqplg9zozn.fsf@gitster.g","threadId":"63465","inReplyTo":"7a54005bd26ac17cb6d99a2e18932f97575d4aca.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] sequencer: stop pretending that an assignment is a condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T18:51:40Z","receivedAt":"2025-05-15T18:51:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 3e81bccdf3 (sequencer: factor out todo command name parsing,\n> 2019-06-27), a `return` statement was introduced that basically was a\n> long sequence of conditions, combined with `&&`, except for the last\n> condition which is not really a condition but an assignment.\n>\n> The point of this construct was to return 1 (i.e. `true`) from the\n> function if all of those conditions held true, and also assign the `bol`\n> pointer to the end of the parsed command.\n\nTrue, as the value of 'p' cannot be NULL at that point where it is\nstored to the pointer variable bol points at. The second paragraph\nabove does convey what the long expression really wants to achieve.\n\n> Some static analyzers are really unhappy about such constructs. And\n> human readers are at least puzzled, if not confused, by seeing a single\n> `=` inside a chain of conditions where they would have expected to see\n> `==` instead and, based on experience, immediately suspect a typo.\n\nYes. Good thing to get rid of.\n\n>\n> Let's help all of this by turning this into the more verbose, more\n> readable form of an `if` construct that both assigns the pointer as well\n> as returns 1 if all of the conditions hold true.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index b5c4043757e9..e5e3bc6fa5ea 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2600,9 +2600,12 @@ static int is_command(enum todo_command command, const char **bol)\n> \tconst char nick = todo_command_info[command].c;\n> \tconst char *p = *bol;\n> \n> -\treturn (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n> -\t\t(*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n> -\t\t(*bol = p);\n> +\tif ((skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n> +\t (*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p)) {\n> +\t\t*bol = p;\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> }\n\nPerfect. That's quite a natural way to express the intention.\n\n\n\n> \n> static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n"},{"id":"518165","messageId":"xmqqldqxznmi.fsf@gitster.g","threadId":"63465","inReplyTo":"4dc3e2335afb42e5006ead7b9b18d33bdae7238f.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T19:21:09Z","receivedAt":"2025-05-15T19:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31)\n> code was introduced that assumes that an `sscanf()` call leaves its\n> output variables unchanged unless the return value indicates success.\n>\n> However, the POSIX documentation makes no such guarantee:\n> https://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html\n>\n> So let's make sure that the output variable `maxCreationToken` is\n> always well-defined.\n>\n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 96d2ba726d99..13a42f92387e 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -532,11 +532,13 @@ static int fetch_bundles_by_token(struct repository *r,\n> \t */\n> \tif (!repo_config_get_value(r,\n> \t\t\t\t \"fetch.bundlecreationtoken\",\n> -\t\t\t\t &creationTokenStr) &&\n> -\t sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) == 1 &&\n> -\t bundles.items[0]->creationToken <= maxCreationToken) {\n> -\t\tfree(bundles.items);\n> -\t\treturn 0;\n\nThe original said \"if we successfully parsed and the value of the\ntoken is larger than the token, we are done\", which is probably OK,\nbut the problem is if we were fed garbage and failed to parse it, we\nwould have smudged maxCreationToken to some unknown value, and the\ncode path that follows here, which assumes that maxCreationToken is\nleft as initialized to 0 will be broken.\n\nSo the problem is real, but I find the rewritten form a bit hard to\nfollow. Namely, when sscanf() failed to grab maxCreationToken, we\nnever compared it with bundles.items[0]->creationToken, which makes\nperfect sense to me, but now...\n\n> +\t\t\t\t &creationTokenStr)) {\n> +\t\tif (sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) != 1)\n> +\t\t\tmaxCreationToken = 0;\n> +\t\tif (bundles.items[0]->creationToken <= maxCreationToken) {\n> +\t\t\tfree(bundles.items);\n> +\t\t\treturn 0;\n> +\t\t}\n\n... the updated code does use the just-assigned-because-we-failed-to-parse\nvalue 0 in comparison.\n\nI have to wonder if the attached patch is simpler to reason about,\nmore in line with what the original wanted to do, and more correct?\n\nWhen we fail to grab the configuration, or when the value we grabbed\nfrom the configuration does not parse, then we reset\nmaxCreationToken to 0, but otherwise we have a valid\nmaxCreationToken so use it to see if we should return early.\n\nThe cases we do not return early here are either (1) we did not have\nusable configured value, in which case maxCreationToken is set to 0\nbefore reaching the loop after this code, or (2) the value of\nmaxCreationToken we grabbed from the configuration is smaller than\nthe creationToken in the bundle list, in which case that value is\nused when entering the loop.\n\nThanks.\n\n bundle-uri.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git c/bundle-uri.c w/bundle-uri.c\nindex f3579e228e..13a43f8e32 100644\n--- c/bundle-uri.c\n+++ w/bundle-uri.c\n@@ -531,11 +531,11 @@ static int fetch_bundles_by_token(struct repository *r,\n \t * is not strictly smaller than the maximum creation token in the\n \t * bundle list, then do not download any bundles.\n \t */\n-\tif (!repo_config_get_value(r,\n-\t\t\t\t \"fetch.bundlecreationtoken\",\n-\t\t\t\t &creationTokenStr) &&\n-\t sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) == 1 &&\n-\t bundles.items[0]->creationToken <= maxCreationToken) {\n+\tif (repo_config_get_value(r, \"fetch.bundlecreationtoken\",\n+\t\t\t\t &creationTokenStr) ||\n+\t sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) != 1)\n+\t\tmaxCreationToken = 0;\n+\telse if (bundles.items[0]->creationToken <= maxCreationToken) {\n \t\tfree(bundles.items);\n \t\treturn 0;\n \t}\n"},{"id":"518167","messageId":"20250515194003.GB3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"c66eaee64ad5484120f1905ce5501fd8b75b2266.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 02/11] fetch: carefully clear local variable's address after use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T19:40:03Z","receivedAt":"2025-05-15T19:40:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:40PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> As pointed out by CodeQL, it is a potentially dangerous practice to\n> store local variables' addresses in non-local structs. Yet this is\n> exactly what happens with the `acked_commits` attribute that is used in\n> `cmd_fetch()`: The pointer to a local variable is assigned to it.\n> \n> Now, it is Git's convention that `cmd_*()` functions are essentially\n> only returning just before exiting the process, therefore there is\n> little danger that this attribute is used after the code flow returns\n> from that function.\n\nI was going to say: the real sin here is using a global variable in the\nfirst place, without which gtransport would not survive outside of\ncmd_fetch(). But the issue is even worse than that. The acked_commits\nvariable is inside a conditional block, so the address is stale for the\nrest of cmd_fetch(), too!\n\nIt doesn't look like we ever examine it after that, but it's hard to\ntrace, since it's a global. ;)\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index cda6eaf1fd6e..c1a1434c7096 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2560,6 +2560,7 @@ int cmd_fetch(int argc,\n> \t\tif (server_options.nr)\n> \t\t\tgtransport->server_options = &server_options;\n> \t\tresult = transport_fetch_refs(gtransport, NULL);\n> +\t\tgtransport->smart_options->acked_commits = NULL;\n> \n> \t\toidset_iter_init(&acked_commits, &iter);\n> \t\twhile ((oid = oidset_iter_next(&iter)))\n\nHere you unset it within that conditional block, which is the right\nspot. Looks good.\n\n-Peff\n"},{"id":"518168","messageId":"20250515194806.GC3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"767b1e74fc70d78c7e3c84a2dc5eb7b607252f4f.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/11] commit: simplify code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T19:48:06Z","receivedAt":"2025-05-15T19:48:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:39PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 66bd91fd523d..fba0dded64a7 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1022,7 +1022,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\t\tfor (i = 0; i < the_repository->index->cache_nr; i++)\n> \t\t\t\tif (ce_intent_to_add(the_repository->index->cache[i]))\n> \t\t\t\t\tita_nr++;\n> -\t\t\tcommittable = the_repository->index->cache_nr - ita_nr > 0;\n> +\t\t\tcommittable = the_repository->index->cache_nr > ita_nr;\n\nI guess it is not possible for ita_nr to be greater than cache_nr, since\nwe are counting up entries in the loop above. If ita_nr were greater,\nthe original would wrap around and set committable to true, but yours\nwould not.\n\nSo really, I think the original was equivalent to:\n\n committable = cache_nr != ita_nr;\n\nbut I think \">\" probably expresses the intent better (we want to know if\nthere are any non-ita entries). Though in that case I'd think:\n\n committable = 0;\n for (i = 0; i < cache_nr; i++) {\n\tif (!ce_intent_to_add(...) {\n\t\tcommittable = 1;\n\t\tbreak;\n\t}\n }\n\nwould be the most clear, since we do not otherwise care about the actual\nnumber of ita entries. And lets us break out of the loop early.\n\nI dunno if it is worth refactoring further, though. Your patch does the\ncorrect thing and fixes the codeql complaint (which I do think is a\nfalse positive, because ita_nr must be less than cache_nr).\n\n-Peff\n"},{"id":"518170","messageId":"20250515195428.GD3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"5a3a8880a68f8c69c2af39d2e32ebb56eb5fa483.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 03/11] commit-graph: avoid malloc'ing a local variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T19:54:28Z","receivedAt":"2025-05-15T19:54:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:41PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> We do need a context to write the commit graph, but that context is only\n> needed during the life time of `commit_graph_write()`, therefore it can\n> easily be a stack variable.\n\nYay. I am in favor of using stack variables when possible as a general\nrule.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 6394752b0b08..9f0115dac9b5 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2509,7 +2509,17 @@ int write_commit_graph(struct object_directory *odb,\n> \t\t const struct commit_graph_opts *opts)\n> {\n> \tstruct repository *r = the_repository;\n> -\tstruct write_commit_graph_context *ctx;\n> +\tstruct write_commit_graph_context ctx = {\n> +\t\t.r = r,\n> +\t\t.odb = odb,\n> +\t\t.append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0,\n> +\t\t.report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0,\n> +\t\t.split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0,\n> +\t\t.opts = opts,\n> +\t\t.total_bloom_filter_data_size = 0,\n> +\t\t.write_generation_data = (get_configured_generation_version(r) == 2),\n> +\t\t.num_generation_data_overflows = 0,\n> +\t};\n> \tuint32_t i;\n> \tint res = 0;\n> \tint replace = 0;\n> @@ -2531,17 +2541,6 @@ int write_commit_graph(struct object_directory *odb,\n> \t\treturn 0;\n> \t}\n> \n> -\tCALLOC_ARRAY(ctx, 1);\n> -\tctx->r = r;\n> -\tctx->odb = odb;\n> -\tctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;\n> -\tctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;\n> -\tctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;\n> -\tctx->opts = opts;\n> -\tctx->total_bloom_filter_data_size = 0;\n> -\tctx->write_generation_data = (get_configured_generation_version(r) == 2);\n> -\tctx->num_generation_data_overflows = 0;\n\nOK, this moves the initialization to the top of the function. So to\nreview this for correctness, we must make sure that we do not change the\nvalues of any of those variables between the two spots (i.e., in the\ndiff context that is omitted).\n\nMost of it looks fine. Our call to get_configured_generation_version()\nnow happens earlier, before the call to prepare_repo_settings(). I think\nthat is OK, because the former calls repo_config_get_int() directly. It\ndoes seem like a potential maintenance problem if that call is ever\nrolled into prepare_repo_settings().\n\nSo maybe OK, but the smaller change would be to just replace the calloc\nwith a memset(), and s/->/./ on the subsequent lines.\n\n-Peff\n"},{"id":"518171","messageId":"20250515195536.GE3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"8d712a0ebc8d5161bdc80bdd7f4d05a967c66a09.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 04/11] upload-pack: rename `enum` to reflect the operation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T19:55:36Z","receivedAt":"2025-05-15T19:55:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:42PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> While 3145ea957d (upload-pack: introduce fetch server command,\n> 2018-03-15) added support for the `fetch` command, from the server's\n> point of view it is an upload, and hence the `enum` should really be\n> called `upload_state` instead of `fetch_state`. Likewise, rename its\n> values.\n> \n> This also helps unconfuse CodeQL which would otherwise be at sixes or\n> sevens about having _two_ non-local definitions of the same `enum` with\n> the same values.\n\nIt unconfuses me, too. Nice change.\n\n-Peff\n"},{"id":"518173","messageId":"20250515200435.GF3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"80422a5770ded04993c73c657b363ddad45e2f4a.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 05/11] has_dir_name(): make code more obvious","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:04:35Z","receivedAt":"2025-05-15T20:04:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:43PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> One thing that might be non-obvious to readers (or to analyzers like\n> CodeQL) is that the function essentially does nothing when the Git index\n> is empty, and in particular that it does not look at the value of\n> `len_eq_last` (which would be uninitialized at that point).\n> \n> Let's make this much easier to understand, by returning early if the Git\n> index is empty, and by avoiding empty `else` blocks.\n\nOK, so we return early, skipping not only what's in the conditional\nthat you're touching here, but also the \"for(;;)\" loop below.\n\nAnd in that one, we'll look for the next slash (and break if none).\nWe'll check the name up to that stage via index_name_stage_pos(). And\nobviously that will not find a match if there are no index entries. So\nwe'd do nothing and loop again, looking for the next slash, until we\neventually hit the end.\n\nSo yeah, I agree if there are no index entries we can bail immediately.\n\n> This commit changes indentation and is hence best viewed using\n> `--ignore-space-change`.\n\nYeah. I was puzzled at first by the amount of dropped code, but they are\nall comments that say \"fall through to the code below\".\n\nSo I think the change here is correct. We are losing some comments that\ncould be helpful, but I'm not familiar enough with those code to say\nwhether they would be. Just reading what you've left makes sense to me\non its own.\n\n-Peff\n"},{"id":"518174","messageId":"20250515201145.GG3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"dff0a3ec886cdccf199bdc3881094ec0ab8b3bca.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 06/11] fetch: avoid unnecessary work when there is no current branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:11:45Z","receivedAt":"2025-05-15T20:11:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:44PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> As pointed out by CodeQL, `branch_get()` may return `NULL`, in which\n> case `branch_has_merge_config()` would return early, but we can even\n> avoid enumerating the refs prefixes in that case, saving even more CPU\n> cycles.\n\nI am not sure how this patch changes anything with respect to CPU. If\nbranch is NULL, then branch_has_merge_config(branch) will always return\nfalse.\n\nI think this is just an issue that CodeQL is not looking inside\nbranch_has_merge_config(), and thus does not realize we will never hit\nthe rest of the short-circuit conditional (let alone the body) in that\ncase?\n\nStill may be worth dealing with, but it makes me a little sad to have to\nadd an extra redundant check (one place is not a big deal, but as a\ngeneral pattern).\n\n-Peff\n"},{"id":"518175","messageId":"20250515201307.GH3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"7d92e08b0c06a546ffd937c333b97d2fb6cd9817.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 07/11] Avoid redundant conditions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:13:07Z","receivedAt":"2025-05-15T20:13:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:45PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> While `if (i <= 0) ... else if (i > 0) ...` is technically equivalent to\n> `if (i <= 0) ... else ...`, the latter is vastly easier to read because\n> it avoids writing out a condition that is unnecessary. Let's drop such\n> unnecessary conditions.\n> \n> Pointed out by CodeQL.\n\nYeah, I'd agree that it is easier (otherwise, you are left wondering if\nthere is an \"else\" case you are missing).\n\n> help.c | 2 +-\n> transport-helper.c | 2 +-\n\nBoth spots look good to me.\n\n-Peff\n"},{"id":"518176","messageId":"20250515201625.GI3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"a3f60183633eb17fb300309cf1a629234588418f.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 08/11] trace2: avoid \"futile conditional\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:16:25Z","receivedAt":"2025-05-15T20:16:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:46PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> CodeQL reports empty `if` blocks that only contain a comment as \"futile\n> conditional\". The comment talks about potential plans to turn this into\n> a warning, but that seems not to have been necessary. Replace the entire\n> construct with a concise comment.\n\nOK...\n\n> -\t\tif (t->recursion_count) {\n> -\t\t\t/*\n> -\t\t\t * The current thread is exiting with\n> -\t\t\t * timer[tid] still running.\n> -\t\t\t *\n> -\t\t\t * Technically, this is a bug, but I'm going\n> -\t\t\t * to ignore it.\n> -\t\t\t *\n> -\t\t\t * I don't think it is worth calling die()\n> -\t\t\t * for. I don't think it is worth killing the\n> -\t\t\t * process for this bookkeeping error. We\n> -\t\t\t * might want to call warning(), but I'm going\n> -\t\t\t * to wait on that.\n> -\t\t\t *\n> -\t\t\t * The downside here is that total_ns won't\n> -\t\t\t * include the current open interval (now -\n> -\t\t\t * start_ns). I can live with that.\n> -\t\t\t */\n> -\t\t}\n> +\t\t/*\n> +\t\t * `t->recursion_count` could technically be non-zero, which\n> +\t\t * would constitute a bug. Reporting the bug would potentially\n> +\t\t * cause an infinite recursion, though, so let's ignore it.\n> +\t\t */\n\nThe original doesn't talk about infinite recursion at all, though I can\nwell believe that would be the case, having run into trace->die->trace\ntypes of bugs before. Did you trace out the actual path of recursion? If\nso, it might be worth summarizing it.\n\nObviously the code change itself cannot hurt anything, as it was a noop.\n\n-Peff\n"},{"id":"518178","messageId":"20250515201943.GJ3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"077bcab206f5bfc9fc10a28ad7b726a6ec16c2bb.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 09/11] commit-graph: avoid using stale stack addresses","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:19:43Z","receivedAt":"2025-05-15T20:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:47PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> The code is a bit too hard to reason about to fully assess whether the\n> `fill_commit_graph_info()` function is called at all after\n> `write_commit_graph()` returns (and hence the stack variable\n> `topo_levels` goes out of context).\n> \n> Let's simply make sure that the stack address is no longer used at that\n> stage, thereby making the code quite a bit easier to reason about.\n\nYep, I think this is a good practice in general. If the topo_levels\nmember is never used outside of writing, I wonder if it could live in a\nseparate data structure. But that is a much bigger refactor that I don't\nthink we need to tackle here.\n\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 9f0115dac9b5..d052c1bf15c5 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -2683,6 +2683,15 @@ cleanup:\n> \toid_array_clear(&ctx.oids);\n> \tclear_topo_level_slab(&topo_levels);\n> \n> +\tif (ctx.r->objects->commit_graph) {\n> +\t\tstruct commit_graph *g = ctx.r->objects->commit_graph;\n> +\n> +\t\twhile (g) {\n> +\t\t\tg->topo_levels = NULL;\n> +\t\t\tg = g->base_graph;\n> +\t\t}\n> +\t}\n\nThis just clears the pointers to the local variable. Looks good.\n\n-Peff\n"},{"id":"518181","messageId":"20250515202502.GK3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"4dc3e2335afb42e5006ead7b9b18d33bdae7238f.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:25:02Z","receivedAt":"2025-05-15T20:25:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:48PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> In c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31)\n> code was introduced that assumes that an `sscanf()` call leaves its\n> output variables unchanged unless the return value indicates success.\n> \n> However, the POSIX documentation makes no such guarantee:\n> https://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html\n> \n> So let's make sure that the output variable `maxCreationToken` is\n> always well-defined.\n\nDefinitely an issue, but...why are we using sscanf() at all?\n\nWouldn't strtoul() be the usual thing in our code base? Or even just\nrepo_config_get_ulong()? The behavior of the latter would differ in that\nwe'd complain about a garbage value in fetch.bundlecreationtoken, but\nwouldn't that be a good thing?\n\n-Peff\n"},{"id":"518182","messageId":"20250515202607.GL3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"7a54005bd26ac17cb6d99a2e18932f97575d4aca.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] sequencer: stop pretending that an assignment is a condition","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:26:07Z","receivedAt":"2025-05-15T20:26:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:49PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Let's help all of this by turning this into the more verbose, more\n> readable form of an `if` construct that both assigns the pointer as well\n> as returns 1 if all of the conditions hold true.\n\nI see Junio already reviewed this one, but just so I can say I read all\nof the patches: yes, I also agree the result is easier to understand. :)\n\n-Peff\n"},{"id":"518184","messageId":"20250515202652.GM3320240@coredump.intra.peff.net","threadId":"63465","inReplyTo":"pull.1891.git.1747314709.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/11] CodeQL-inspired fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:26:52Z","receivedAt":"2025-05-15T20:26:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:11:38PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> CodeQL [https://codeql.github.com/] pointed out a couple of issues, which\n> are addressed in this patch series.\n> \n> Johannes Schindelin (11):\n> commit: simplify code\n> fetch: carefully clear local variable's address after use\n> commit-graph: avoid malloc'ing a local variable\n> upload-pack: rename `enum` to reflect the operation\n> has_dir_name(): make code more obvious\n> fetch: avoid unnecessary work when there is no current branch\n> Avoid redundant conditions\n> trace2: avoid \"futile conditional\"\n> commit-graph: avoid using stale stack addresses\n> bundle-uri: avoid using undefined output of `sscanf()`\n> sequencer: stop pretending that an assignment is a condition\n\nI read through all of these and didn't find anything incorrect. I did\nleave a few comments that might or might not be worth following up on.\nThanks for fixing these.\n\n-Peff\n"},{"id":"518186","messageId":"xmqq4ixlzk43.fsf@gitster.g","threadId":"63465","inReplyTo":"20250515194806.GC3320240@coredump.intra.peff.net","subject":"Re: [PATCH 01/11] commit: simplify code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T20:37:00Z","receivedAt":"2025-05-15T20:37:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... there are any non-ita entries). Though in that case I'd think:\n>\n> committable = 0;\n> for (i = 0; i < cache_nr; i++) {\n> \tif (!ce_intent_to_add(...) {\n> \t\tcommittable = 1;\n> \t\tbreak;\n> \t}\n> }\n>\n> would be the most clear, since we do not otherwise care about the actual\n> number of ita entries. And lets us break out of the loop early.\n\nExactly. If you focus on the warning too narrowly, the minimal\nchange in the original patch does look OK, but in the original (even\nbefore Dscho's patch, that is) the intent is unclear, as opposed to\nwhat you showed above. And the update to squelch false positive\ndoes not improve the clarity of the logic as the above rewrite does.\n\nThanks.\n"},{"id":"518187","messageId":"20250515204957.GA3334771@coredump.intra.peff.net","threadId":"63465","inReplyTo":"xmqq4ixlzk43.fsf@gitster.g","subject":"Re: [PATCH 01/11] commit: simplify code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T20:49:57Z","receivedAt":"2025-05-15T20:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 01:37:00PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... there are any non-ita entries). Though in that case I'd think:\n> >\n> > committable = 0;\n> > for (i = 0; i < cache_nr; i++) {\n> > \tif (!ce_intent_to_add(...) {\n> > \t\tcommittable = 1;\n> > \t\tbreak;\n> > \t}\n> > }\n> >\n> > would be the most clear, since we do not otherwise care about the actual\n> > number of ita entries. And lets us break out of the loop early.\n> \n> Exactly. If you focus on the warning too narrowly, the minimal\n> change in the original patch does look OK, but in the original (even\n> before Dscho's patch, that is) the intent is unclear, as opposed to\n> what you showed above. And the update to squelch false positive\n> does not improve the clarity of the logic as the above rewrite does.\n\nOK. If we do want to refactor, I think pulling it into a separate\nfunction is the most descriptive, like:\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66bd91fd52..a8d43d223d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -740,6 +740,15 @@ static void change_data_free(void *util, const char *str UNUSED)\n \tfree(d);\n }\n \n+static int has_non_ita_entries(struct index_state *index)\n+{\n+\tint i;\n+\tfor (i = 0; i < index->cache_nr; i++)\n+\t\tif (!ce_intent_to_add(index->cache[i]))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t struct commit *current_head,\n \t\t\t struct wt_status *s,\n@@ -1015,14 +1024,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\tparent = \"HEAD^1\";\n \n \t\tif (repo_get_oid(the_repository, parent, &oid)) {\n-\t\t\tint i, ita_nr = 0;\n-\n \t\t\t/* TODO: audit for interaction with sparse-index. */\n \t\t\tensure_full_index(the_repository->index);\n-\t\t\tfor (i = 0; i < the_repository->index->cache_nr; i++)\n-\t\t\t\tif (ce_intent_to_add(the_repository->index->cache[i]))\n-\t\t\t\t\tita_nr++;\n-\t\t\tcommittable = the_repository->index->cache_nr - ita_nr > 0;\n+\t\t\tcommittable =\n+\t\t\t\thas_non_ita_entries(the_repository->index);\n \t\t} else {\n \t\t\t/*\n \t\t\t * Unless the user did explicitly request a submodule\n\n-Peff\n"},{"id":"518188","messageId":"xmqqzffdy4j9.fsf@gitster.g","threadId":"63465","inReplyTo":"20250515202652.GM3320240@coredump.intra.peff.net","subject":"Re: [PATCH 00/11] CodeQL-inspired fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T20:58:50Z","receivedAt":"2025-05-15T20:58:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 15, 2025 at 01:11:38PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n>> CodeQL [https://codeql.github.com/] pointed out a couple of issues, which\n>> are addressed in this patch series.\n>> \n>> Johannes Schindelin (11):\n>> commit: simplify code\n>> fetch: carefully clear local variable's address after use\n>> commit-graph: avoid malloc'ing a local variable\n>> upload-pack: rename `enum` to reflect the operation\n>> has_dir_name(): make code more obvious\n>> fetch: avoid unnecessary work when there is no current branch\n>> Avoid redundant conditions\n>> trace2: avoid \"futile conditional\"\n>> commit-graph: avoid using stale stack addresses\n>> bundle-uri: avoid using undefined output of `sscanf()`\n>> sequencer: stop pretending that an assignment is a condition\n>\n> I read through all of these and didn't find anything incorrect. I did\n> leave a few comments that might or might not be worth following up on.\n> Thanks for fixing these.\n\nYup, I also looked at them and didn't see any incorrect updates.\n"},{"id":"518189","messageId":"xmqqv7q1y2ld.fsf@gitster.g","threadId":"63465","inReplyTo":"20250515195428.GD3320240@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] commit-graph: avoid malloc'ing a local variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T21:40:46Z","receivedAt":"2025-05-15T21:40:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So maybe OK, but the smaller change would be to just replace the calloc\n> with a memset(), and s/->/./ on the subsequent lines.\n\nTrue, and it would be a bit easier to merge with other topics in\nflight. The .oid member and parameter are both renamed IIUC.\n"},{"id":"518230","messageId":"694d8e62-486b-43d4-966c-4fa1e5a95061@crinan.ddns.net","threadId":"63465","inReplyTo":"20250515202502.GK3320240@coredump.intra.peff.net","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-16T10:11:59Z","receivedAt":"2025-05-16T10:12:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/05/2025 21:25, Jeff King wrote:\n> On Thu, May 15, 2025 at 01:11:48PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> \n>> In c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31)\n>> code was introduced that assumes that an `sscanf()` call leaves its\n>> output variables unchanged unless the return value indicates success.\n>>\n>> However, the POSIX documentation makes no such guarantee:\n>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html\n>>\n>> So let's make sure that the output variable `maxCreationToken` is\n>> always well-defined.\n> \n> Definitely an issue, but...why are we using sscanf() at all?\n> \n> Wouldn't strtoul() be the usual thing in our code base? Or even just\n> repo_config_get_ulong()? The behavior of the latter would differ in that\n> we'd complain about a garbage value in fetch.bundlecreationtoken, but\n> wouldn't that be a good thing?\n\nI had a similar thought, though to make sure that we parsed 64 bit \nvalues correctly on windows so we'd need something based on strtoumax() \nI think. There is another call to sscanf() in this file which the \nanalyzer does not complain about because it stores the result in a local \nvariable that is not used if the call to sscanf() fails. We should stop \nusing sscanf() there as well. I wonder if we should add something about \nnot using sscanf() to our coding guidelines. Apart from this file the \nonly other use of sscanf() is in a test helper which doesn't seem so bad \nthough if we removed that we could add sscanf() to banned.h.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"518231","messageId":"8641d540-516f-4dd2-bafa-cbc2568b5281@crinan.ddns.net","threadId":"63465","inReplyTo":"7a54005bd26ac17cb6d99a2e18932f97575d4aca.1747314709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] sequencer: stop pretending that an assignment is a condition","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-16T10:13:04Z","receivedAt":"2025-05-16T10:13:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nThanks for cleaning this up - I'm not sure why I didn't just write \nsomething like this in the first place.\n\nBest Wishes\n\nPhillip\n\nOn 15/05/2025 14:11, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In 3e81bccdf3 (sequencer: factor out todo command name parsing,\n> 2019-06-27), a `return` statement was introduced that basically was a\n> long sequence of conditions, combined with `&&`, except for the last\n> condition which is not really a condition but an assignment.\n> \n> The point of this construct was to return 1 (i.e. `true`) from the\n> function if all of those conditions held true, and also assign the `bol`\n> pointer to the end of the parsed command.\n> \n> Some static analyzers are really unhappy about such constructs. And\n> human readers are at least puzzled, if not confused, by seeing a single\n> `=` inside a chain of conditions where they would have expected to see\n> `==` instead and, based on experience, immediately suspect a typo.\n> \n> Let's help all of this by turning this into the more verbose, more\n> readable form of an `if` construct that both assigns the pointer as well\n> as returns 1 if all of the conditions hold true.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index b5c4043757e9..e5e3bc6fa5ea 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2600,9 +2600,12 @@ static int is_command(enum todo_command command, const char **bol)\n> \tconst char nick = todo_command_info[command].c;\n> \tconst char *p = *bol;\n> \n> -\treturn (skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n> -\t\t(*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p) &&\n> -\t\t(*bol = p);\n> +\tif ((skip_prefix(p, str, &p) || (nick && *p++ == nick)) &&\n> +\t (*p == ' ' || *p == '\\t' || *p == '\\n' || *p == '\\r' || !*p)) {\n> +\t\t*bol = p;\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> }\n> \n> static int check_label_or_ref_arg(enum todo_command command, const char *arg)\n\n"},{"id":"518238","messageId":"20250516134054.217202-1-phillip.wood123@gmail.com","threadId":"63465","inReplyTo":"694d8e62-486b-43d4-966c-4fa1e5a95061@crinan.ddns.net","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-16T13:40:54Z","receivedAt":"2025-05-16T13:41:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/05/2025 11:11, Phillip Wood wrote:\n\n> I had a similar thought, though to make sure that we parsed 64 bit \n> values correctly on windows so we'd need something based on strtoumax() \n> I think.\n\nPerhaps something like the diff below which adds strtoul_u64() in a\nsimilar vein to strtoul_ui(). I think it's debatable whether we really\nwant to skip leading whitespace so we could perhaps tighten things up\nby replacing \"if (strchr(s, '-'))\" with \"if (!isdigit(*s))\" though\nthat would mean this function would behave slightly differently to\nstrtoul_ui().\n\nBest Wishes\n\nPhillip\n\n---- >8 ----\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 96d2ba726d9..9dff7a1c09d 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -214,7 +214,7 @@ static int bundle_list_update(const char *key, const char *value,\n \t}\n \n \tif (!strcmp(subkey, \"creationtoken\")) {\n-\t\tif (sscanf(value, \"%\"PRIu64, &bundle->creationToken) != 1)\n+\t\tif (strtoul_u64(value, 10, &bundle->creationToken))\n \t\t\twarning(_(\"could not parse bundle list key %s with value '%s'\"),\n \t\t\t\t\"creationToken\", value);\n \t\treturn 0;\n@@ -533,7 +533,7 @@ static int fetch_bundles_by_token(struct repository *r,\n \tif (!repo_config_get_value(r,\n \t\t\t\t \"fetch.bundlecreationtoken\",\n \t\t\t\t &creationTokenStr) &&\n-\t sscanf(creationTokenStr, \"%\"PRIu64, &maxCreationToken) == 1 &&\n+\t strtoul_u64(creationTokenStr,10, &maxCreationToken) &&\n \t bundles.items[0]->creationToken <= maxCreationToken) {\n \t\tfree(bundles.items);\n \t\treturn 0;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 36b9577c8d4..d34d07fce1e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -939,6 +939,22 @@ static inline int strtol_i(char const *s, int base, int *result)\n \treturn 0;\n }\n \n+static inline int strtoul_u64(char const *s, int base, uint64_t *result)\n+{\n+\tuintmax_t ul;\n+\tchar *p;\n+\n+\terrno = 0;\n+\t/* negative values would be accepted by strtoumax */\n+\tif (strchr(s, '-'))\n+\t\treturn -1;\n+\tul = strtoumax(s, &p, base);\n+\tif (errno || *p || p == s || (uint64_t) ul != ul)\n+\t\treturn -1;\n+\t*result = ul;\n+\treturn 0;\n+}\n+\n #ifndef REG_STARTEND\n #error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n #endif\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"518257","messageId":"20250516154250.GA26009@coredump.intra.peff.net","threadId":"63465","inReplyTo":"20250516134054.217202-1-phillip.wood123@gmail.com","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-16T15:42:50Z","receivedAt":"2025-05-16T15:42:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 16, 2025 at 02:40:54PM +0100, Phillip Wood wrote:\n\n> On 16/05/2025 11:11, Phillip Wood wrote:\n> \n> > I had a similar thought, though to make sure that we parsed 64 bit \n> > values correctly on windows so we'd need something based on strtoumax() \n> > I think.\n> \n> Perhaps something like the diff below which adds strtoul_u64() in a\n> similar vein to strtoul_ui(). I think it's debatable whether we really\n> want to skip leading whitespace so we could perhaps tighten things up\n> by replacing \"if (strchr(s, '-'))\" with \"if (!isdigit(*s))\" though\n> that would mean this function would behave slightly differently to\n> strtoul_ui().\n\nIt feels like we would had to have dealt with this before for other\nlarge values. But poking around at a few obvious suspects (e.g.,\npackSizeLimit), it looks like they are all constrained to \"unsigned\nlong\".\n\nSo yeah, we probably do need something new. IMHO we should probably have\nrepo_config_get_u64() or similar (with the appropriate underlying\nhelpers as well) as use it here. But I am happy with any solution.\n\nAnd I do agree that we should consider banning *scanf(). With numeric\nplaceholders I don't think they're a security problem (though they are\neasy to get wrnog, as this discussion shows). But using them with \"%s\"\nshould generally be disallowed.\n\nThere is an fscanf() in builtin/gc.c that uses \"%s\", but it is careful\nto construct a custom format string that limits the string size. Yuck.\nThe usual thing in our code base would be to read into a buffer and\nparse from there.\n\n-Peff\n"},{"id":"518397","messageId":"32de776c-9f15-4030-9f78-76384dd6dbb1@gmail.com","threadId":"63465","inReplyTo":"20250516154250.GA26009@coredump.intra.peff.net","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-19T09:03:47Z","receivedAt":"2025-05-19T09:03:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/05/2025 16:42, Jeff King wrote:\n> On Fri, May 16, 2025 at 02:40:54PM +0100, Phillip Wood wrote:\n> \n>> On 16/05/2025 11:11, Phillip Wood wrote:\n>>\n>>> I had a similar thought, though to make sure that we parsed 64 bit\n>>> values correctly on windows so we'd need something based on strtoumax()\n>>> I think.\n>>\n>> Perhaps something like the diff below which adds strtoul_u64() in a\n>> similar vein to strtoul_ui(). I think it's debatable whether we really\n>> want to skip leading whitespace so we could perhaps tighten things up\n>> by replacing \"if (strchr(s, '-'))\" with \"if (!isdigit(*s))\" though\n>> that would mean this function would behave slightly differently to\n>> strtoul_ui().\n> \n> It feels like we would had to have dealt with this before for other\n> large values. But poking around at a few obvious suspects (e.g.,\n> packSizeLimit), it looks like they are all constrained to \"unsigned\n> long\".\n\nI was surprised by that as well\n\n> So yeah, we probably do need something new. IMHO we should probably have\n> repo_config_get_u64() or similar (with the appropriate underlying\n> helpers as well) as use it here. But I am happy with any solution.\nI think repo_config_get_ulong() and friends all accept a multiplier \nsuffix. That makes sense for things like packSizeLimit but here we're \nexpecting a bare integer. It probably doesn't really matter but as one \nof the code paths parses a file that comes from the bundle server we \nmight want to be as strict as we can be.\n\n> And I do agree that we should consider banning *scanf(). With numeric\n> placeholders I don't think they're a security problem (though they are\n> easy to get wrnog, as this discussion shows). But using them with \"%s\"\n> should generally be disallowed.\n> \n> There is an fscanf() in builtin/gc.c that uses \"%s\", but it is careful\n> to construct a custom format string that limits the string size. Yuck.\n\nYes that looks pretty horrid\n\nPhillip\n\n> The usual thing in our code base would be to read into a buffer and\n> parse from there.\n> \n> -Peff\n\n"},{"id":"518645","messageId":"20250522060337.GA1137854@coredump.intra.peff.net","threadId":"63465","inReplyTo":"32de776c-9f15-4030-9f78-76384dd6dbb1@gmail.com","subject":"Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T06:03:37Z","receivedAt":"2025-05-22T06:03:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 19, 2025 at 10:03:47AM +0100, Phillip Wood wrote:\n\n> > So yeah, we probably do need something new. IMHO we should probably have\n> > repo_config_get_u64() or similar (with the appropriate underlying\n> > helpers as well) as use it here. But I am happy with any solution.\n> I think repo_config_get_ulong() and friends all accept a multiplier suffix.\n> That makes sense for things like packSizeLimit but here we're expecting a\n> bare integer. It probably doesn't really matter but as one of the code paths\n> parses a file that comes from the bundle server we might want to be as\n> strict as we can be.\n\nYeah, I agree it's a little funny to allow suffixes where we wouldn't\nexpect them. But I do think there may be value in just consistently\naccepting them for any numeric config value.\n\nI dunno. I am OK with the more limited solution you proposed.\n\n-Peff\n"}]}