Re: [PATCH v6 05/22] rev-list tests: test for behavior with invalid object types

On Thu, Sep 16 2021, Taylor Blau wrote:

On Tue, Sep 07, 2021 at 12:58:00PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Fix a blindspot in the tests for the "rev-list --disk-usage" feature
added in 16950f8384a (rev-list: add --disk-usage option for
calculating disk usage, 2021-02-09) to test for what happens when it's
asked to calculate the disk usage of invalid object types.

I'm not sure that I agree this is a blindspot, or at least one worth
testing. Is the goal to add tests to every Git command that might have
to do something with a corrupt object and make sure that it is handled
correctly?

I'm not sure that doing so would be useful, or at the very least that
it would be worth the effort. [...] I think there are so many parts of
Git that might touch a corrupt object that trying to test all of them
seems like a losing battle.

I'll drop it since it doesn't have anything directly to do with this
series. This slipped in from the work I meant to follow-up after this
with.

This isn't just any random command that might come across an invalid
object though, it's specifically reporting object sizes. Once we change
that to not die we'll we'll want to see how invalid objects are handled
by it. Will the disk size be reported as -1? 0? ~0?

[...]

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/t6115-rev-list-du.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh
index b4aef32b713..edb2ed55846 100755
--- a/t/t6115-rev-list-du.sh
+++ b/t/t6115-rev-list-du.sh

@@ -48,4 +48,15 @@ check_du HEAD
 check_du --objects HEAD
 check_du --objects HEAD^..HEAD

+test_expect_success 'setup garbage repository' '
+	git clone --bare . garbage.git &&

Since this is cloned within the working directory, should we bother to
clean this up to avoid munging with future tests?

In general (and I had some other replies with this) I think no, if a an
individual test is picking a unique name for its data it doesn't need to
bother with test_when_finished, it can just leave the cleanup to the
eventual trash directory cleanup.

quoted

+	garbage_oid=$(git -C garbage.git hash-object -t garbage -w --stdin --literally <one.t) &&
+	git -C garbage.git rev-list --objects --all --disk-usage &&
+
+	# Manually create a ref because "update-ref", "tag" etc. have
+	# no corresponding --literally option.
+	echo $garbage_oid >garbage.git/refs/tags/garbage-tag &&
+	test_must_fail git -C garbage.git rev-list --objects --all --disk-usage

See also my earlier comment about this being much more readable in a
sub-shell.

*nod*