On 4/20/2022 4:53 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> 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. >> +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
