| Message ID | patch-v6-05.22-82db40ebf8a-20210907T104559Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fsck: lib-ify object-file.c & better fsck "invalid object" error reporting | expand |
On Tue, Sep 07, 2021 at 12:58:00PM +0200, Ævar Arnfjörð Bjarmason wrote: > 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. That's not to say I'm not interested in having tests fail when we don't handle corrupt objects correctly, but more to say that 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. Assuming that this is a useful direction, though... > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > 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? > + 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. Thanks, Taylor
On Thu, Sep 16 2021, Taylor Blau wrote: > On Tue, Sep 07, 2021 at 12:58:00PM +0200, Ævar Arnfjörð Bjarmason wrote: >> 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? > [...] >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> 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. >> + 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*
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 && + 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 +' + test_done
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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t6115-rev-list-du.sh | 11 +++++++++++ 1 file changed, 11 insertions(+)