Re: [PATCH 2/2] connected: add incremental connectivity check via rev-list

[Junio C Hamano <gitster@pobox.com>]

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +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?

> +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.