Re: [PATCH v2.5 02/11] bundle: verify using connected()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2023, 17:33 UTC
Message-ID: <xmqqfsbzhm7h.fsf@gitster.g>
In-Reply-To: <ecc6b167-f5c4-48ce-3973-461d1659ed40@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

> When Git verifies a bundle to see if it is safe for unbundling, it first
> looks to see if the prerequisite commits are in the object store. This
> is usually a sufficient filter, and those missing commits are indicated
> clearly in the error messages.

I am not sure if our early check is because "does the prerequisite even exist?" is sufficient. It is a short-cut that is cheap and can be done without preparing the commit traversal.

Show 5 quoted lines

> However, if the commits are present in
> the object store, then there could still be issues if those commits are
> not reachable from the repository's references. The repository only has
> guarantees that its object store is closed under reachability for the
> objects that are reachable from references.

Correct.

Show 5 quoted lines

> Thus, the code in verify_bundle() has previously had the additional
> check that all prerequisite commits are reachable from repository
> references. This is done via a revision walk from all references,
> stopping only if all prerequisite commits are discovered or all commits
> are walked. This uses a custom walk to verify_bundle().

Correct.

> This check is more strict than what Git applies even to fetched
> pack-files.

I do not see the need to say "even" here. In what other situation do we make connectivity checks, and is there a need to be more strict than others when checking fetched packfiles?

> In the fetch case, Git guarantees that the new references
> are closed under reachability by walking from the new references until
> walking commits that are reachable from repository refs. This is done
> through the well-used check_connected() method.

Correct and is a good point to make.

> To better align with the restrictions required by 'git fetch',
> reimplement this check in verify_bundle() to use check_connected(). This
> also simplifies the code significantly.

Wonderful. I never liked the custom check done in unbundle code, which I am reasonably sure came from scripted hack to unbundle I wrote eons ago.