From 131a8fa8151c95f309241ead33018f30f57ff57c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:39 +0000 Subject: [PATCH 01/11] commit: simplify code The difference of two unsigned integers is defined to be unsigned, and therefore it is misleading to check whether it is greater than zero (instead, the more natural way would be to check whether the difference is zero or not). Let's instead avoid the subtraction altogether, and compare the two operands directly, which makes the code more obvious as a side effect. Pointed out by CodeQL's rule with the ID `cpp/unsigned-difference-expression-compared-zero`. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index 66bd91fd52..fba0dded64 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1022,7 +1022,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, for (i = 0; i < the_repository->index->cache_nr; i++) if (ce_intent_to_add(the_repository->index->cache[i])) ita_nr++; - committable = the_repository->index->cache_nr - ita_nr > 0; + committable = the_repository->index->cache_nr > ita_nr; } else { /* * Unless the user did explicitly request a submodule From c607410ada02fce5ee2366b68543736176101295 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:40 +0000 Subject: [PATCH 02/11] fetch: carefully clear local variable's address after use As pointed out by CodeQL, it is a potentially dangerous practice to store local variables' addresses in non-local structs. Yet this is exactly what happens with the `acked_commits` attribute that is used in `cmd_fetch()`: The pointer to a local variable is assigned to it. Now, it is Git's convention that `cmd_*()` functions are essentially only returning just before exiting the process, therefore there is little danger that this attribute is used after the code flow returns from that function. However, code in `cmd_*()` function is often so useful that it gets lifted into a library function, at which point this issue could become a real problem. Let's make sure to clear the `acked_commits` attribute out after it was used, and before the function returns (at which point the address would go stale). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/fetch.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/fetch.c b/builtin/fetch.c index cda6eaf1fd..c1a1434c70 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2560,6 +2560,7 @@ int cmd_fetch(int argc, if (server_options.nr) gtransport->server_options = &server_options; result = transport_fetch_refs(gtransport, NULL); + gtransport->smart_options->acked_commits = NULL; oidset_iter_init(&acked_commits, &iter); while ((oid = oidset_iter_next(&iter))) From 7f3ed75ff551e2ca4f8eb0242784e7aacbb14fb3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:41 +0000 Subject: [PATCH 03/11] commit-graph: avoid malloc'ing a local variable We do need a context to write the commit graph, but that context is only needed during the life time of `commit_graph_write()`, therefore it can easily be a stack variable. This also helps CodeQL recognize that it is safe to assign the address of other local variables to the context's fields. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- commit-graph.c | 141 ++++++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 72 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 6394752b0b..9f0115dac9 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2509,7 +2509,17 @@ int write_commit_graph(struct object_directory *odb, const struct commit_graph_opts *opts) { struct repository *r = the_repository; - struct write_commit_graph_context *ctx; + struct write_commit_graph_context ctx = { + .r = r, + .odb = odb, + .append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0, + .report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0, + .split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0, + .opts = opts, + .total_bloom_filter_data_size = 0, + .write_generation_data = (get_configured_generation_version(r) == 2), + .num_generation_data_overflows = 0, + }; uint32_t i; int res = 0; int replace = 0; @@ -2531,17 +2541,6 @@ int write_commit_graph(struct object_directory *odb, return 0; } - CALLOC_ARRAY(ctx, 1); - ctx->r = r; - ctx->odb = odb; - ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0; - ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0; - ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0; - ctx->opts = opts; - ctx->total_bloom_filter_data_size = 0; - ctx->write_generation_data = (get_configured_generation_version(r) == 2); - ctx->num_generation_data_overflows = 0; - bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version; bloom_settings.bits_per_entry = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY", bloom_settings.bits_per_entry); @@ -2549,14 +2548,14 @@ int write_commit_graph(struct object_directory *odb, bloom_settings.num_hashes); bloom_settings.max_changed_paths = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS", bloom_settings.max_changed_paths); - ctx->bloom_settings = &bloom_settings; + ctx.bloom_settings = &bloom_settings; init_topo_level_slab(&topo_levels); - ctx->topo_levels = &topo_levels; + ctx.topo_levels = &topo_levels; - prepare_commit_graph(ctx->r); - if (ctx->r->objects->commit_graph) { - struct commit_graph *g = ctx->r->objects->commit_graph; + prepare_commit_graph(ctx.r); + if (ctx.r->objects->commit_graph) { + struct commit_graph *g = ctx.r->objects->commit_graph; while (g) { g->topo_levels = &topo_levels; @@ -2565,15 +2564,15 @@ int write_commit_graph(struct object_directory *odb, } if (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS) - ctx->changed_paths = 1; + ctx.changed_paths = 1; if (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) { struct commit_graph *g; - g = ctx->r->objects->commit_graph; + g = ctx.r->objects->commit_graph; /* We have changed-paths already. Keep them in the next graph */ if (g && g->bloom_filter_settings) { - ctx->changed_paths = 1; + ctx.changed_paths = 1; /* don't propagate the hash_version unless unspecified */ if (bloom_settings.hash_version == -1) @@ -2586,116 +2585,114 @@ int write_commit_graph(struct object_directory *odb, bloom_settings.hash_version = bloom_settings.hash_version == 2 ? 2 : 1; - if (ctx->split) { - struct commit_graph *g = ctx->r->objects->commit_graph; + if (ctx.split) { + struct commit_graph *g = ctx.r->objects->commit_graph; while (g) { - ctx->num_commit_graphs_before++; + ctx.num_commit_graphs_before++; g = g->base_graph; } - if (ctx->num_commit_graphs_before) { - ALLOC_ARRAY(ctx->commit_graph_filenames_before, ctx->num_commit_graphs_before); - i = ctx->num_commit_graphs_before; - g = ctx->r->objects->commit_graph; + if (ctx.num_commit_graphs_before) { + ALLOC_ARRAY(ctx.commit_graph_filenames_before, ctx.num_commit_graphs_before); + i = ctx.num_commit_graphs_before; + g = ctx.r->objects->commit_graph; while (g) { - ctx->commit_graph_filenames_before[--i] = xstrdup(g->filename); + ctx.commit_graph_filenames_before[--i] = xstrdup(g->filename); g = g->base_graph; } } - if (ctx->opts) - replace = ctx->opts->split_flags & COMMIT_GRAPH_SPLIT_REPLACE; + if (ctx.opts) + replace = ctx.opts->split_flags & COMMIT_GRAPH_SPLIT_REPLACE; } - ctx->approx_nr_objects = repo_approximate_object_count(the_repository); + ctx.approx_nr_objects = repo_approximate_object_count(the_repository); - if (ctx->append && ctx->r->objects->commit_graph) { - struct commit_graph *g = ctx->r->objects->commit_graph; + if (ctx.append && ctx.r->objects->commit_graph) { + struct commit_graph *g = ctx.r->objects->commit_graph; for (i = 0; i < g->num_commits; i++) { struct object_id oid; oidread(&oid, g->chunk_oid_lookup + st_mult(g->hash_len, i), the_repository->hash_algo); - oid_array_append(&ctx->oids, &oid); + oid_array_append(&ctx.oids, &oid); } } if (pack_indexes) { - ctx->order_by_pack = 1; - if ((res = fill_oids_from_packs(ctx, pack_indexes))) + ctx.order_by_pack = 1; + if ((res = fill_oids_from_packs(&ctx, pack_indexes))) goto cleanup; } if (commits) { - if ((res = fill_oids_from_commits(ctx, commits))) + if ((res = fill_oids_from_commits(&ctx, commits))) goto cleanup; } if (!pack_indexes && !commits) { - ctx->order_by_pack = 1; - fill_oids_from_all_packs(ctx); + ctx.order_by_pack = 1; + fill_oids_from_all_packs(&ctx); } - close_reachable(ctx); + close_reachable(&ctx); - copy_oids_to_commits(ctx); + copy_oids_to_commits(&ctx); - if (ctx->commits.nr >= GRAPH_EDGE_LAST_MASK) { + if (ctx.commits.nr >= GRAPH_EDGE_LAST_MASK) { error(_("too many commits to write graph")); res = -1; goto cleanup; } - if (!ctx->commits.nr && !replace) + if (!ctx.commits.nr && !replace) goto cleanup; - if (ctx->split) { - split_graph_merge_strategy(ctx); + if (ctx.split) { + split_graph_merge_strategy(&ctx); if (!replace) - merge_commit_graphs(ctx); + merge_commit_graphs(&ctx); } else - ctx->num_commit_graphs_after = 1; + ctx.num_commit_graphs_after = 1; - ctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph); + ctx.trust_generation_numbers = validate_mixed_generation_chain(ctx.r->objects->commit_graph); - compute_topological_levels(ctx); - if (ctx->write_generation_data) - compute_generation_numbers(ctx); + compute_topological_levels(&ctx); + if (ctx.write_generation_data) + compute_generation_numbers(&ctx); - if (ctx->changed_paths) - compute_bloom_filters(ctx); + if (ctx.changed_paths) + compute_bloom_filters(&ctx); - res = write_commit_graph_file(ctx); + res = write_commit_graph_file(&ctx); - if (ctx->changed_paths) + if (ctx.changed_paths) deinit_bloom_filters(); - if (ctx->split) - mark_commit_graphs(ctx); + if (ctx.split) + mark_commit_graphs(&ctx); - expire_commit_graphs(ctx); + expire_commit_graphs(&ctx); cleanup: - free(ctx->graph_name); - free(ctx->base_graph_name); - free(ctx->commits.list); - oid_array_clear(&ctx->oids); + free(ctx.graph_name); + free(ctx.base_graph_name); + free(ctx.commits.list); + oid_array_clear(&ctx.oids); clear_topo_level_slab(&topo_levels); - for (i = 0; i < ctx->num_commit_graphs_before; i++) - free(ctx->commit_graph_filenames_before[i]); - free(ctx->commit_graph_filenames_before); + for (i = 0; i < ctx.num_commit_graphs_before; i++) + free(ctx.commit_graph_filenames_before[i]); + free(ctx.commit_graph_filenames_before); - for (i = 0; i < ctx->num_commit_graphs_after; i++) { - free(ctx->commit_graph_filenames_after[i]); - free(ctx->commit_graph_hash_after[i]); + for (i = 0; i < ctx.num_commit_graphs_after; i++) { + free(ctx.commit_graph_filenames_after[i]); + free(ctx.commit_graph_hash_after[i]); } - free(ctx->commit_graph_filenames_after); - free(ctx->commit_graph_hash_after); - - free(ctx); + free(ctx.commit_graph_filenames_after); + free(ctx.commit_graph_hash_after); return res; } From bf0468e2ba64ac358a61cb01a675b7c5919d64fd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:42 +0000 Subject: [PATCH 04/11] upload-pack: rename `enum` to reflect the operation While 3145ea957d (upload-pack: introduce fetch server command, 2018-03-15) added support for the `fetch` command, from the server's point of view it is an upload, and hence the `enum` should really be called `upload_state` instead of `fetch_state`. Likewise, rename its values. This also helps unconfuse CodeQL which would otherwise be at sixes or sevens about having _two_ non-local definitions of the same `enum` with the same values. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- upload-pack.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index 956da5b061..26f29b85b5 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1780,16 +1780,16 @@ static void send_shallow_info(struct upload_pack_data *data) packet_delim(1); } -enum fetch_state { - FETCH_PROCESS_ARGS = 0, - FETCH_SEND_ACKS, - FETCH_SEND_PACK, - FETCH_DONE, +enum upload_state { + UPLOAD_PROCESS_ARGS = 0, + UPLOAD_SEND_ACKS, + UPLOAD_SEND_PACK, + UPLOAD_DONE, }; int upload_pack_v2(struct repository *r, struct packet_reader *request) { - enum fetch_state state = FETCH_PROCESS_ARGS; + enum upload_state state = UPLOAD_PROCESS_ARGS; struct upload_pack_data data; clear_object_flags(the_repository, ALL_FLAGS); @@ -1798,9 +1798,9 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) data.use_sideband = LARGE_PACKET_MAX; get_upload_pack_config(r, &data); - while (state != FETCH_DONE) { + while (state != UPLOAD_DONE) { switch (state) { - case FETCH_PROCESS_ARGS: + case UPLOAD_PROCESS_ARGS: process_args(request, &data); if (!data.want_obj.nr && !data.wait_for_done) { @@ -1811,27 +1811,27 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) * to just send 'have's without 'want's); guess * they didn't want anything. */ - state = FETCH_DONE; + state = UPLOAD_DONE; } else if (data.seen_haves) { /* * Request had 'have' lines, so lets ACK them. */ - state = FETCH_SEND_ACKS; + state = UPLOAD_SEND_ACKS; } else { /* * Request had 'want's but no 'have's so we can * immediately go to construct and send a pack. */ - state = FETCH_SEND_PACK; + state = UPLOAD_SEND_PACK; } break; - case FETCH_SEND_ACKS: + case UPLOAD_SEND_ACKS: if (process_haves_and_send_acks(&data)) - state = FETCH_SEND_PACK; + state = UPLOAD_SEND_PACK; else - state = FETCH_DONE; + state = UPLOAD_DONE; break; - case FETCH_SEND_PACK: + case UPLOAD_SEND_PACK: send_wanted_ref_info(&data); send_shallow_info(&data); @@ -1841,9 +1841,9 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) packet_writer_write(&data.writer, "packfile\n"); create_pack_file(&data, NULL); } - state = FETCH_DONE; + state = UPLOAD_DONE; break; - case FETCH_DONE: + case UPLOAD_DONE: continue; } } From 655268452cafd061c6c38541a719b6f5b9d528e3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:43 +0000 Subject: [PATCH 05/11] has_dir_name(): make code more obvious One thing that might be non-obvious to readers (or to analyzers like CodeQL) is that the function essentially does nothing when the Git index is empty, and in particular that it does not look at the value of `len_eq_last` (which would be uninitialized at that point). Let's make this much easier to understand, by returning early if the Git index is empty, and by avoiding empty `else` blocks. This commit changes indentation and is hence best viewed using `--ignore-space-change`. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- read-cache.c | 55 +++++++++++++--------------------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/read-cache.c b/read-cache.c index 73f83a7e7a..c0bb760ad4 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1117,48 +1117,19 @@ static int has_dir_name(struct index_state *istate, * * Compare the entry's full path with the last path in the index. */ - if (istate->cache_nr > 0) { - cmp_last = strcmp_offset(name, - istate->cache[istate->cache_nr - 1]->name, - &len_eq_last); - if (cmp_last > 0) { - if (name[len_eq_last] != '/') { - /* - * The entry sorts AFTER the last one in the - * index. - * - * If there were a conflict with "file", then our - * name would start with "file/" and the last index - * entry would start with "file" but not "file/". - * - * The next character after common prefix is - * not '/', so there can be no conflict. - */ - return retval; - } else { - /* - * The entry sorts AFTER the last one in the - * index, and the next character after common - * prefix is '/'. - * - * Either the last index entry is a file in - * conflict with this entry, or it has a name - * which sorts between this entry and the - * potential conflicting file. - * - * In both cases, we fall through to the loop - * below and let the regular search code handle it. - */ - } - } else if (cmp_last == 0) { - /* - * The entry exactly matches the last one in the - * index, but because of multiple stage and CE_REMOVE - * items, we fall through and let the regular search - * code handle it. - */ - } - } + if (!istate->cache_nr) + return 0; + + cmp_last = strcmp_offset(name, + istate->cache[istate->cache_nr - 1]->name, + &len_eq_last); + if (cmp_last > 0 && name[len_eq_last] != '/') + /* + * The entry sorts AFTER the last one in the + * index and their paths have no common prefix, + * so there cannot be a F/D conflict. + */ + return 0; for (;;) { size_t len; From 6c91162449cb0a2fe3c42a1caa232444afed9c7c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:44 +0000 Subject: [PATCH 06/11] fetch: avoid unnecessary work when there is no current branch As pointed out by CodeQL, `branch_get()` may return `NULL`, in which case `branch_has_merge_config()` would return early, but we can even avoid enumerating the refs prefixes in that case, saving even more CPU cycles. Technically, we should enclose these two statements in an `if (branch) {...}` block, but the indentation is already quite deep, therefore I refrained from doing that. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index c1a1434c70..40a0e8d244 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1728,7 +1728,7 @@ static int do_fetch(struct transport *transport, if (transport->remote->follow_remote_head != FOLLOW_REMOTE_NEVER) do_set_head = 1; } - if (branch_has_merge_config(branch) && + if (branch && branch_has_merge_config(branch) && !strcmp(branch->remote_name, transport->remote->name)) { int i; for (i = 0; i < branch->merge_nr; i++) { From 3d39bcd98ecce0fce77b00fd680bd245b2161ddf Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:45 +0000 Subject: [PATCH 07/11] Avoid redundant conditions While `if (i <= 0) ... else if (i > 0) ...` is technically equivalent to `if (i <= 0) ... else ...`, the latter is vastly easier to read because it avoids writing out a condition that is unnecessary. Let's drop such unnecessary conditions. Pointed out by CodeQL. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- help.c | 2 +- transport-helper.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index 6ef90838f1..21b778707a 100644 --- a/help.c +++ b/help.c @@ -214,7 +214,7 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) else if (cmp == 0) { ei++; free(cmds->names[ci++]); - } else if (cmp > 0) + } else ei++; } diff --git a/transport-helper.c b/transport-helper.c index 69391ee7d2..0789e5bca5 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1437,7 +1437,7 @@ static int udt_do_read(struct unidirectional_transfer *t) transfer_debug("%s EOF (with %i bytes in buffer)", t->src_name, (int)t->bufuse); t->state = SSTATE_FLUSHING; - } else if (bytes > 0) { + } else { t->bufuse += bytes; transfer_debug("Read %i bytes from %s (buffer now at %i)", (int)bytes, t->src_name, (int)t->bufuse); From fc451e6ea85310725532cbdbc280f8a56a7ec7df Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:46 +0000 Subject: [PATCH 08/11] trace2: avoid "futile conditional" CodeQL reports empty `if` blocks that only contain a comment as "futile conditional". The comment talks about potential plans to turn this into a warning, but that seems not to have been necessary. Replace the entire construct with a concise comment. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- trace2/tr2_tmr.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c index 51f564b07a..038181ad9b 100644 --- a/trace2/tr2_tmr.c +++ b/trace2/tr2_tmr.c @@ -102,25 +102,11 @@ void tr2_update_final_timers(void) struct tr2_timer *t_final = &final_timer_block.timer[tid]; struct tr2_timer *t = &ctx->timer_block.timer[tid]; - if (t->recursion_count) { - /* - * The current thread is exiting with - * timer[tid] still running. - * - * Technically, this is a bug, but I'm going - * to ignore it. - * - * I don't think it is worth calling die() - * for. I don't think it is worth killing the - * process for this bookkeeping error. We - * might want to call warning(), but I'm going - * to wait on that. - * - * The downside here is that total_ns won't - * include the current open interval (now - - * start_ns). I can live with that. - */ - } + /* + * `t->recursion_count` could technically be non-zero, which + * would constitute a bug. Reporting the bug would potentially + * cause an infinite recursion, though, so let's ignore it. + */ if (!t->interval_count) continue; /* this timer was not used by this thread */ From ee63d026b407118221aca455a9c4f03a08ecf648 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:47 +0000 Subject: [PATCH 09/11] commit-graph: avoid using stale stack addresses The code is a bit too hard to reason about to fully assess whether the `fill_commit_graph_info()` function is called at all after `write_commit_graph()` returns (and hence the stack variable `topo_levels` goes out of context). Let's simply make sure that the stack address is no longer used at that stage, thereby making the code quite a bit easier to reason about. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- commit-graph.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 9f0115dac9..d052c1bf15 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2683,6 +2683,15 @@ cleanup: oid_array_clear(&ctx.oids); clear_topo_level_slab(&topo_levels); + if (ctx.r->objects->commit_graph) { + struct commit_graph *g = ctx.r->objects->commit_graph; + + while (g) { + g->topo_levels = NULL; + g = g->base_graph; + } + } + for (i = 0; i < ctx.num_commit_graphs_before; i++) free(ctx.commit_graph_filenames_before[i]); free(ctx.commit_graph_filenames_before); From d7cfbd4351bb304eefc09a8b1ba24fd40a9f36a0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:48 +0000 Subject: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()` In c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31) code was introduced that assumes that an `sscanf()` call leaves its output variables unchanged unless the return value indicates success. However, the POSIX documentation makes no such guarantee: https://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html So let's make sure that the output variable `maxCreationToken` is always well-defined. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- bundle-uri.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/bundle-uri.c b/bundle-uri.c index 96d2ba726d..13a42f9238 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -532,11 +532,13 @@ static int fetch_bundles_by_token(struct repository *r, */ if (!repo_config_get_value(r, "fetch.bundlecreationtoken", - &creationTokenStr) && - sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 && - bundles.items[0]->creationToken <= maxCreationToken) { - free(bundles.items); - return 0; + &creationTokenStr)) { + if (sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) != 1) + maxCreationToken = 0; + if (bundles.items[0]->creationToken <= maxCreationToken) { + free(bundles.items); + return 0; + } } /* From 22488332393646cfa4263bcb24836f492876406e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 15 May 2025 13:11:49 +0000 Subject: [PATCH 11/11] sequencer: stop pretending that an assignment is a condition In 3e81bccdf3 (sequencer: factor out todo command name parsing, 2019-06-27), a `return` statement was introduced that basically was a long sequence of conditions, combined with `&&`, except for the last condition which is not really a condition but an assignment. The point of this construct was to return 1 (i.e. `true`) from the function if all of those conditions held true, and also assign the `bol` pointer to the end of the parsed command. Some static analyzers are really unhappy about such constructs. And human readers are at least puzzled, if not confused, by seeing a single `=` inside a chain of conditions where they would have expected to see `==` instead and, based on experience, immediately suspect a typo. Let's help all of this by turning this into the more verbose, more readable form of an `if` construct that both assigns the pointer as well as returns 1 if all of the conditions hold true. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sequencer.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index b5c4043757..e5e3bc6fa5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2600,9 +2600,12 @@ static int is_command(enum todo_command command, const char **bol) const char nick = todo_command_info[command].c; const char *p = *bol; - return (skip_prefix(p, str, &p) || (nick && *p++ == nick)) && - (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) && - (*bol = p); + if ((skip_prefix(p, str, &p) || (nick && *p++ == nick)) && + (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p)) { + *bol = p; + return 1; + } + return 0; } static int check_label_or_ref_arg(enum todo_command command, const char *arg)