Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects
Hi Junio, On Thu, 11 Sep 2014, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: 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. [...] It wasn't too painful to do one of them, and the result looks rather nice. I was loathe to make it easier for interested parties to create invalid Git objects and to push them onto servers that cannot yet benefit from my patch series. At the very least, I would have preferred to put such functionality into test-* executables (where I searched for that functionality in the first place), i.e. outside the distributed binaries. But since you already did the work and it does the job, I won't worry about it. A bigger worry is that the additional test verifies that fsck catches the invalid tag object and exits, when we really want to be certain that git fetch --strict will abort on such an object. So the test is still indirect, although I admit that it is closer now to what we want. Version 4 of the patch series (without your hash-object --literally patch because mailed patch series cannot declare on what branches from 'pu' they rely, I always base my patch series on 'next' for that reason [*1*]) coming up. Ciao, Dscho Footnote *1*: As always, I push my patch series to a topic branch on GitHub. The one corresponding to the upcoming patch series is in https://github.com/dscho/git/compare/next...fsck-tag, the one with your additional test in https://github.com/dscho/git/compare/next...fsck-tag-plus (the latter being a thicket rather than a linear topic branch). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects
Johannes Schindelin johannes.schinde...@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.schinde...@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/2err/;
+grep ^error:.* expected .tagger. line err
+'
+
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects
Junio C Hamano gits...@pobox.com writes: 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. It wasn't too painful to do one of them, and the result looks rather nice. It lets us add this patch on top of your series. -- 8 -- Subject: [PATCH] t1450: make sure fsck detects a malformed tagger line With hash-object --literally, write a tag object that is not supposed to pass one of the new checks added to fsck, and make sure that the new check catches the breakage. Signed-off-by: Junio C Hamano gits...@pobox.com --- t/t1450-fsck.sh | 19 +++ 1 file changed, 19 insertions(+) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 9118253..6ecb844 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -234,6 +234,25 @@ test_expect_success 'tag with incorrect tag name missing tagger' ' grep expected .tagger. line out ' +test_expect_success 'tag with bad tagger' ' + sha=$(git rev-parse HEAD) + cat wrong-tag -EOF + object $sha + type commit + tag not-quite-wrong + tagger Bad Tagger Name + + This is an invalid tag. + EOF + + tag=$(git hash-object --literally -t tag -w --stdin wrong-tag) + test_when_finished remove_object $tag + echo $tag .git/refs/tags/wrong + test_when_finished git update-ref -d refs/tags/wrong + test_must_fail git fsck --tags 2out + grep error in tag .*: invalid author/committer out +' + test_expect_success 'cleaned up' ' git fsck actual 21 test_cmp empty actual -- 2.1.0-459-g1bc3b2b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
