Re: ab/fsck-unexpected-type (and "cat-file replace handling and optimization")

From: Jeff King <peff@peff.net>
Date: Oct 8, 2021, 02:25 UTC
Message-ID: <YV+sLmlWufC5ullA@coredump.intra.peff.net>
In-Reply-To: <xmqq8rz4pldt.fsf@gitster.g>

On Thu, Oct 07, 2021 at 02:37:02PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I don't mind rebasing on top if that's easier for Junio, but in that
> > case it may make sense to float the test cleanup to the front of the
> > series.
> 
> Yeah, I would rather not to see such a rebase (and I hate the kind
> of churns we seem to be allowing more and more in our codebase that
> causes needless textual conflicts.  We should really slow down and
> make fewer pointless "clean-up" that disrupts real work).

In this case I think neither is churn. I needed to clean up the earlier part of the test script so that my later tests didn't break, and Ævar touched the same area to cover more cases in those tests.

Show 10 quoted lines

> > - test_expect_success 'cat-file -t and -s on corrupt loose object' '
> > - 	git init --bare corrupt-loose.git &&
> > - 	(
> > - 		cd corrupt-loose.git &&
> > ...
> > - 		test_must_fail git fsck 2>err.fsck &&
> > - 		grep "^error: inflate: data stream error (" err.fsck
> > - 	)
> 
> ... what's this lossage of the whole new test?

Whoops, indeed. I was little too anxious in my editor and then read the combined diff backwards. That test should obviously stay (it is new in Ævar's series). It still appears in total in the combined diff, but of course as a "+" on his side.

One other possible thing to do during the resolution is combine the cleanup of both objects in a single test (since in Ævar's series, both objects are created together).

I.e., throw away my side from both conflicts, and do this on top:

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 0f52ca9cc8..508a70d877 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -487,6 +487,11 @@ test_expect_success "Size of large broken object is correct when type is large"
 	test_cmp expect actual
 '
 
+test_expect_success 'clean up broken objects' '
+	rm .git/objects/$(test_oid_to_path $bogus_short_sha1) \
+	   .git/objects/$(test_oid_to_path $bogus_long_sha1)
+'
+
 test_expect_success 'cat-file -t and -s on corrupt loose object' '
 	git init --bare corrupt-loose.git &&
 	(

That makes the merge more evil, but the end result is perhaps a little
nicer for people reading it later.

Thanks for reading my proposed resolution so carefully. :)

-Peff