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

From: Junio C Hamano <[email-masked]>
Date: Tue, 24 Jan 2023 09:33:06 -0800

Derrick Stolee <[email-masked]> 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.

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

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