Re: [PATCH 2/2] connected: add incremental connectivity check via rev-list
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 14, 2026, 15:26 UTC
- Message-ID
- <xmqqh5jr7t1h.fsf@gitster.g>
- In-Reply-To
- <ebe6c90cc58b9e1f64c9bec4a18e8cb3ce9be1b2.1789379276.git.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 23 quoted lines
> +static void verify_blob(struct repository *repo,
> + const struct object_id *oid,
> + struct verify_state *vs)
> +{
> + int type;
> +
> + if (oidset_contains(&vs->trusted_blobs, oid))
> + return;
> +
> + vs->blobs_checked++;
> + type = odb_read_object_info(repo->objects, oid, NULL);
> + if (type == OBJ_BLOB) {
> + oidset_insert(&vs->trusted_blobs, oid);
> + return;
> + }
> + if (type >= 0)
> + die(_("object %s is a %s, not a blob"),
> + oid_to_hex(oid), type_name(type));
> + if (vs->exclude_promisor_objects &&
> + is_promisor_object(repo, oid))
> + return;
> + die(_("missing blob object '%s'"), oid_to_hex(oid));
> +}I wonder if this is_promisor_object() call comes a bit too late, as we earlier already have called odb_read_object_info() which may have fetched it lazily from the promisor remote? Or do we globally disable promisor_remote_get_direct() call somehow without having to pass OBJECT_INFO_SKIP_FETCH_OBJECT flag?
Show 24 quoted lines
> +static void verify_commit_tree(struct repository *repo,
> + struct commit *commit,
> + struct verify_state *vs)
> +{
> + struct oid_array base_trees = OID_ARRAY_INIT;
> + struct commit_list *p;
> +
> + /*
> + * Parent trees are trusted: boundary parents are already
> + * connected, and earlier incoming parents were verified
> + * first due to the topological processing order.
> + */
> + for (p = commit->parents; p; p = p->next) {
> + const struct object_id *tree_oid;
> + parse_commit_or_die(p->item);
> + tree_oid = get_commit_tree_oid(p->item);
> + tree_map_add(vs->trees, tree_oid, TREE_TRUSTED);
> + oid_array_append(&base_trees, tree_oid);
> + }
> +
> + verify_tree(repo, get_commit_tree_oid(commit),
> + &base_trees, vs, 0);
> + oid_array_clear(&base_trees);
> +}Do we assume that we do not have to deal with repository corruption in any graceful way? I am just wondering what happens when get_commit_tree_oid() yields NULL after parse_commit_or_die() finds p->item is a valid-looking commit object but the tree within it is not, and we end up passing NULL to tree_map_add(), perhaps?
The same potential issue may exist in the get_commit_tree_oid() call outside the look at the end on the incoming commit's tree.