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

From: Derrick Stolee <[email-masked]>
Date: Tue, 24 Jan 2023 13:46:02 -0500

On 1/24/2023 12:33 PM, Junio C Hamano wrote:
> 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.

I suppose I should say "Usually, existence in the object store is
correlated with having all reachable objects, but this is not
guaranteed." I'll also mention that it is a short-cut that can fail
faster than the reachability check.

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

I suppose that I was implying that fetches are the more common
operation, and the scrutiny applied to an arbitrary pack-file from
a remote is probably higher there. However, who knows where a
bundle came from, so the scrutiny should be the same.

>> 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.
 
Excellent. Thanks for your feedback.

-Stolee