Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> One of the most important use cases for the strict tag object checking
> is when transfer.fsckobjects is set to true to catch invalid objects
> early on. This new regression test essentially tests the same code path
> by directly calling 'index-pack --strict' on a pack containing an
> tag object without a 'tagger' line.
>
> Technically, this test is not enough: it only exercises a code path that
> *warns*, not one that *fails*. The reason is that it would be exquisitely
> convoluted to test that: not only hash-object, but also pack-index
> actually *parse* tag objects when encountering them. Therefore we would
> have to actively *break* pack-index in order to test this. Or rewrite
> both hash-object and pack-index in shell script. Ain't gonna happen.

It is perfectly OK to feel and even say "I am not going to do that
in this series" here, but is not very welcome to cast such a
negative "Ain't gonna happen." attitude in stone in the log message
in our history.

When our toolset has become too tight without leaving enough escape
hatch to hinder further development, it is very sensible to at least
think about adding a new "--for-debug" option to hash-object and
pack-objects that allows us to deliberately create broken
datastreams to test against.

I think peff recently added helpers to t5303 to allow us test
corrupt pack streams, which is a way different from what you
envisioned above (i.e. "actively break pack-index") to test
breakages like the ones you were trying to test here.

Having said all that, I do think the series that added new warnings,
errors and a test for the new warnings is an improvement without a
test for the new errors.  So let's queue this, see if somebody comes
up a way to check for these error detection codepath on top of this
series.

Thanks.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t5302-pack-index.sh | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
> index 4bbb718..4d033df 100755
> --- a/t/t5302-pack-index.sh
> +++ b/t/t5302-pack-index.sh
> @@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object store' '
>      test -f .git/objects/pack/pack-${pack1}.idx
>  '
>  
> +test_expect_success 'index-pack --strict warns upon missing tagger in tag' '
> +    sha=$(git rev-parse HEAD) &&
> +    cat >wrong-tag <<EOF &&
> +object $sha
> +type commit
> +tag guten tag
> +
> +This is an invalid tag.
> +EOF
> +
> +    tag=$(git hash-object -t tag -w --stdin <wrong-tag) &&
> +    pack1=$(echo $tag $sha | git pack-objects tag-test) &&
> +    echo remove tag object &&
> +    thirtyeight=${tag#??} &&
> +    rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight &&
> +    git index-pack --strict tag-test-${pack1}.pack 2> err &&

s/2> err/2>err/;

> +    grep "^error:.* expected .tagger. line" err
> +'
> +
>  test_done