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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 24, 2023, 18:46 UTC
Message-ID: <bad5d998-b34a-15dd-4f9d-9bf1cb74dda0@github.com>
In-Reply-To: <xmqqfsbzhm7h.fsf@gitster.g>

On 1/24/2023 12:33 PM, Junio C Hamano wrote:

Show 10 quoted lines

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

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.

Show 6 quoted lines

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

Show 7 quoted lines

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