| Message ID | patch-v6-03.22-d442a309178-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:57:58PM +0200, Ævar Arnfjörð Bjarmason wrote: > Test for what happens when the -t and -s flags are asked to operate on > a missing object, this extends tests added in 3e370f9faf0 (t1006: add > tests for git cat-file --allow-unknown-type, 2015-05-03). The -t and > -s flags are the only ones that can be combined with > --allow-unknown-type, so let's test with and without that flag. I'm a little skeptical to have tests for all four pairs of `-t` or `-s` and "with `--allow-unknown-type` and without `--allow-unknown-type`". Testing both the presence and absence of `--allow-unknown-type` seems useful to me, but I'm not sure what testing `-t` and `-s` separately buys us. (If you really feel the need test both, I'd encourage looping like: for arg in -t -s do test_must_fail git cat-file $arg $missing_oid >out 2>err && test_must_be_empty out && test_cmp expect.err err && test_must_fail git cat-file $arg --allow-unknown-type $missing_oid >out 2>err && test_must_be_empty out && test_cmp expect.err err done && but I would be equally or perhaps even happier to just have one of the two tests). Thanks, Taylor
On Thu, Sep 16, 2021 at 03:57:30PM -0400, Taylor Blau wrote: > On Tue, Sep 07, 2021 at 12:57:58PM +0200, Ævar Arnfjörð Bjarmason wrote: > > Test for what happens when the -t and -s flags are asked to operate on > > a missing object, this extends tests added in 3e370f9faf0 (t1006: add > > tests for git cat-file --allow-unknown-type, 2015-05-03). The -t and > > -s flags are the only ones that can be combined with > > --allow-unknown-type, so let's test with and without that flag. > > I'm a little skeptical to have tests for all four pairs of `-t` or `-s` > and "with `--allow-unknown-type` and without `--allow-unknown-type`". Ah. Reading the next patch makes me feel even more certain of this advice. Consider squashing this and the next patch with my suggestion to use a loop below? Thanks, Taylor
On Thu, Sep 16 2021, Taylor Blau wrote: > On Tue, Sep 07, 2021 at 12:57:58PM +0200, Ævar Arnfjörð Bjarmason wrote: >> Test for what happens when the -t and -s flags are asked to operate on >> a missing object, this extends tests added in 3e370f9faf0 (t1006: add >> tests for git cat-file --allow-unknown-type, 2015-05-03). The -t and >> -s flags are the only ones that can be combined with >> --allow-unknown-type, so let's test with and without that flag. > > I'm a little skeptical to have tests for all four pairs of `-t` or `-s` > and "with `--allow-unknown-type` and without `--allow-unknown-type`". > > Testing both the presence and absence of `--allow-unknown-type` seems > useful to me, but I'm not sure what testing `-t` and `-s` separately > buys us. > > (If you really feel the need test both, I'd encourage looping like: Thanks, I'll try to simplify it. > for arg in -t -s > do > test_must_fail git cat-file $arg $missing_oid >out 2>err && > test_must_be_empty out && > test_cmp expect.err err && > > test_must_fail git cat-file $arg --allow-unknown-type $missing_oid >out 2>err && > test_must_be_empty out && > test_cmp expect.err err > done && > > but I would be equally or perhaps even happier to just have one of the > two tests). A loop like that can be further simplified as just (just inlining arg=-s): test_must_fail git cat-file -s $missing_oid >out 2>err && test_must_be_empty out && test_cmp expect.err err && test_must_fail git cat-file -s --allow-unknown-type $missing_oid >out 2>err && test_must_be_empty out && test_cmp expect.err err :) I.e. unless you end &&-chains in loops in the test framework with an || return 1 you're only testing your last iteration. Aside from whatever I'm doing here I generally prefer to either just spell it out twice (if small enough), or: for arg in -t -s do test_expect_success '...' "[... use $arg ...]" done Which both nicely get around the issue of that easy-to-make mistake. We've got some in-tree tests that are broken this way, well, at least 4cf67869b2a (list-objects.c: don't segfault for missing cmdline objects, 2018-12-05). But I think I'll leave that for a #leftoverbits submission given my outstanding patch queue..., oh there's another one in t1010-mktree.sh ... :)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 18b3779ccb6..3a7b138fe4e 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -315,6 +315,33 @@ test_expect_success '%(deltabase) reports packed delta bases' ' } ' +missing_oid=$(test_oid deadbeef) +test_expect_success 'error on type of missing object' ' + cat >expect.err <<-\EOF && + fatal: git cat-file: could not get object info + EOF + test_must_fail git cat-file -t $missing_oid >out 2>err && + test_must_be_empty out && + test_cmp expect.err err && + + test_must_fail git cat-file -t --allow-unknown-type $missing_oid >out 2>err && + test_must_be_empty out && + test_cmp expect.err err +' + +test_expect_success 'error on size of missing object' ' + cat >expect.err <<-\EOF && + fatal: git cat-file: could not get object info + EOF + test_must_fail git cat-file -s $missing_oid >out 2>err && + test_must_be_empty out && + test_cmp expect.err err && + + test_must_fail git cat-file -s --allow-unknown-type $missing_oid >out 2>err && + test_must_be_empty out && + test_cmp expect.err err +' + bogus_type="bogus" bogus_content="bogus" bogus_size=$(strlen "$bogus_content")
Test for what happens when the -t and -s flags are asked to operate on a missing object, this extends tests added in 3e370f9faf0 (t1006: add tests for git cat-file --allow-unknown-type, 2015-05-03). The -t and -s flags are the only ones that can be combined with --allow-unknown-type, so let's test with and without that flag. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t1006-cat-file.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)