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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2022, 20:53 UTC
Message-ID: <xmqqczhbo4s3.fsf@gitster.g>
In-Reply-To: <pull.1214.git.1650301959803.gitgitgadget@gmail.com>

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

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

Show 6 quoted lines

> -	if (!new_oid || is_null_oid(new_oid))
> -		BUG("create called without valid new_oid");
> +	if (!new_oid || is_null_oid(new_oid)) {
> +		strbuf_addf(err, "'%s' has a null OID", refname);
> +		return 1;
> +	}