Re: [RFC PATCH 2/4] fsck: support refs pointing to lazy objects

[Jonathan Tan <jonathantanmy@google.com>]

On Thu, 27 Jul 2017 11:59:47 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> > @@ -438,6 +438,14 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,
> >  
> >  	obj = parse_object(oid);
> >  	if (!obj) {
> > +		if (repository_format_lazy_object) {
> > +			/*
> > +			 * Increment default_refs anyway, because this is a
> > +			 * valid ref.
> > +			 */
> > +			default_refs++;
> > +			return 0;
> > +		}
> >  		error("%s: invalid sha1 pointer %s", refname, oid_to_hex(oid));
> >  		errors_found |= ERROR_REACHABLE;
> 
> At this point, do we know (or can we tell) if this is a missing
> object or a file exists as a loose object but is corrupt?  If we
> could, it would be nice to do this only for the former to avoid
> sweeping a real corruption that is unrelated to the lazy fetch under
> the rug.

Before all this is run, there is a check over all loose and packed
objects and I've verified that this check reports failure in
corrupt-object situations (see test below).

It is true that parse_object() cannot report the difference, but since
fsck has already verified non-corruptness, I don't think it needs to
know the difference at this point.

> > +test_expect_success 'fsck fails on lazy object pointed to by ref' '
> > +	rm -rf repo &&
> > +	test_create_repo repo &&
> > +	test_commit -C repo 1 &&
> > +
> > +	A=$(git -C repo commit-tree -m a HEAD^{tree}) &&
> > +
> > +	# Reference $A only from ref, and delete it
> > +	git -C repo branch mybranch "$A" &&
> > +	delete_object repo "$A" &&
> > +
> > +	test_must_fail git -C repo fsck
> > +'
> 
> And a new test that uses a helper different from delete_object
> (perhaps call it corrupt_object?) can be used to make sure that we
> complain in that case here.

I agree that object corruption can cause this specific part of the
production code to falsely work. But I think that this specific part of
the code can and should rely on object corruption being checked
elsewhere. (I usually don't like to assume that other components work
and will continue to work, but in this case, I think that fsck checking
for object corruption is very foundational and should be relied upon.)

But if we think that defense "in depth" is a good idea, I have no
problem adding such tests (like the one below).

---
delete_object () {
	rm $1/.git/objects/$(echo $2 | cut -c1-2)/$(echo $2 | cut -c3-40)
}

corrupt_object () {
	chmod a+w $1/.git/objects/$(echo $2 | cut -c1-2)/$(echo $2 | cut -c3-40) &&
	echo CORRUPT >$1/.git/objects/$(echo $2 | cut -c1-2)/$(echo $2 | cut -c3-40)
}

setup_object_in_reflog () {
	rm -rf repo &&
	test_create_repo repo &&
	test_commit -C repo 1 &&

	A=$(git -C repo commit-tree -m a HEAD^{tree}) &&
	B=$(git -C repo commit-tree -m b HEAD^{tree}) &&

	# Reference $A only from reflog
	git -C repo branch mybranch "$A" &&
	git -C repo branch -f mybranch "$B"
}

test_expect_success 'lazy object in reflog' '
	setup_object_in_reflog &&
	delete_object repo "$A" &&
	test_must_fail git -C repo fsck &&
	git -C repo config core.repositoryformatversion 1 &&
	git -C repo config extensions.lazyobject "arbitrary string" &&
	git -C repo fsck
'

test_expect_success 'corrupt loose object in reflog' '
	setup_object_in_reflog &&
	corrupt_object repo "$A" &&
	test_must_fail git -C repo fsck &&
	git -C repo config core.repositoryformatversion 1 &&
	git -C repo config extensions.lazyobject "arbitrary string" &&
	test_must_fail git -C repo fsck
'

test_expect_success 'missing packed object in reflog' '
	setup_object_in_reflog &&
	git -C repo repack -a &&
	delete_object repo "$A" &&
	chmod a+w repo/.git/objects/pack/*.pack &&
	echo CORRUPT >"$(echo repo/.git/objects/pack/*.pack)" &&
	test_must_fail git -C repo fsck &&
	git -C repo config core.repositoryformatversion 1 &&
	git -C repo config extensions.lazyobject "arbitrary string" &&
	test_must_fail git -C repo fsck
'