Re: [PATCH 2/4] fsck: check rev-index checksums

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 18, 2023, 14:51 UTC
Message-ID: <ZD6ua4dSynRWmW2a@nand.local>
In-Reply-To: <2628249e-fe9a-d15c-5414-33d815b35cd1@github.com>

On Tue, Apr 18, 2023 at 10:27:57AM -0400, Derrick Stolee wrote:

Show 8 quoted lines

> >> +test_expect_success 'fsck catches invalid checksum' '
> >> +	revfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&
> >
> > Would this test be tighter if we introduced a sub-shell and cd'd into
> > "corrupt" here?
>
> corrupt_rev_and_verify does the subshell thing. Why should we do that
> here in the test?

I was thinking that it might be more concise if you moved the subshell to the test and out of corrupt_rev_and_verify. In addition to making corrupt_rev_and_verify work in other instances where the repository isn't required to be in a directory named "corrupt", I think it simplifies the result.

Here's what I was thinking, as a diff on top of this patch:

--- >8 ---

diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
index 6b7c709a1f..7dfbaf6b37 100755
--- a/t/t5325-reverse-index.sh
+++ b/t/t5325-reverse-index.sh
@@ -160,29 +160,30 @@ test_expect_success 'set up rev-index corruption tests' '
 '

 corrupt_rev_and_verify () {
-	(
-		pos="$1" &&
-		value="$2" &&
-		error="$3" &&
+	pos="$1" &&
+	value="$2" &&
+	error="$3" &&

-		cd corrupt &&
-		revfile=$(ls .git/objects/pack/pack-*.rev) &&
+	revfile=$(ls .git/objects/pack/pack-*.rev) &&

-		# Reset to original rev-file.
-		cp $revfile.bak $revfile &&
+	# Reset to original rev-file.
+	cp $revfile.bak $revfile &&

-		printf "$value" | dd of=$revfile bs=1 seek="$pos" conv=notrunc &&
-		test_must_fail git fsck 2>err &&
-		grep "$error" err
-	)
+	printf "$value" | dd of=$revfile bs=1 seek="$pos" conv=notrunc &&
+	test_must_fail git fsck 2>err &&
+	grep "$error" err
 }

 test_expect_success 'fsck catches invalid checksum' '
-	revfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&
-	orig_size=$(wc -c <$revfile) &&
-	hashpos=$((orig_size - 10)) &&
-	corrupt_rev_and_verify $hashpos bogus \
-		"invalid checksum"
+	(
+		cd corrupt &&
+
+		revfile=$(ls .git/objects/pack/pack-*.rev) &&
+		orig_size=$(wc -c <$revfile) &&
+		hashpos=$((orig_size - 10)) &&
+		corrupt_rev_and_verify $hashpos bogus \
+			"invalid checksum"
+	)
 '

 test_done
--- 8< ---

If you do take my suggestion, make sure to remember to come back in
patches 3/4 and 4/4 and adjust those instances of
'corrupt_rev_and_verify' to first change into "corrupt".

Thanks,
Taylor