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

2 messages, 2 authors, 2021-10-08 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-10-07 21:37:08

Jeff King [off-list ref] 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).
quoted hunk
diff --cc t/t1006-cat-file.sh
index a5e7401af8,4a753705ec..0000000000
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@@ -475,9 -332,18 +475,13 @@@ test_expect_success "Size of broken obj
  	test_cmp expect actual
  '
  
+ test_expect_success 'clean up broken object' '
 -	rm .git/objects/$(test_oid_to_path $bogus_sha1)
++	rm .git/objects/$(test_oid_to_path $bogus_short_sha1)
+ '
+ 
 -bogus_type="abcdefghijklmnopqrstuvwxyz1234679"
 -bogus_content="bogus"
 -bogus_size=$(strlen "$bogus_content")
 -bogus_sha1=$(echo_without_newline "$bogus_content" | git hash-object -t $bogus_type --literally -w --stdin)
 -
  test_expect_success "Type of broken object is correct when type is large" '
 -	echo $bogus_type >expect &&
 -	git cat-file -t --allow-unknown-type $bogus_sha1 >actual &&
 +	echo $bogus_long_type >expect &&
 +	git cat-file -t --allow-unknown-type $bogus_long_sha1 >actual &&
  	test_cmp expect actual
  '
These look sensible, but ...

- 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?
+ test_expect_success 'clean up broken object' '
 -	rm .git/objects/$(test_oid_to_path $bogus_sha1)
++	rm .git/objects/$(test_oid_to_path $bogus_long_sha1)
  '
Again, this one makes sense.

Thanks.

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

From: Jeff King <hidden>
Date: 2021-10-08 02:25:54

On Thu, Oct 07, 2021 at 02:37:02PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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.
quoted
- 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help