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

On Mon, 14 Sept 2026 at 17:26, Junio C Hamano [off-list ref] wrote:

"Kristofer Karlsson via GitGitGadget" [off-list ref]
writes:

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?

Yes, I think it's safe due to the following mechanism:

1. If promisors exist, the connectivity-check will invoke
   rev-list with --exclude-promisor-objects.
2. rev-list in turn sets repo->fetch_if_missing = 0 on startup.
3. Then the odb read goes down into do_oid_object_info_extended()
   which respects that flag.

However, my paranoia kicked in so I re-ran my test for this,
after adding some temporary code inside
verify_commits_incremental():

    repo->fetch_if_missing = 1;

And fortunately, one of the tests failed as expected.

    Exactly 1 failure out of 62 tests: test 53
      "incremental: verifies new subtree when parent subtree is
       promised".

And the relevant assertion is this one:

    test_must_fail env GIT_NO_LAZY_FETCH=1 \
        git cat-file -e "$parent_subtree"

which ensures that the object was never fetched.

However, the test only catches this scenario for trees,
not blobs -- that's an oversight, I will add a matching
test for blobs too.

I think the code technically works as-is, but I could also try
to rewrite the code to stop depending on odb_read_object_info()
and instead use odb_read_object_info_extended() which allows
me to pass the flags.  That gives us belts and suspenders, which
may be nicer here.

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.

You're right, this is an oversight.
I think I incorrectly assumed that parse_commit_or_die()
would catch any malformed commit.

I will add a NULL check and a die()-exit at the two call sites
in verify_commit_tree()

    die(_("unable to load root tree for commit %s"),
        oid_to_hex(&commit->object.oid));

Thanks for spotting these errors,
Kristofer