{"thread":{"id":"8489","subject":"[PATCH] fsck: do not crash on tag objects which do not contain an empty line","startedAt":"2007-06-07T22:40:12Z","lastAt":"2007-06-08T08:32:03Z","messageCount":10,"participants":["Johannes Schindelin","Johan Herland","Nicolas Pitre","Junio C Hamano","Andy Parkins"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"44272","messageId":"Pine.LNX.4.64.0706072338260.4046@racer.site","threadId":"8489","inReplyTo":null,"subject":"[PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T22:40:12Z","receivedAt":"2007-06-07T22:40:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe first empty line in a tag object separates the header from the\nmessage. If the tag object has no empty line, do not crash, but\ncomplain loudly instead.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tI have no idea how this tag crept into one of my repos, but it is \n\tno good to crash for git-fsck.\n\n builtin-fsck.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 7a92e47..607136a 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -545,9 +545,14 @@ static void fsck_verify_ref_to_tag_object(const char *refname, struct object *ob\n {\n \t/* Verify that refname matches the name stored in obj's \"tag\" header */\n \tstruct tag *tagobj = (struct tag *) parse_object(obj->sha1);\n-\tsize_t tagname_len = strlen(tagobj->tag);\n+\tsize_t tagname_len;\n \tsize_t refname_len = strlen(refname);\n \n+\tif (!tagobj->tag) {\n+\t\terror(\"tag %s does not contain any tag?\", refname);\n+\t\treturn;\n+\t}\n+\ttagname_len = strlen(tagobj->tag);\n \tif (!tagname_len) return; /* No tag name stored in tagobj. Nothing to do. */\n \n \tif (tagname_len < refname_len &&\n-- \n1.5.2.1.2683.gab86-dirty\n"},{"id":"44281","messageId":"200706080108.50042.johan@herland.net","threadId":"8489","inReplyTo":"Pine.LNX.4.64.0706072338260.4046@racer.site","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-07T23:08:49Z","receivedAt":"2007-06-07T23:08:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 08 June 2007, Johannes Schindelin wrote:\n> \n> The first empty line in a tag object separates the header from the\n> message. If the tag object has no empty line, do not crash, but\n> complain loudly instead.\n\nAren't tag objects _required_ to have an empty line separating the headers \nfrom the body? At least I wrote the new tag code with that assumption in \nmind.\n\nCould this be related to the \"error: char103: premature end of data\" you're \nseeing?\n\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44283","messageId":"Pine.LNX.4.64.0706080011430.4046@racer.site","threadId":"8489","inReplyTo":"200706080108.50042.johan@herland.net","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T23:13:55Z","receivedAt":"2007-06-07T23:13:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Jun 2007, Johan Herland wrote:\n\n> On Friday 08 June 2007, Johannes Schindelin wrote:\n> > \n> > The first empty line in a tag object separates the header from the \n> > message. If the tag object has no empty line, do not crash, but \n> > complain loudly instead.\n> \n> Aren't tag objects _required_ to have an empty line separating the \n> headers from the body? At least I wrote the new tag code with that \n> assumption in mind.\n\nYes, evidently you did.\n\nBut even then, isn't it always better to not rely on such assumptions, but \nfail gracefully? The rest of Git's source code seems to be nicer to \nfailures as this one, IMHO.\n\n> Could this be related to the \"error: char103: premature end of data\" \n> you're seeing?\n\nDefinitely. It breaks even _fetching_.\n\nCiao,\nDscho\n"},{"id":"44284","messageId":"200706080128.48637.johan@herland.net","threadId":"8489","inReplyTo":"Pine.LNX.4.64.0706080011430.4046@racer.site","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-07T23:28:48Z","receivedAt":"2007-06-07T23:28:48Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 08 June 2007, Johannes Schindelin wrote:\n> Hi,\n> \n> On Fri, 8 Jun 2007, Johan Herland wrote:\n> \n> > On Friday 08 June 2007, Johannes Schindelin wrote:\n> > > \n> > > The first empty line in a tag object separates the header from the \n> > > message. If the tag object has no empty line, do not crash, but \n> > > complain loudly instead.\n> > \n> > Aren't tag objects _required_ to have an empty line separating the \n> > headers from the body? At least I wrote the new tag code with that \n> > assumption in mind.\n> \n> Yes, evidently you did.\n> \n> But even then, isn't it always better to not rely on such assumptions, but \n> fail gracefully? The rest of Git's source code seems to be nicer to \n> failures as this one, IMHO.\n\nI agree that we should fail gracefully, and my code is clearly not doing \nthat in this case. My bad.\n\nBut the code should also detect invalid tag objects, and in this case I'm \nnot yet convinced that the tag object causing the failure is in fact valid. \nIf someone can convince me that the blank line after headers is optional, \nthen I'll gladly fix the code.\n\n> > Could this be related to the \"error: char103: premature end of data\" \n> > you're seeing?\n> \n> Definitely. It breaks even _fetching_.\n\nSorry again. Still, if I could get a look at the object that'd help me alot \nin debugging.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44287","messageId":"Pine.LNX.4.64.0706080034510.4046@racer.site","threadId":"8489","inReplyTo":"200706080128.48637.johan@herland.net","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T23:38:20Z","receivedAt":"2007-06-07T23:38:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Jun 2007, Johan Herland wrote:\n\n> On Friday 08 June 2007, Johannes Schindelin wrote:\n> > \n> > On Fri, 8 Jun 2007, Johan Herland wrote:\n> > \n> > > On Friday 08 June 2007, Johannes Schindelin wrote:\n> > > > \n> > > > The first empty line in a tag object separates the header from the \n> > > > message. If the tag object has no empty line, do not crash, but \n> > > > complain loudly instead.\n> > > \n> > > Aren't tag objects _required_ to have an empty line separating the \n> > > headers from the body? At least I wrote the new tag code with that \n> > > assumption in mind.\n> > \n> > Yes, evidently you did.\n> > \n> > But even then, isn't it always better to not rely on such assumptions, but \n> > fail gracefully? The rest of Git's source code seems to be nicer to \n> > failures as this one, IMHO.\n> \n> I agree that we should fail gracefully, and my code is clearly not doing \n> that in this case. My bad.\n> \n> But the code should also detect invalid tag objects, and in this case I'm \n> not yet convinced that the tag object causing the failure is in fact valid. \n\nIT BROKE GIT IN A REPO THAT WAS PERFECTLY VALID BEFORE, AND I COULD NOT DO \nANYTHING AFTER THAT CHANGE.\n\nThank you very much again.\n\n> If someone can convince me that the blank line after headers is optional, \n> then I'll gladly fix the code.\n\nConvincing enough?\n\n> > > Could this be related to the \"error: char103: premature end of data\" \n> > > you're seeing?\n> > \n> > Definitely. It breaks even _fetching_.\n> \n> Sorry again. Still, if I could get a look at the object that'd help me alot \n> in debugging.\n\nobject f90084c7b53b1c2fb4606acafd84ef8a748a7d78\ntype commit\ntag start\ntagger me \n\nBut I have to say that I am unlikely to review any fix you make, if that \nfix is as unreadable as those mega long lines with funny spaces in the \nmiddle of the line in that mega patch that unfortunately was already \napplied in next.\n\nI need a beer,\nDscho\n"},{"id":"44298","messageId":"alpine.LFD.0.99.0706072158160.12885@xanadu.home","threadId":"8489","inReplyTo":"200706080128.48637.johan@herland.net","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-06-08T02:04:22Z","receivedAt":"2007-06-08T02:04:22Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 8 Jun 2007, Johan Herland wrote:\n\n> I agree that we should fail gracefully, and my code is clearly not doing \n> that in this case. My bad.\n> \n> But the code should also detect invalid tag objects, and in this case I'm \n> not yet convinced that the tag object causing the failure is in fact valid. \n> If someone can convince me that the blank line after headers is optional, \n> then I'll gladly fix the code.\n\nThat's irrelevant.\n\nBecause you are fed invalid data is no excuse for crashing.\n\nEspecially in a tool like fsck, you should _expect_ and cope with \ninvalid data. That's why it exists in the first place: to identify such\ndata.\n\n\nNicolas\n"},{"id":"44300","messageId":"200706080414.24388.johan@herland.net","threadId":"8489","inReplyTo":"alpine.LFD.0.99.0706072158160.12885@xanadu.home","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-08T02:14:24Z","receivedAt":"2007-06-08T02:14:24Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 08 June 2007, Nicolas Pitre wrote:\n> On Fri, 8 Jun 2007, Johan Herland wrote:\n> \n> > I agree that we should fail gracefully, and my code is clearly not doing \n> > that in this case. My bad.\n> > \n> > But the code should also detect invalid tag objects, and in this case I'm \n> > not yet convinced that the tag object causing the failure is in fact valid. \n> > If someone can convince me that the blank line after headers is optional, \n> > then I'll gladly fix the code.\n> \n> That's irrelevant.\n> \n> Because you are fed invalid data is no excuse for crashing.\n> \n> Especially in a tool like fsck, you should _expect_ and cope with \n> invalid data. That's why it exists in the first place: to identify such\n> data.\n\nOf course. If you read my first sentence in the quote above, you will see\nthat I agree with you 100%.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44307","messageId":"7vmyzbm3cx.fsf@assigned-by-dhcp.cox.net","threadId":"8489","inReplyTo":"Pine.LNX.4.64.0706072338260.4046@racer.site","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-08T05:17:50Z","receivedAt":"2007-06-08T05:17:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The first empty line in a tag object separates the header from the\n> message. If the tag object has no empty line, do not crash, but\n> complain loudly instead.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n>\n> \tI have no idea how this tag crept into one of my repos, but it is \n> \tno good to crash for git-fsck.\n\nAbsolutely. git-fsck is to find suspicious data and should not\ncrash on them.\n\nThanks.\n\nBut it _does_ make me wonder how. Recent git-tag does this\n\n ( printf 'object %s\\ntype %s\\ntag %s\\ntagger %s\\n\\n' \\\n\t\"$object\" \"$type\" \"$name\" \"$tagger\";\n cat \"$GIT_DIR\"/TAG_FINALMSG ) >\"$GIT_DIR\"/TAG_TMP\n\nso even if TAG_FINALMSG (which is -m or edited message after\nfiltering out the comments and git-stripspace) is empty, you\nwould have the two LFs at the end of the header. Ancient\n\"git-tag-script\" did this, which is a moral equivalent with echo\nand has the same effect:\n\n ( echo -e \"object $object\\ntype $type\\ntag $name\\ntagger $tagger\\n\";\n cat .tagmsg ) > .tmp-t\n\nDid this tag come from cvsimport (or svnimport) perhaps? We are\nin the process of updating its use of git-mktag to git-tag, but\nthey use git-mktag and do not leave a blank line after the\nheader (and they do not add any body).\n\nI notice that gitk creates lightweight tag by hand (it directly\ngoes to the filesystem) It should probably use git-tag as well.\n\nI agree with you that if something does not have body, we should\nnot require an empty trailing line after the header.\n"},{"id":"44310","messageId":"Pine.LNX.4.64.0706080621420.4059@racer.site","threadId":"8489","inReplyTo":"7vmyzbm3cx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-08T05:30:13Z","receivedAt":"2007-06-08T05:30:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 7 Jun 2007, Junio C Hamano wrote:\n\n> Recent git-tag does this\n> \n> ( printf 'object %s\\ntype %s\\ntag %s\\ntagger %s\\n\\n' \\\n> \t\"$object\" \"$type\" \"$name\" \"$tagger\";\n> cat \"$GIT_DIR\"/TAG_FINALMSG ) >\"$GIT_DIR\"/TAG_TMP\n> \n> so even if TAG_FINALMSG (which is -m or edited message after\n> filtering out the comments and git-stripspace) is empty, you\n> would have the two LFs at the end of the header.\n\nI could imagine that there is still a git-stripspace after that. Undoing \nthe last LF.\n\n> Did this tag come from cvsimport (or svnimport) perhaps?\n\nYep. cvsimport.\n\nBut nevertheless, I know the convention of \"LFLF\" being the end of the \nheader from several places: HTTP, mail, proxies.\n\n_None_ of them fail as badly as what I experienced. Heck, Git used to be \nthe _most_ stable software I ever used, _including_ the Linux kernel. And \nall of a sudden, when I really needed it just to work properly, and did \nnot bother to install a \"stable\" version, but just took \"next\", it failed \non me.\n\nI don't like this at all.\n\nAnd I have to say, even if I was responsible for my share of \nnot-quite-perfect patches, this patch _series_ is bad IMHO.\n\nNot only does already the _first_ patch contain a serious error. I did not \neven bother to read it in total, because it was _huge_, contained \n_white-space_ fixes in combination with some dubious comment changes, did \nnot clean up _obvious_ errors (like lookup_object(sha1)->object!!), but at \nthe same time _changed_ behaviour.\n\nAnd it is totally unreadable, with 120+ characters per line.\n\nFrankly, I am angry at myself that I did not have a look and shot that \npatch down on the merits of its _line size_ alone.\n\nI have come to expect bad code from bad style and I would have been right \nagain, had I read the patch in time.\n\n> I agree with you that if something does not have body, we should not \n> require an empty trailing line after the header.\n\nIt is just _one_ issue about it.\n\nWe should not fail to process a fetch _at all_, just because a single \n_tag_ is not really conforming to some obscure convention.\n\nCiao,\nDscho \"still somewhat upset\"\n"},{"id":"44323","messageId":"200706080932.05098.andyparkins@gmail.com","threadId":"8489","inReplyTo":"Pine.LNX.4.64.0706072338260.4046@racer.site","subject":"Re: [PATCH] fsck: do not crash on tag objects which do not contain an empty line","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-06-08T08:32:03Z","receivedAt":"2007-06-08T08:32:03Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007 June 07, Johannes Schindelin wrote:\n\n> +\t\terror(\"tag %s does not contain any tag?\", refname);\n\nSeems a bad error message to me. Also, why is it a question?\n\n git: tag does not contain a tag\n user: WTF?\n\nPerhaps, \"The annotated tag, %s, does not contain a message\", or similar?\n\nAlso - is this really something that should be complained about? I'm \nwondering why it is important that tag objects contain a message? For \nexample, tagging a release as v1.0.0 - it's important to record who tagged \nit, and when it was tagged, but I can certainly envisage there not being a \nmessage to go with it. Most tags in the git repository are just a repeat of \nthe tag name in the message.\n\n\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"}]}