From: Kristofer Karlsson <krka@spotify.com>
Date: Mon, 14 Sep 2026 17:46:05 GMT
Subject: Re: [PATCH 2/2] connected: add incremental connectivity check via rev-list
Message-ID: <CAL71e4My+maYAtWbkoHXvsm=qhCmao7K_7mLV5wg1FyKTa3u8A@mail.gmail.com>
In-Reply-To: <xmqqh5jr7t1h.fsf@gitster.g>

On Mon, 14 Sept 2026 at 17:26, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> 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