Re: [PATCH] clone: ignore invalid local refs in remote

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 21, 2022, 13:29 UTC
Message-ID: <b884d627-4a81-5be9-eed5-feff3b2bd010@github.com>
In-Reply-To: <xmqqczhbo4s3.fsf@gitster.g>

On 4/20/2022 4:53 PM, Junio C Hamano wrote:

Show 10 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>> Subject: Re: [PATCH] clone: ignore invalid local refs in remote
> 
> After seeing the title, I expected that cloning from such a
> repository with cruft in .git/refs/ directory would issue a warning
> and succeed without these non-ref files.
> 
> But that is not what is happening here?

Sorry, this title of the commit message is stale from a version
where I started making the clones succeed (but without these
bad refs). I changed my mind to only switch BUG() to die() to
avoid giving the impression that we have a "matching" repo after
the clone.

Show 21 quoted lines

>> +test_expect_success 'local clone from repo with corrupt refs fails gracefully' '
>> +	git init corrupt &&
>> +	test_commit -C corrupt one &&
>> +	echo a >corrupt/.git/refs/heads/topic &&
>> +
>> +	test_must_fail git clone corrupt working 2>err &&
>> +	grep "has a null OID" err
>> +'
>> +
> 
> We keep expecting that clone _will_ fail.
> 
> So the net change is that we still do not tolerate a corrupt
> repository and do not let corruption to propagate through cloning,
> but we diagnose this breakage as an error by calling die(), which 
> is appropriate for dealing with runtime data error, instead of
> hitting a BUG(), which is reserved for program errors.
> 
> I agree with the fixed behaviour and implementation.  It just is
> that "ignore" on the title seems misleading.  Other than that,
> thanks for a good finding and a clean fix.

Perhaps the commit could instead start with

	clone: die() instead of BUG() on bad refs

Thanks, -Stolee