Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King <peff@peff.net>]

On Thu, Mar 19, 2015 at 01:04:16PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +test_expect_success 'create history with missing tip commit' '
> > +	test_tick && git commit --allow-empty -m one &&
> > +	recoverable=$(git rev-parse HEAD) &&
> > +	git cat-file commit $recoverable >saved &&
> > +	test_tick && git commit --allow-empty -m two &&
> > +	missing=$(git rev-parse HEAD) &&
> > +	# point HEAD elsewhere
> > +	git checkout $base &&
> 
> Could you spell this as "$base^0" (or "--detach") to clarify the
> intention?  I have been scraching my head for a few minutes just
> now, trying to figure out what you are doing here.  I _think_ you
> wanted master to point at the missing "two" and wanted to make sure
> all other refs (including HEAD) to point away from it.

Yes, exactly. I've squashed in your suggestion and added a comment
explaining it:

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 1001a69..1cdbd9f 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh
@@ -50,14 +50,24 @@ test_expect_success 'clean up bogus ref' '
 	rm .git/refs/heads/bogus..name
 '
 
+# We create two new objects here, "one" and "two". Our
+# master branch points to "two", which is deleted,
+# corrupting the repository. But we'd like to make sure
+# that the otherwise unreachable "one" is not pruned
+# (since it is the user's best bet for recovering
+# from the corruption).
+#
+# Note that we also point HEAD somewhere besides "two",
+# as we want to make sure we test the case where we
+# pick up the reference to "two" by iterating the refs,
+# not by resolving HEAD.
 test_expect_success 'create history with missing tip commit' '
 	test_tick && git commit --allow-empty -m one &&
 	recoverable=$(git rev-parse HEAD) &&
 	git cat-file commit $recoverable >saved &&
 	test_tick && git commit --allow-empty -m two &&
 	missing=$(git rev-parse HEAD) &&
-	# point HEAD elsewhere
-	git checkout $base &&
+	git checkout --detach $base &&
 	rm .git/objects/$(echo $missing | sed "s,..,&/,") &&
 	test_must_fail git cat-file -e $missing
 '

> > +# we do not want to count on running pack-refs to
> > +# actually pack it, as it is perfectly reasonable to
> > +# skip processing a broken ref
> > +test_expect_success 'create packed-refs file with broken ref' '
> > +	rm -f .git/refs/heads/master &&
> > +	cat >.git/packed-refs <<-EOF
> > +	$missing refs/heads/master
> > +	$recoverable refs/heads/other
> > +	EOF
> 
> I do not know offhand if the lack of the pack-refs feature header
> matters here; I assume it does not?

It doesn't matter. We also do similarly gross things in other
corruption-related tests, but I suspect if you git-blamed them all you
would find that I am responsible. :)

> A safer check may be to pack and then make it missing, I guess, but
> I do not know if the difference matters.

Yeah, I considered that. The trouble is that we are relying on the
earlier setup that made the object go missing. We cannot pack the refs
in the setup step, because the earlier tests are checking the loose-ref
behavior. So we would have to actually restore the object, pack, and
then re-delete it.

Another option would be to restructure the whole test script to perform
each individual corruption in its own sub-repo. I thought that would end
up making things harder to understand due to the extra setup
boilerplate, but it would make the tests less fragile with respect to
each other (e.g., see the "clean up bogus ref" step which exists only to
clean up our earlier corruption that could influence later tests).

-Peff