{"thread":{"id":"8491","subject":"error: char103: premature end of data","startedAt":"2007-06-07T22:50:42Z","lastAt":"2007-06-11T01:11:02Z","messageCount":90,"participants":["Johannes Schindelin","Johan Herland","Junio C Hamano","Alex Riesen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"44276","messageId":"Pine.LNX.4.64.0706072348110.4046@racer.site","threadId":"8491","inReplyTo":null,"subject":"error: char103: premature end of data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T22:50:42Z","receivedAt":"2007-06-07T22:50:42Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI just tried to fetch from one of my repos which was perfectly usable (and \nfetchable) before, and got this error message:\n\n\terror: char103: premature end of data\n\nWTF? Since when did we stop using non-cryptic error messages?\n\nNeedless to say, I am very unhappy with such a message, especially in a \nrepo which worked perfectly, thank you.\n\nI somehow suspect that this has something to do with the recent work on \nthe tag objects. If so, NACK on that patch series from me.\n\nCiao,\nDscho\n"},{"id":"44280","messageId":"200706080105.28102.johan@herland.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706072348110.4046@racer.site","subject":"Re: error: char103: premature end of data","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-07T23:05:27Z","receivedAt":"2007-06-07T23:05:27Z","isPatch":false,"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> I just tried to fetch from one of my repos which was perfectly usable (and \n> fetchable) before, and got this error message:\n> \n> \terror: char103: premature end of data\n\nSorry about that. Do you have an idea of which tag object caused the \nfailure? If so, could you send the output of \"git-cat-file tag \" on \nit?\n\ngit-fsck on the repo should report the same error. If you run with -v \n(verbose) you should also get some hints as to which tag object causes \nthis.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44282","messageId":"Pine.LNX.4.64.0706080003470.4046@racer.site","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706072348110.4046@racer.site","subject":"Re: error: char103: premature end of data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T23:11:03Z","receivedAt":"2007-06-07T23:11:03Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 7 Jun 2007, Johannes Schindelin wrote:\n\n> I somehow suspect that this has something to do with the recent work on \n> the tag objects. If so, NACK on that patch series from me.\n\nOkay. Instead of doing the work I have to do, which I cannot, because \na vital part of git stopped working for me, I tracked down that it is \nindeed that monster commit v1.5.2.1-134-gc7de6eb AKA ':/Refactor git tag \nobjects;'\n\nI have to repeat my NACK on it. I did not have time to review it, when \nyou sent it, so I tried to find what is wrong now (when I don't have \ntime for it to begin with).\n\nBut it is really intrusive, introduces problems, is less than readable, \nand it is not at all clear what problems it solves, while it looks like it \ncould be broken down to easily-reviewable fixes, cleanups, and changes, so \nplease fix it while I recover from my troubles.\n\nCiao,\nDscho\n"},{"id":"44285","messageId":"Pine.LNX.4.64.0706080023450.4046@racer.site","threadId":"8491","inReplyTo":"200706080105.28102.johan@herland.net","subject":"Re: error: char103: premature end of data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T23:28:14Z","receivedAt":"2007-06-07T23:28:14Z","isPatch":false,"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> > I just tried to fetch from one of my repos which was perfectly usable \n> > (and fetchable) before, and got this error message:\n> > \n> > \terror: char103: premature end of data\n> \n> Sorry about that. Do you have an idea of which tag object caused the \n> failure? If so, could you send the output of \"git-cat-file tag \" \n> on it?\n\nYes, I know what causes it. A tag with an empty message.\n\nAnd I even know why it does that. It's easy. Look into git-tag.sh, and you \nwill find that it does a git-stripspace on the final message. If that was \nempty, then the tag will just be the tag header.\n\n> git-fsck on the repo should report the same error. If you run with -v \n> (verbose) you should also get some hints as to which tag object causes \n> this.\n\nYes, it finds the error. And crashes. And costs me time.\n\nWhy do we have to parse _everything_ in the tag to begin with? It's not \nlike I will show the information of the darn thing when I just fetch it \nfrom repo A into repo B.\n\nCiao,\nDscho\n"},{"id":"44289","messageId":"200706080147.50207.johan@herland.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706080023450.4046@racer.site","subject":"Re: error: char103: premature end of data","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-07T23:47:50Z","receivedAt":"2007-06-07T23:47:50Z","isPatch":false,"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> > > I just tried to fetch from one of my repos which was perfectly usable \n> > > (and fetchable) before, and got this error message:\n> > > \n> > > \terror: char103: premature end of data\n> > \n> > Sorry about that. Do you have an idea of which tag object caused the \n> > failure? If so, could you send the output of \"git-cat-file tag \" \n> > on it?\n> \n> Yes, I know what causes it. A tag with an empty message.\n\nOf course. Although if one uses git-tag -a to create an annotated tag object \nI have a hard time seeing why one wouldn't supply a message. But it's of \ncourse entirly valid not to supply a message.\n\n> And I even know why it does that. It's easy. Look into git-tag.sh, and you \n> will find that it does a git-stripspace on the final message. If that was \n> empty, then the tag will just be the tag header.\n\nYou're right. I'll fix my code to handle this case. I'll also try to split\nthat first patch in my series into more manageable chunks.\n\n> > git-fsck on the repo should report the same error. If you run with -v \n> > (verbose) you should also get some hints as to which tag object causes \n> > this.\n> \n> Yes, it finds the error. And crashes. And costs me time.\n> \n> Why do we have to parse _everything_ in the tag to begin with? It's not \n> like I will show the information of the darn thing when I just fetch it \n> from repo A into repo B.\n\nWe parse the tag completely in order to detect corruption/invalid tags as \nearly as possible. If I'm pulling from a corrupt repo, I'd sure as hell \nwant git to tell me when I first fetched, and not a couple of weeks later \nwhen I'd try to use the corrupt data, or call fsck, or whatever.\n\nAgain, I'm sorry for your problems.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44290","messageId":"Pine.LNX.4.64.0706080054330.4046@racer.site","threadId":"8491","inReplyTo":"200706080147.50207.johan@herland.net","subject":"Re: error: char103: premature end of data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-07T23:55:48Z","receivedAt":"2007-06-07T23:55:48Z","isPatch":false,"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> If I'm pulling from a corrupt repo, I'd sure as hell want git to tell me \n> when I first fetched, and not a couple of weeks later when I'd try to \n> use the corrupt data, or call fsck, or whatever.\n\nWe don't check that the object names match the object contents on pull. \nWith your argument, we should do that, too. And of course, get rid of \ngit-fsck, because it is of no more use, then.\n\nCiao,\nDscho\n"},{"id":"44292","messageId":"200706080208.14571.johan@herland.net","threadId":"8491","inReplyTo":"200706080147.50207.johan@herland.net","subject":"[PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\\n')","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-08T00:08:14Z","receivedAt":"2007-06-08T00:08:14Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Thanks to Johannes Schindelin for\ndiscovering this.\n\nAlso add a testcase for this condition.\n\nSigned-off-by: Johan Herland \n---\n\nThis patch should hopefully fix your problem.\n\n\n...Johan\n\n t/t3800-mktag.sh | 15 +++++++++++++++\n tag.c | 9 ++-------\n 2 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex ac9008a..ac7cbbc 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -399,5 +399,20 @@ test_expect_success \\\n 'create valid tag #4' \\\n 'git-mktag .git/refs/tags/mytag 2>message'\n \n+############################################################\n+# 24. create valid tag #4 (with empty message)\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n \n test_done\ndiff --git a/tag.c b/tag.c\nindex e371179..875145b 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -131,10 +131,6 @@ int parse_and_verify_tag_buffer(struct tag *item, const char *data, const unsign\n \t\theader_end = memchr(tagger_line, '\\n', end - tagger_line);\n \t\tif (!header_end++)\n \t\t\treturn error(\"char\" PD_FMT \": could not find \\\"\\\\n\\\" after \\\"tagger\\\"\", tagger_line - data);\n-\t\tif (end - header_end < 1)\n-\t\t\treturn error(\"char\" PD_FMT \": premature end of data\", header_end - data);\n-\t\tif (*header_end != '\\n') /* header must end with \"\\n\\n\" */\n-\t\t\treturn error(\"char\" PD_FMT \": could not find blank line after header section\", header_end - data);\n \t}\n \telse {\n \t\t/* Treat tagger line as optional */\n@@ -148,9 +144,8 @@ int parse_and_verify_tag_buffer(struct tag *item, const char *data, const unsign\n \t\t\theader_end = tagger_line;\n \t}\n \n-\tif (end - header_end < 1)\n-\t\treturn error(\"char\" PD_FMT \": premature end of data\", header_end - data);\n-\tif (*header_end != '\\n') /* header must end with \"\\n\\n\" */\n+\t/* header must end with \"\\n\\n\", but \"\\n\" is acceptable if at EOF */\n+\tif (header_end < end - 1 && *header_end != '\\n') /* not at EOF, and next char is not '\\n' */\n \t\treturn error(\"char\" PD_FMT \": could not find blank line after header section\", header_end - data);\n \n \t/*\n-- \n1.5.2\n"},{"id":"44312","messageId":"7vd507m159.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706080208.14571.johan@herland.net","subject":"Re: [PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\\n')","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-08T06:05:38Z","receivedAt":"2007-06-08T06:05:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Thanks to Johannes Schindelin for\n> discovering this.\n>\n> Also add a testcase for this condition.\n>\n> Signed-off-by: Johan Herland \n\nWhile this certainly is an improvement, I suspect that your\nparse_tag() does a little too much. In a format such as \"tag\"\nobject that does header + blank + body, it is customary to allow\nheader fields that your version does not understand (assuming\nthat such extention will go after the known fields is fine).\n\nWhich means that you should not be even saying \"Ok, I've checked all\nheaders I know about---there should be a double LF to terminate it\",\nas you do not know if headers have ended.\n"},{"id":"44322","messageId":"200706081018.34207.johan@herland.net","threadId":"8491","inReplyTo":"7vd507m159.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\\n')","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-08T08:18:34Z","receivedAt":"2007-06-08T08:18:34Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 08 June 2007, Junio C Hamano wrote:\n> Johan Herland writes:\n> \n> > Thanks to Johannes Schindelin for\n> > discovering this.\n> >\n> > Also add a testcase for this condition.\n> >\n> > Signed-off-by: Johan Herland \n> \n> While this certainly is an improvement, I suspect that your\n> parse_tag() does a little too much. In a format such as \"tag\"\n> object that does header + blank + body, it is customary to allow\n> header fields that your version does not understand (assuming\n> that such extention will go after the known fields is fine).\n> \n> Which means that you should not be even saying \"Ok, I've checked all\n> headers I know about---there should be a double LF to terminate it\",\n> as you do not know if headers have ended.\n\nOk, I'm currently working on a patch series for Dscho and others where I \nsplit up the big patch ('[PATCH 1/6] Refactor git tag objects; make \"tag\" \nheader optional; introduce new optional \"keywords\" header') into babysteps.\n\nI can:\n\n1. Provide a new patch series to totally replace the previous 6-part patch \nseries (plus bugfixes). The new patch series will make smaller steps and \nend up (hopefully) in a better place, with less overzealous \nchecking/parsing, and more \"traditional\" whitespacing.\n\nOR\n\n2. Provide the babystep-series ending up exactly where we are today (i.e. \nafter the patch series, plus bug fixes). Then, provide patches on top of \nthe existing series to get it into shape, both scope-wise (i.e. not trying \nto do too much) and whitespace-wise.\n\nWhich do you prefer?\n\n\n...Johan\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44357","messageId":"7vzm3aig7j.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706081018.34207.johan@herland.net","subject":"Re: [PATCH] Fix failed tag parsing when tag object has no body/message (and thus ends with a single '\\n')","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-08T16:06:08Z","receivedAt":"2007-06-08T16:06:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> I can:\n>\n> 1. Provide a new patch series to totally replace the previous 6-part patch \n> series (plus bugfixes). The new patch series will make smaller steps and \n> end up (hopefully) in a better place, with less overzealous \n> checking/parsing, and more \"traditional\" whitespacing.\n>\n> OR\n>\n> 2. Provide the babystep-series ending up exactly where we are today (i.e. \n> after the patch series, plus bug fixes). Then, provide patches on top of \n> the existing series to get it into shape, both scope-wise (i.e. not trying \n> to do too much) and whitespace-wise.\n>\n> Which do you prefer?\n\nI am not sure if there is any practical difference between the\ntwo ;-). But in either case, it appears that we should first\nrevert d9fa4a8 from 'next' and start from clean slate. It\nreally seems that the patch series did upset too many people;\npersonally I found the first patch still was follow-able, but I\ndo agree that it should have been much smaller and not mixing\ntoo many things into one).\n\nSo, let's do 1.\n"},{"id":"44401","messageId":"200706090210.36270.johan@herland.net","threadId":"8491","inReplyTo":"7vzm3aig7j.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH 0/21] Refactor the tag object (take 2)","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:10:36Z","receivedAt":"2007-06-09T00:10:36Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This patch series implements part of the ground work for the 'notes'\nfeature discussed earlier in the thread \"[PATCH 00/15] git-note: A\nmechanism for providing free-form after-the-fact annotations on commits\".\n\nThe following patches refactors the tag object by:\n1. Unifying parsing and verification of tag objects (patches 1-9)\n2. Do better and more thorough verification of tag objects (patches 10-13)\n3. Making the \"tagger\" header mandatory as far as possible (patch 14)\n4. Making the \"tag\" header optional (patch 15)\n5. Introducing a new optional \"keywords\" header (patch 16)\n6. Auxiliary changes supporting the above (patches 17-21)\n\nThis patch series replaces the earlier patch series of the same name\n(plus the current bugfixes on top of that series). It's also much easier\non the eyes for those with 80 chars wide displays. Also, the selftest\nsuite should run successfully at any point in this patch series.\n\nHere's the shortlog:\n\nJohan Herland (21):\n Remove unnecessary code and comments on non-existing 8kB tag object restriction\n Return error messages when parsing fails.\n Refactoring to make verify_tag() and parse_tag_buffer() more similar\n Refactor verification of \"tagger\" line to be more similar to verification of \"type\" and \"tagger\" lines\n Make parse_tag_buffer_internal() handle item == NULL\n Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'\n Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()\n Switch from verify_tag() to parse_and_verify_tag_buffer() for verifying tag objects in git-mktag\n Remove unneeded code from mktag.c\n Free mktag's buffer before dying\n Rewrite error messages; fix up line lengths\n Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers\n Collect skipping of header field names and calculation of line lengths in one place\n Add proper parsing of \"tagger\" line, but only when thorough_verify is set\n Make tag names (i.e. the tag object's \"tag\" line) optional\n Introduce optional \"keywords\" on tag objects\n Update comments on tag objects in mktag.c\n git-fsck: Do thorough verification of tag objects\n Documentation/git-mktag: Document the changes in tag object structure\n git-mktag tests: Expand on mktag selftests according to the new tag object structure\n Add fsck_verify_ref_to_tag_object() to verify that refname matches name stored in tag object\n\n Documentation/git-mktag.txt | 41 +++++--\n builtin-fsck.c | 41 +++++++\n builtin-log.c | 2 +-\n mktag.c | 147 +++++-------------------\n t/t3800-mktag.sh | 231 ++++++++++++++++++++++++++++++++++---\n tag.c | 268 +++++++++++++++++++++++++++++++++++--------\n tag.h | 5 +-\n 7 files changed, 540 insertions(+), 195 deletions(-)\n\n\nHave fun!\n\n...Johan\n"},{"id":"44402","messageId":"200706090212.37904.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 01/21] Remove unnecessary code and comments on non-existing 8kB tag object restriction","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:12:37Z","receivedAt":"2007-06-09T00:12:37Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n mktag.c | 7 -------\n 1 files changed, 0 insertions(+), 7 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 070bc96..b82e377 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -12,15 +12,8 @@\n * \"object \\n\" is 48 bytes, \"type tag\\n\" at 9 bytes is the\n * shortest possible type-line, and \"tag .\\n\" at 6 bytes is the\n * shortest single-character-tag line.\n- *\n- * We also artificially limit the size of the full object to 8kB.\n- * Just because I'm a lazy bastard, and if you can't fit a signature\n- * in that size, you're doing something wrong.\n */\n \n-/* Some random size */\n-#define MAXSIZE (8192)\n-\n /*\n * We refuse to tag something we can't verify. Just because.\n */\n-- \n1.5.2\n"},{"id":"44403","messageId":"200706090213.16031.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 02/21] Return error messages when parsing fails.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:13:15Z","receivedAt":"2007-06-09T00:13:15Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This patch brings the already similar tag.c:parse_tag_buffer() and\nmktag.c:verify_tag() a little bit closer to eachother.\n\nSigned-off-by: Johan Herland \n---\n tag.c | 39 ++++++++++++++++++++++++++++++---------\n 1 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex bbacd59..954ed8a 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -35,6 +35,12 @@ struct tag *lookup_tag(const unsigned char *sha1)\n \n int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n {\n+#ifdef NO_C99_FORMAT\n+#define PD_FMT \"%d\"\n+#else\n+#define PD_FMT \"%td\"\n+#endif\n+\n \tint typelen, taglen;\n \tunsigned char sha1[20];\n \tconst char *type_line, *tag_line, *sig_line;\n@@ -45,28 +51,41 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n item->object.parsed = 1;\n \n \tif (size < 64)\n-\t\treturn -1;\n-\tif (memcmp(\"object \", data, 7) || get_sha1_hex((char *) data + 7, sha1))\n-\t\treturn -1;\n+\t\treturn error(\"failed preliminary size check\");\n \n+\t/* Verify object line */\n+\tif (memcmp(data, \"object \", 7))\n+\t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n+\n+\tif (get_sha1_hex((char *) data + 7, sha1))\n+\t\treturn error(\"char%d: could not get SHA1 hash\", 7);\n+\n+\t/* Verify type line */\n \ttype_line = (char *) data + 48;\n-\tif (memcmp(\"\\ntype \", type_line-1, 6))\n-\t\treturn -1;\n+\tif (memcmp(type_line - 1, \"\\ntype \", 6))\n+\t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n \n+\t/* Verify tag-line */\n \ttag_line = strchr(type_line, '\\n');\n-\tif (!tag_line || memcmp(\"tag \", ++tag_line, 4))\n-\t\treturn -1;\n+\tif (!tag_line)\n+\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - (char *) data);\n+\ttag_line++;\n+\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n+\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - (char *) data);\n \n \tsig_line = strchr(tag_line, '\\n');\n \tif (!sig_line)\n \t\treturn -1;\n \tsig_line++;\n \n+\t/* Get the actual type */\n \ttypelen = tag_line - type_line - strlen(\"type \\n\");\n-\tif (typelen >= 20)\n-\t\treturn -1;\n+\tif (typelen >= sizeof(type))\n+\t\treturn error(\"char\" PD_FMT \": type too long\", type_line+5 - (char *) data);\n+\n \tmemcpy(type, type_line + 5, typelen);\n \ttype[typelen] = '\\0';\n+\n \ttaglen = sig_line - tag_line - strlen(\"tag \\n\");\n \titem->tag = xmalloc(taglen + 1);\n \tmemcpy(item->tag, tag_line + 4, taglen);\n@@ -92,6 +111,8 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \t}\n \n \treturn 0;\n+\n+#undef PD_FMT\n }\n \n int parse_tag(struct tag *item)\n-- \n1.5.2\n"},{"id":"44404","messageId":"200706090213.40785.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:13:40Z","receivedAt":"2007-06-09T00:13:40Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"A fair amount of refactoring is included in this patch, none of which\nshould affect the actual behaviour of the code in any way.\n\nHere are the changes done:\n\n- Refactor out all the (char *) casting in parse_tag_buffer(). Create a\n wrapper function (parse_tag_buffer()) that casts _once_ and then calls\n parse_tag_buffer_internal() which does the real work\n\n- Variable renaming in parse_tag_buffer_internal():\n - sig_line -> tagger_line\n - typelen -> type_len\n - taglen -> tag_len\n\n- Variable renaming in verify_tag():\n - buffer -> data\n - typelen -> type_len\n\n- Remove unnecessary variable 'object' from verify_tag(). It has always\n the same value as 'data', so use 'data' directly instead\n\n- Fix type of length variables (int -> unsigned long)\n\n- Move null-termination of tag buffer out of verify_tag() and into main().\n\nSigned-off-by: Johan Herland \n---\n mktag.c | 41 +++++++++++++++++++----------------------\n tag.c | 50 +++++++++++++++++++++++++++-----------------------\n 2 files changed, 46 insertions(+), 45 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex b82e377..0bc20c8 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -32,52 +32,48 @@ static int verify_object(unsigned char *sha1, const char *expected_type)\n \treturn ret;\n }\n \n+static int verify_tag(char *data, unsigned long size)\n+{\n #ifdef NO_C99_FORMAT\n #define PD_FMT \"%d\"\n #else\n #define PD_FMT \"%td\"\n #endif\n \n-static int verify_tag(char *buffer, unsigned long size)\n-{\n-\tint typelen;\n-\tchar type[20];\n \tunsigned char sha1[20];\n-\tconst char *object, *type_line, *tag_line, *tagger_line;\n+\tchar type[20];\n+\tconst char *type_line, *tag_line, *tagger_line;\n+\tunsigned long type_len;\n \n \tif (size < 64)\n \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n \n-\tbuffer[size] = 0;\n-\n \t/* Verify object line */\n-\tobject = buffer;\n-\tif (memcmp(object, \"object \", 7))\n+\tif (memcmp(data, \"object \", 7))\n \t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n \n-\tif (get_sha1_hex(object + 7, sha1))\n+\tif (get_sha1_hex(data + 7, sha1))\n \t\treturn error(\"char%d: could not get SHA1 hash\", 7);\n \n \t/* Verify type line */\n-\ttype_line = object + 48;\n+\ttype_line = data + 48;\n \tif (memcmp(type_line - 1, \"\\ntype \", 6))\n \t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n \n \t/* Verify tag-line */\n \ttag_line = strchr(type_line, '\\n');\n \tif (!tag_line)\n-\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - buffer);\n+\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - data);\n \ttag_line++;\n \tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n-\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - buffer);\n+\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - data);\n \n \t/* Get the actual type */\n-\ttypelen = tag_line - type_line - strlen(\"type \\n\");\n-\tif (typelen >= sizeof(type))\n-\t\treturn error(\"char\" PD_FMT \": type too long\", type_line+5 - buffer);\n-\n-\tmemcpy(type, type_line+5, typelen);\n-\ttype[typelen] = 0;\n+\ttype_len = tag_line - type_line - strlen(\"type \\n\");\n+\tif (type_len >= sizeof(type))\n+\t\treturn error(\"char\" PD_FMT \": type too long\", type_line + 5 - data);\n+\tmemcpy(type, type_line + 5, type_len);\n+\ttype[type_len] = '\\0';\n \n \t/* Verify that the object matches */\n \tif (verify_object(sha1, type))\n@@ -91,23 +87,23 @@ static int verify_tag(char *buffer, unsigned long size)\n \t\t\tbreak;\n \t\tif (c > ' ')\n \t\t\tcontinue;\n-\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - buffer);\n+\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - data);\n \t}\n \n \t/* Verify the tagger line */\n \ttagger_line = tag_line;\n \n \tif (memcmp(tagger_line, \"tagger\", 6) || (tagger_line[6] == '\\n'))\n-\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - buffer);\n+\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - data);\n \n \t/* TODO: check for committer info + blank line? */\n \t/* Also, the minimum length is probably + \"tagger .\", or 63+8=71 */\n \n \t/* The actual stuff afterwards we don't care about.. */\n \treturn 0;\n-}\n \n #undef PD_FMT\n+}\n \n int main(int argc, char **argv)\n {\n@@ -124,6 +120,7 @@ int main(int argc, char **argv)\n \t\tfree(buffer);\n \t\tdie(\"could not read from stdin\");\n \t}\n+\tbuffer[size] = 0;\n \n \t/* Verify it for some basic sanity: it needs to start with\n \t \"object \\ntype\\ntagger \" */\ndiff --git a/tag.c b/tag.c\nindex 954ed8a..8d31603 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -33,7 +33,7 @@ struct tag *lookup_tag(const unsigned char *sha1)\n return (struct tag *) obj;\n }\n \n-int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n+static int parse_tag_buffer_internal(struct tag *item, const char *data, const unsigned long size)\n {\n #ifdef NO_C99_FORMAT\n #define PD_FMT \"%d\"\n@@ -41,14 +41,14 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n #define PD_FMT \"%td\"\n #endif\n \n-\tint typelen, taglen;\n \tunsigned char sha1[20];\n-\tconst char *type_line, *tag_line, *sig_line;\n \tchar type[20];\n+\tconst char *type_line, *tag_line, *tagger_line;\n+\tunsigned long type_len, tag_len;\n \n- if (item->object.parsed)\n- return 0;\n- item->object.parsed = 1;\n+\tif (item->object.parsed)\n+\t\treturn 0;\n+\titem->object.parsed = 1;\n \n \tif (size < 64)\n \t\treturn error(\"failed preliminary size check\");\n@@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \tif (memcmp(data, \"object \", 7))\n \t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n \n-\tif (get_sha1_hex((char *) data + 7, sha1))\n+\tif (get_sha1_hex(data + 7, sha1))\n \t\treturn error(\"char%d: could not get SHA1 hash\", 7);\n \n \t/* Verify type line */\n-\ttype_line = (char *) data + 48;\n+\ttype_line = data + 48;\n \tif (memcmp(type_line - 1, \"\\ntype \", 6))\n \t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n \n \t/* Verify tag-line */\n \ttag_line = strchr(type_line, '\\n');\n \tif (!tag_line)\n-\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - (char *) data);\n+\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - data);\n \ttag_line++;\n \tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n-\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - (char *) data);\n+\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - data);\n \n-\tsig_line = strchr(tag_line, '\\n');\n-\tif (!sig_line)\n+\ttagger_line = strchr(tag_line, '\\n');\n+\tif (!tagger_line)\n \t\treturn -1;\n-\tsig_line++;\n+\ttagger_line++;\n \n \t/* Get the actual type */\n-\ttypelen = tag_line - type_line - strlen(\"type \\n\");\n-\tif (typelen >= sizeof(type))\n-\t\treturn error(\"char\" PD_FMT \": type too long\", type_line+5 - (char *) data);\n-\n-\tmemcpy(type, type_line + 5, typelen);\n-\ttype[typelen] = '\\0';\n+\ttype_len = tag_line - type_line - strlen(\"type \\n\");\n+\tif (type_len >= sizeof(type))\n+\t\treturn error(\"char\" PD_FMT \": type too long\", type_line + 5 - data);\n+\tmemcpy(type, type_line + 5, type_len);\n+\ttype[type_len] = '\\0';\n \n-\ttaglen = sig_line - tag_line - strlen(\"tag \\n\");\n-\titem->tag = xmalloc(taglen + 1);\n-\tmemcpy(item->tag, tag_line + 4, taglen);\n-\titem->tag[taglen] = '\\0';\n+\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n+\titem->tag = xmalloc(tag_len + 1);\n+\tmemcpy(item->tag, tag_line + 4, tag_len);\n+\titem->tag[tag_len] = '\\0';\n \n \tif (!strcmp(type, blob_type)) {\n \t\titem->tagged = &lookup_blob(sha1)->object;\n@@ -115,6 +114,11 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n #undef PD_FMT\n }\n \n+int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n+{\n+\treturn parse_tag_buffer_internal(item, (const char *) data, size);\n+}\n+\n int parse_tag(struct tag *item)\n {\n \tenum object_type type;\n-- \n1.5.2\n"},{"id":"44406","messageId":"200706090214.11589.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 04/21] Refactor verification of \"tagger\" line to be more similar to verification of \"type\" and \"tagger\" lines","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:14:11Z","receivedAt":"2007-06-09T00:14:11Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Also update selftests to reflect that verification of \"tagger\" now\nhappens _before_ verification of type name, object sha1 and tag name.\n\nSigned-off-by: Johan Herland \n---\n mktag.c | 16 ++++++++--------\n t/t3800-mktag.sh | 3 +++\n tag.c | 6 +++---\n 3 files changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 0bc20c8..4dbefab 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -62,12 +62,18 @@ static int verify_tag(char *data, unsigned long size)\n \n \t/* Verify tag-line */\n \ttag_line = strchr(type_line, '\\n');\n-\tif (!tag_line)\n+\tif (!tag_line++)\n \t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - data);\n-\ttag_line++;\n \tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n \t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - data);\n \n+\t/* Verify the tagger line */\n+\ttagger_line = strchr(tag_line, '\\n');\n+\tif (!tagger_line++)\n+\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", tag_line - data);\n+\tif (memcmp(tagger_line, \"tagger \", 7) || (tagger_line[7] == '\\n'))\n+\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - data);\n+\n \t/* Get the actual type */\n \ttype_len = tag_line - type_line - strlen(\"type \\n\");\n \tif (type_len >= sizeof(type))\n@@ -90,12 +96,6 @@ static int verify_tag(char *data, unsigned long size)\n \t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - data);\n \t}\n \n-\t/* Verify the tagger line */\n-\ttagger_line = tag_line;\n-\n-\tif (memcmp(tagger_line, \"tagger\", 6) || (tagger_line[6] == '\\n'))\n-\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - data);\n-\n \t/* TODO: check for committer info + blank line? */\n \t/* Also, the minimum length is probably + \"tagger .\", or 63+8=71 */\n \ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex 7c7e433..b4edb4d 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -133,6 +133,7 @@ cat >tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <\n---\n tag.c | 54 +++++++++++++++++++++++++++++-------------------------\n 1 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 19c66cd..b134967 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n \tconst char *type_line, *tag_line, *tagger_line;\n \tunsigned long type_len, tag_len;\n \n-\tif (item->object.parsed)\n-\t\treturn 0;\n-\titem->object.parsed = 1;\n+\tif (item) {\n+\t\tif (item->object.parsed)\n+\t\t\treturn 0;\n+\t\titem->object.parsed = 1;\n+\t}\n \n \tif (size < 64)\n \t\treturn error(\"failed preliminary size check\");\n@@ -85,28 +87,30 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n \tmemcpy(type, type_line + 5, type_len);\n \ttype[type_len] = '\\0';\n \n-\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n-\titem->tag = xmalloc(tag_len + 1);\n-\tmemcpy(item->tag, tag_line + 4, tag_len);\n-\titem->tag[tag_len] = '\\0';\n-\n-\tif (!strcmp(type, blob_type)) {\n-\t\titem->tagged = &lookup_blob(sha1)->object;\n-\t} else if (!strcmp(type, tree_type)) {\n-\t\titem->tagged = &lookup_tree(sha1)->object;\n-\t} else if (!strcmp(type, commit_type)) {\n-\t\titem->tagged = &lookup_commit(sha1)->object;\n-\t} else if (!strcmp(type, tag_type)) {\n-\t\titem->tagged = &lookup_tag(sha1)->object;\n-\t} else {\n-\t\terror(\"Unknown type %s\", type);\n-\t\titem->tagged = NULL;\n-\t}\n-\n-\tif (item->tagged && track_object_refs) {\n-\t\tstruct object_refs *refs = alloc_object_refs(1);\n-\t\trefs->ref[0] = item->tagged;\n-\t\tset_object_refs(&item->object, refs);\n+\tif (item) {\n+\t\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n+\t\titem->tag = xmalloc(tag_len + 1);\n+\t\tmemcpy(item->tag, tag_line + 4, tag_len);\n+\t\titem->tag[tag_len] = '\\0';\n+\n+\t\tif (!strcmp(type, blob_type)) {\n+\t\t\titem->tagged = &lookup_blob(sha1)->object;\n+\t\t} else if (!strcmp(type, tree_type)) {\n+\t\t\titem->tagged = &lookup_tree(sha1)->object;\n+\t\t} else if (!strcmp(type, commit_type)) {\n+\t\t\titem->tagged = &lookup_commit(sha1)->object;\n+\t\t} else if (!strcmp(type, tag_type)) {\n+\t\t\titem->tagged = &lookup_tag(sha1)->object;\n+\t\t} else {\n+\t\t\terror(\"Unknown type %s\", type);\n+\t\t\titem->tagged = NULL;\n+\t\t}\n+\n+\t\tif (item->tagged && track_object_refs) {\n+\t\t\tstruct object_refs *refs = alloc_object_refs(1);\n+\t\t\trefs->ref[0] = item->tagged;\n+\t\t\tset_object_refs(&item->object, refs);\n+\t\t}\n \t}\n \n \treturn 0;\n-- \n1.5.2\n"},{"id":"44408","messageId":"200706090215.05318.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:15:05Z","receivedAt":"2007-06-09T00:15:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n mktag.c | 29 ++++++++++++++++-------------\n 1 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 4dbefab..2e70504 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -81,19 +81,22 @@ static int verify_tag(char *data, unsigned long size)\n \tmemcpy(type, type_line + 5, type_len);\n \ttype[type_len] = '\\0';\n \n-\t/* Verify that the object matches */\n-\tif (verify_object(sha1, type))\n-\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n-\n-\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n-\ttag_line += 4;\n-\tfor (;;) {\n-\t\tunsigned char c = *tag_line++;\n-\t\tif (c == '\\n')\n-\t\t\tbreak;\n-\t\tif (c > ' ')\n-\t\t\tcontinue;\n-\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - data);\n+\t{\n+\t\tunsigned long i;\n+\n+\t\t/* Verify that the object matches */\n+\t\tif (verify_object(sha1, type))\n+\t\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n+\n+\t\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n+\t\tfor (i = 4;;) {\n+\t\t\tunsigned char c = tag_line[i++];\n+\t\t\tif (c == '\\n')\n+\t\t\t\tbreak;\n+\t\t\tif (c > ' ')\n+\t\t\t\tcontinue;\n+\t\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line + i - data);\n+\t\t}\n \t}\n \n \t/* TODO: check for committer info + blank line? */\n-- \n1.5.2\n"},{"id":"44409","messageId":"200706090215.31731.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:15:31Z","receivedAt":"2007-06-09T00:15:31Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Rename parse_tag_buffer_internal() to parse_and_verify_tag_buffer() since\nit now does tag object verification as well.\n\nAdd a new parameter 'thorough_verify' for turning on/off the extra code\nto be run when verifying tag objects (as opposed to general parsing).\n\nverify_tag() and parse_and_verify_tag_buffer() are now functionally\nequivalent, provided that parse_and_verify_tag_buffer() is called with\nitem == NULL and thorough_verification != 0.\n\nSigned-off-by: Johan Herland \n---\n tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex b134967..3896e45 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -33,7 +33,26 @@ struct tag *lookup_tag(const unsigned char *sha1)\n return (struct tag *) obj;\n }\n \n-static int parse_tag_buffer_internal(struct tag *item, const char *data, const unsigned long size)\n+/*\n+ * We refuse to tag something we can't verify. Just because.\n+ */\n+static int verify_object(unsigned char *sha1, const char *expected_type)\n+{\n+\tint ret = -1;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tvoid *buffer = read_sha1_file(sha1, &type, &size);\n+\n+\tif (buffer) {\n+\t\tif (type == type_from_string(expected_type))\n+\t\t\tret = check_sha1_signature(sha1, buffer, size, expected_type);\n+\t\tfree(buffer);\n+\t}\n+\treturn ret;\n+}\n+\n+static int parse_and_verify_tag_buffer(struct tag *item,\n+\t\tconst char *data, const unsigned long size, int thorough_verify)\n {\n #ifdef NO_C99_FORMAT\n #define PD_FMT \"%d\"\n@@ -79,6 +98,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n \ttagger_line = strchr(tag_line, '\\n');\n \tif (!tagger_line++)\n \t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", tag_line - data);\n+\tif (thorough_verify) {\n+\t\tif (memcmp(tagger_line, \"tagger \", 7) || (tagger_line[7] == '\\n'))\n+\t\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - data);\n+\t}\n \n \t/* Get the actual type */\n \ttype_len = tag_line - type_line - strlen(\"type \\n\");\n@@ -87,6 +110,29 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n \tmemcpy(type, type_line + 5, type_len);\n \ttype[type_len] = '\\0';\n \n+\tif (thorough_verify) {\n+\t\tunsigned long i;\n+\n+\t\t/* Verify that the object matches */\n+\t\tif (verify_object(sha1, type))\n+\t\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n+\n+\t\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n+\t\tfor (i = 4;;) {\n+\t\t\tunsigned char c = tag_line[i++];\n+\t\t\tif (c == '\\n')\n+\t\t\t\tbreak;\n+\t\t\tif (c > ' ')\n+\t\t\t\tcontinue;\n+\t\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line + i - data);\n+\t\t}\n+\n+\t\t/* Verify the tagger line */\n+\t\t/* TODO: check for committer/tagger info */\n+\n+\t\t/* The actual stuff afterwards we don't care about.. */\n+\t}\n+\n \tif (item) {\n \t\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n \t\titem->tag = xmalloc(tag_len + 1);\n@@ -120,7 +166,7 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n \n int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n {\n-\treturn parse_tag_buffer_internal(item, (const char *) data, size);\n+\treturn parse_and_verify_tag_buffer(item, (const char *) data, size, 0);\n }\n \n int parse_tag(struct tag *item)\n-- \n1.5.2\n"},{"id":"44410","messageId":"200706090215.54494.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 08/21] Switch from verify_tag() to parse_and_verify_tag_buffer() for verifying tag objects in git-mktag","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:15:54Z","receivedAt":"2007-06-09T00:15:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This involves exposing parse_and_verify_tag_buffer() in the tag API\n(tag.h).\n\nAlso synchronize selftest with change in error message.\n\nSigned-off-by: Johan Herland \n---\n mktag.c | 5 ++---\n t/t3800-mktag.sh | 2 +-\n tag.c | 2 +-\n tag.h | 2 ++\n 4 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 2e70504..5780f33 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -125,9 +125,8 @@ int main(int argc, char **argv)\n \t}\n \tbuffer[size] = 0;\n \n-\t/* Verify it for some basic sanity: it needs to start with\n-\t \"object \\ntype\\ntagger \" */\n-\tif (verify_tag(buffer, size) < 0)\n+\t/* Verify tag object data */\n+\tif (parse_and_verify_tag_buffer(0, buffer, size, 1))\n \t\tdie(\"invalid tag signature file\");\n \n \tif (write_sha1_file(buffer, size, tag_type, result_sha1) < 0)\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex b4edb4d..0e87d2a 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -34,7 +34,7 @@ too short for a tag\n EOF\n \n cat >expect.pat <\n---\n mktag.c | 94 ---------------------------------------------------------------\n 1 files changed, 0 insertions(+), 94 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 5780f33..4f226ab 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -14,100 +14,6 @@\n * shortest single-character-tag line.\n */\n \n-/*\n- * We refuse to tag something we can't verify. Just because.\n- */\n-static int verify_object(unsigned char *sha1, const char *expected_type)\n-{\n-\tint ret = -1;\n-\tenum object_type type;\n-\tunsigned long size;\n-\tvoid *buffer = read_sha1_file(sha1, &type, &size);\n-\n-\tif (buffer) {\n-\t\tif (type == type_from_string(expected_type))\n-\t\t\tret = check_sha1_signature(sha1, buffer, size, expected_type);\n-\t\tfree(buffer);\n-\t}\n-\treturn ret;\n-}\n-\n-static int verify_tag(char *data, unsigned long size)\n-{\n-#ifdef NO_C99_FORMAT\n-#define PD_FMT \"%d\"\n-#else\n-#define PD_FMT \"%td\"\n-#endif\n-\n-\tunsigned char sha1[20];\n-\tchar type[20];\n-\tconst char *type_line, *tag_line, *tagger_line;\n-\tunsigned long type_len;\n-\n-\tif (size < 64)\n-\t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n-\n-\t/* Verify object line */\n-\tif (memcmp(data, \"object \", 7))\n-\t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n-\n-\tif (get_sha1_hex(data + 7, sha1))\n-\t\treturn error(\"char%d: could not get SHA1 hash\", 7);\n-\n-\t/* Verify type line */\n-\ttype_line = data + 48;\n-\tif (memcmp(type_line - 1, \"\\ntype \", 6))\n-\t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n-\n-\t/* Verify tag-line */\n-\ttag_line = strchr(type_line, '\\n');\n-\tif (!tag_line++)\n-\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - data);\n-\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n-\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - data);\n-\n-\t/* Verify the tagger line */\n-\ttagger_line = strchr(tag_line, '\\n');\n-\tif (!tagger_line++)\n-\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", tag_line - data);\n-\tif (memcmp(tagger_line, \"tagger \", 7) || (tagger_line[7] == '\\n'))\n-\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - data);\n-\n-\t/* Get the actual type */\n-\ttype_len = tag_line - type_line - strlen(\"type \\n\");\n-\tif (type_len >= sizeof(type))\n-\t\treturn error(\"char\" PD_FMT \": type too long\", type_line + 5 - data);\n-\tmemcpy(type, type_line + 5, type_len);\n-\ttype[type_len] = '\\0';\n-\n-\t{\n-\t\tunsigned long i;\n-\n-\t\t/* Verify that the object matches */\n-\t\tif (verify_object(sha1, type))\n-\t\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n-\n-\t\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n-\t\tfor (i = 4;;) {\n-\t\t\tunsigned char c = tag_line[i++];\n-\t\t\tif (c == '\\n')\n-\t\t\t\tbreak;\n-\t\t\tif (c > ' ')\n-\t\t\t\tcontinue;\n-\t\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line + i - data);\n-\t\t}\n-\t}\n-\n-\t/* TODO: check for committer info + blank line? */\n-\t/* Also, the minimum length is probably + \"tagger .\", or 63+8=71 */\n-\n-\t/* The actual stuff afterwards we don't care about.. */\n-\treturn 0;\n-\n-#undef PD_FMT\n-}\n-\n int main(int argc, char **argv)\n {\n \tunsigned long size = 4096;\n-- \n1.5.2\n"},{"id":"44412","messageId":"200706090216.41856.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 10/21] Free mktag's buffer before dying","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:16:41Z","receivedAt":"2007-06-09T00:16:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n mktag.c | 13 ++++++++-----\n 1 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 4f226ab..31eadd8 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -21,7 +21,7 @@ int main(int argc, char **argv)\n \tunsigned char result_sha1[20];\n \n \tif (argc != 1)\n-\t\tusage(\"git-mktag < signaturefile\");\n+\t\tusage(\"git-mktag < tag_data_file\");\n \n \tsetup_git_directory();\n \n@@ -32,14 +32,17 @@ int main(int argc, char **argv)\n \tbuffer[size] = 0;\n \n \t/* Verify tag object data */\n-\tif (parse_and_verify_tag_buffer(0, buffer, size, 1))\n-\t\tdie(\"invalid tag signature file\");\n+\tif (parse_and_verify_tag_buffer(0, buffer, size, 1)) {\n+\t\tfree(buffer);\n+\t\tdie(\"invalid tag data file\");\n+\t}\n \n-\tif (write_sha1_file(buffer, size, tag_type, result_sha1) < 0)\n+\tif (write_sha1_file(buffer, size, tag_type, result_sha1) < 0) {\n+\t\tfree(buffer);\n \t\tdie(\"unable to write tag file\");\n+\t}\n \n \tfree(buffer);\n-\n \tprintf(\"%s\\n\", sha1_to_hex(result_sha1));\n \treturn 0;\n }\n-- \n1.5.2\n"},{"id":"44413","messageId":"200706090217.17795.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 11/21] Rewrite error messages; fix up line lengths","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:17:17Z","receivedAt":"2007-06-09T00:17:17Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Also update selftests to reflect new error messages.\n\nSigned-off-by: Johan Herland \n---\n t/t3800-mktag.sh | 22 +++++++++++-----------\n tag.c | 44 +++++++++++++++++++++++++++++---------------\n 2 files changed, 40 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex 0e87d2a..3bce5e0 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -49,7 +49,7 @@ tag mytag\n EOF\n \n cat >expect.pat <expect.pat <expect.pat <tag.sig\n printf \"type tagsssssssssssssssssssssssssssssss\" >>tag.sig\n \n cat >expect.pat <expect.pat <expect.pat <expect.pat <expect.pat <expect.pat <expect.pat <expect.pat <= sizeof(type))\n-\t\treturn error(\"char\" PD_FMT \": type too long\", type_line + 5 - data);\n+\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\"Type too long\", type_line + 5 - data);\n \tmemcpy(type, type_line + 5, type_len);\n \ttype[type_len] = '\\0';\n \n@@ -115,16 +124,20 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \t\t/* Verify that the object matches */\n \t\tif (verify_object(sha1, type))\n-\t\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n+\t\t\treturn error(\"Tag object (@ char 7): \"\n+\t\t\t\t\"Could not verify tagged object %s\",\n+\t\t\t\tsha1_to_hex(sha1));\n \n-\t\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n+\t\t/* Verify tag name: disallow control characters or spaces */\n \t\tfor (i = 4;;) {\n \t\t\tunsigned char c = tag_line[i++];\n \t\t\tif (c == '\\n')\n \t\t\t\tbreak;\n \t\t\tif (c > ' ')\n \t\t\t\tcontinue;\n-\t\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line + i - data);\n+\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\t\"Could not verify tag name\",\n+\t\t\t\ttag_line + i - data);\n \t\t}\n \n \t\t/* Verify the tagger line */\n@@ -148,7 +161,8 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t} else if (!strcmp(type, tag_type)) {\n \t\t\titem->tagged = &lookup_tag(sha1)->object;\n \t\t} else {\n-\t\t\terror(\"Unknown type %s\", type);\n+\t\t\terror(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\t\"Unknown type '%s'\", type_line + 5 - data, type);\n \t\t\titem->tagged = NULL;\n \t\t}\n \n-- \n1.5.2\n"},{"id":"44414","messageId":"200706090217.49818.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:17:49Z","receivedAt":"2007-06-09T00:17:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n tag.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex c7c75b3..ac76ec0 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -51,6 +51,13 @@ static int verify_object(unsigned char *sha1, const char *expected_type)\n \treturn ret;\n }\n \n+/*\n+ * Perform parsing and verification of tag object data.\n+ *\n+ * The 'item' parameter may be set to NULL if only verification is desired.\n+ *\n+ * The given data _must_ be null-terminated.\n+ */\n int parse_and_verify_tag_buffer(struct tag *item,\n \t\tconst char *data, const unsigned long size, int thorough_verify)\n {\n@@ -75,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\treturn error(\"Tag object failed preliminary size check\");\n \n \t/* Verify object line */\n-\tif (memcmp(data, \"object \", 7))\n+\tif (prefixcmp(data, \"object \"))\n \t\treturn error(\"Tag object (@ char 0): \"\n \t\t\t\"Does not start with \\\"object \\\"\");\n \n@@ -84,7 +91,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \t/* Verify type line */\n \ttype_line = data + 48;\n-\tif (memcmp(type_line - 1, \"\\ntype \", 6))\n+\tif (prefixcmp(type_line - 1, \"\\ntype \"))\n \t\treturn error(\"Tag object (@ char 47): \"\n \t\t\t\"Could not find \\\"\\\\ntype \\\"\");\n \n@@ -94,7 +101,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\"Could not find \\\"\\\\n\\\" after \\\"type\\\"\",\n \t\t\ttype_line - data);\n-\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n+\tif (prefixcmp(tag_line, \"tag \") || tag_line[4] == '\\n')\n \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\"Could not find \\\"tag \\\"\", tag_line - data);\n \n@@ -105,7 +112,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\"Could not find \\\"\\\\n\\\" after \\\"tag\\\"\",\n \t\t\ttag_line - data);\n \tif (thorough_verify) {\n-\t\tif (memcmp(tagger_line, \"tagger \", 7) || (tagger_line[7] == '\\n'))\n+\t\tif (prefixcmp(tagger_line, \"tagger \") || (tagger_line[7] == '\\n'))\n \t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"tagger \\\"\",\n \t\t\t\ttagger_line - data);\n-- \n1.5.2\n"},{"id":"44415","messageId":"200706090218.16281.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 13/21] Collect skipping of header field names and calculation of line lengths in one place","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:18:16Z","receivedAt":"2007-06-09T00:18:16Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"For each of the parsed lines we at some point skip past its initial\nidentifier (\"type \", \"tag \", etc.). We also at some point calculate the\nlength of the remaining line. This patch moves these calculations into\none place. This provides _one_ place for all header lines where their\nrespective pointers start pointing at the header value (instead of the\nstart of the line), and their lengths are calculated.\n\nSigned-off-by: Johan Herland \n---\n tag.c | 21 ++++++++++++++-------\n 1 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex ac76ec0..9a6924f 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -118,12 +118,20 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\ttagger_line - data);\n \t}\n \n+\t/*\n+\t * Advance header field pointers past their initial identifier.\n+\t * Calculate lengths of header fields.\n+\t */\n+\ttype_line += strlen(\"type \");\n+\ttype_len = tag_line - type_line - 1;\n+\ttag_line += strlen(\"tag \");\n+\ttag_len = tagger_line - tag_line - 1;\n+\n \t/* Get the actual type */\n-\ttype_len = tag_line - type_line - strlen(\"type \\n\");\n \tif (type_len >= sizeof(type))\n \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n-\t\t\t\"Type too long\", type_line + 5 - data);\n-\tmemcpy(type, type_line + 5, type_len);\n+\t\t\t\"Type too long\", type_line - data);\n+\tmemcpy(type, type_line, type_len);\n \ttype[type_len] = '\\0';\n \n \tif (thorough_verify) {\n@@ -136,7 +144,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\tsha1_to_hex(sha1));\n \n \t\t/* Verify tag name: disallow control characters or spaces */\n-\t\tfor (i = 4;;) {\n+\t\tfor (i = 0;;) {\n \t\t\tunsigned char c = tag_line[i++];\n \t\t\tif (c == '\\n')\n \t\t\t\tbreak;\n@@ -154,9 +162,8 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t}\n \n \tif (item) {\n-\t\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n \t\titem->tag = xmalloc(tag_len + 1);\n-\t\tmemcpy(item->tag, tag_line + 4, tag_len);\n+\t\tmemcpy(item->tag, tag_line, tag_len);\n \t\titem->tag[tag_len] = '\\0';\n \n \t\tif (!strcmp(type, blob_type)) {\n@@ -169,7 +176,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\titem->tagged = &lookup_tag(sha1)->object;\n \t\t} else {\n \t\t\terror(\"Tag object (@ char \" PD_FMT \"): \"\n-\t\t\t\t\"Unknown type '%s'\", type_line + 5 - data, type);\n+\t\t\t\t\"Unknown type '%s'\", type_line - data, type);\n \t\t\titem->tagged = NULL;\n \t\t}\n \n-- \n1.5.2\n"},{"id":"44416","messageId":"200706090218.41941.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 14/21] Add proper parsing of \"tagger\" line, but only when thorough_verify is set","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:18:41Z","receivedAt":"2007-06-09T00:18:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Be explicit about the fact that the \"tagger\" line is now considered a\nmandatory part of the tag object. There are however old tags (from before\nJuly 2005) that don't have a \"tagger\" line. We therefore consider the\n\"tagger\" line _optional_ when parsing tags without thorough_verify set.\n\nIn practice this means that verification of a missing \"tagger\" line will\nonly fail when adding or fsck-ing tags.\n\nSigned-off-by: Johan Herland \n---\n tag.c | 48 ++++++++++++++++++++++++++++++++++--------------\n 1 files changed, 34 insertions(+), 14 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 9a6924f..c373c86 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -69,8 +69,9 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \tunsigned char sha1[20];\n \tchar type[20];\n-\tconst char *type_line, *tag_line, *tagger_line;\n-\tunsigned long type_len, tag_len;\n+\tconst char *type_line, *tag_line, *tagger_line;\n+\tunsigned long type_len, tag_len, tagger_len;\n+\tconst char *header_end;\n \n \tif (item) {\n \t\tif (item->object.parsed)\n@@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \tif (size < 64)\n \t\treturn error(\"Tag object failed preliminary size check\");\n \n-\t/* Verify object line */\n+\t/* Verify mandatory object line */\n \tif (prefixcmp(data, \"object \"))\n \t\treturn error(\"Tag object (@ char 0): \"\n \t\t\t\"Does not start with \\\"object \\\"\");\n@@ -89,13 +90,13 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \tif (get_sha1_hex(data + 7, sha1))\n \t\treturn error(\"Tag object (@ char 7): Could not get SHA1 hash\");\n \n-\t/* Verify type line */\n+\t/* Verify mandatory type line */\n \ttype_line = data + 48;\n \tif (prefixcmp(type_line - 1, \"\\ntype \"))\n \t\treturn error(\"Tag object (@ char 47): \"\n \t\t\t\"Could not find \\\"\\\\ntype \\\"\");\n \n-\t/* Verify tag-line */\n+\t/* Verify mandatory tag line */\n \ttag_line = strchr(type_line, '\\n');\n \tif (!tag_line++)\n \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n@@ -105,27 +106,46 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\"Could not find \\\"tag \\\"\", tag_line - data);\n \n-\t/* Verify the tagger line */\n+\t/*\n+\t * Verify mandatory tagger line, but only when we're checking\n+\t * thoroughly, i.e. on inserting a new tag, and on fsck.\n+\t * There are existing tag objects without a tagger line (most\n+\t * notably the \"v0.99\" tag in the main git repo), and we don't\n+\t * want to fail parsing on these.\n+\t */\n \ttagger_line = strchr(tag_line, '\\n');\n \tif (!tagger_line++)\n \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\"Could not find \\\"\\\\n\\\" after \\\"tag\\\"\",\n \t\t\ttag_line - data);\n-\tif (thorough_verify) {\n-\t\tif (prefixcmp(tagger_line, \"tagger \") || (tagger_line[7] == '\\n'))\n+\tif (prefixcmp(tagger_line, \"tagger \")) { /* no tagger given */\n+\t\tif (thorough_verify)\n \t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"tagger \\\"\",\n \t\t\t\ttagger_line - data);\n+\t\theader_end = tagger_line;\n+\t}\n+\telse { /* tagger given */\n+\t\theader_end = strchr(tagger_line, '\\n');\n+\t\tif (!header_end++)\n+\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\t\"Could not find \\\"\\\\n\\\" after \\\"tagger\\\"\",\n+\t\t\t\ttagger_line - data);\n \t}\n \n \t/*\n \t * Advance header field pointers past their initial identifier.\n-\t * Calculate lengths of header fields.\n+\t * Calculate lengths of header fields (0 for fields that are not given).\n \t */\n-\ttype_line += strlen(\"type \");\n-\ttype_len = tag_line - type_line - 1;\n-\ttag_line += strlen(\"tag \");\n-\ttag_len = tagger_line - tag_line - 1;\n+\ttype_line += strlen(\"type \");\n+\ttype_len = tag_line > type_line ?\n+\t\t\t(tag_line - type_line) - 1 : 0;\n+\ttag_line += strlen(\"tag \");\n+\ttag_len = tagger_line > tag_line ?\n+\t\t\t(tagger_line - tag_line) - 1 : 0;\n+\ttagger_line += strlen(\"tagger \");\n+\ttagger_len = header_end > tagger_line ?\n+\t\t\t(header_end - tagger_line) - 1 : 0;\n \n \t/* Get the actual type */\n \tif (type_len >= sizeof(type))\n@@ -155,7 +175,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\ttag_line + i - data);\n \t\t}\n \n-\t\t/* Verify the tagger line */\n+\t\t/* Verify tagger line */\n \t\t/* TODO: check for committer/tagger info */\n \n \t\t/* The actual stuff afterwards we don't care about.. */\n-- \n1.5.2\n"},{"id":"44417","messageId":"200706090219.12011.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 15/21] Make tag names (i.e. the tag object's \"tag\" line) optional","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:19:11Z","receivedAt":"2007-06-09T00:19:11Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The tag line is now optional. If not given in the tag object data, it\ndefaults to the empty string (\"\") in the parsed tag object.\n\nThe patch also adds a change to git-show; when asked to display a tag\nobject with no name (missing \"tag\" header), we will show the tag's sha1\ninstead of an empty string.\n\nFinally the patch includes some tweaks to the selftests to make them work\nwith optional tag names.\n\nSigned-off-by: Johan Herland \n---\n builtin-log.c | 2 +-\n t/t3800-mktag.sh | 6 +++---\n tag.c | 51 +++++++++++++++++++++++++++++----------------------\n tag.h | 2 +-\n 4 files changed, 34 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 212cdfc..8a238c7 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -181,7 +181,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tprintf(\"%stag %s%s\\n\\n\",\n \t\t\t\t\tdiff_get_color(rev.diffopt.color_diff,\n \t\t\t\t\t\tDIFF_COMMIT),\n-\t\t\t\t\tt->tag,\n+\t\t\t\t\t*(t->tag) ? t->tag : name,\n \t\t\t\t\tdiff_get_color(rev.diffopt.color_diff,\n \t\t\t\t\t\tDIFF_RESET));\n \t\t\tret = show_object(o->sha1, 1);\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex 3bce5e0..3381239 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -106,7 +106,7 @@ xxx mytag\n EOF\n \n cat >expect.pat <expect.pat <expect.pat < ' ')\n-\t\t\t\tcontinue;\n-\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n-\t\t\t\t\"Could not verify tag name\",\n-\t\t\t\ttag_line + i - data);\n+\t\tif (tag_len) { /* tag name was given */\n+\t\t\tfor (i = 0; i < tag_len; ++i) {\n+\t\t\t\tunsigned char c = tag_line[i];\n+\t\t\t\tif (c > ' ' && c != 0x7f)\n+\t\t\t\t\tcontinue;\n+\t\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\t\t\"Could not verify tag name\",\n+\t\t\t\t\ttag_line + i - data);\n+\t\t\t}\n \t\t}\n \n \t\t/* Verify tagger line */\n@@ -181,10 +182,16 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t/* The actual stuff afterwards we don't care about.. */\n \t}\n \n-\tif (item) {\n-\t\titem->tag = xmalloc(tag_len + 1);\n-\t\tmemcpy(item->tag, tag_line, tag_len);\n-\t\titem->tag[tag_len] = '\\0';\n+\tif (item) { /* Store parsed information into item */\n+\t\tif (tag_len) { /* optional tag name was given */\n+\t\t\titem->tag = xmalloc(tag_len + 1);\n+\t\t\tmemcpy(item->tag, tag_line, tag_len);\n+\t\t\titem->tag[tag_len] = '\\0';\n+\t\t}\n+\t\telse { /* optional tag name not given */\n+\t\t\titem->tag = xmalloc(1);\n+\t\t\titem->tag[0] = '\\0';\n+\t\t}\n \n \t\tif (!strcmp(type, blob_type)) {\n \t\t\titem->tagged = &lookup_blob(sha1)->object;\ndiff --git a/tag.h b/tag.h\nindex f341b7f..2631911 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -8,7 +8,7 @@ extern const char *tag_type;\n struct tag {\n \tstruct object object;\n \tstruct object *tagged;\n-\tchar *tag;\n+\tchar *tag; /* optional, may be empty (\"\") */\n \tchar *signature; /* not actually implemented */\n };\n \n-- \n1.5.2\n"},{"id":"44418","messageId":"200706090219.37289.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 16/21] Introduce optional \"keywords\" on tag objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:19:37Z","receivedAt":"2007-06-09T00:19:37Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This patch introduces a new optional header line to the tag object, called\n\"keywords\". The \"keywords\" line may contain a comma-separated list of\ncustom keywords associated with the tag object. There are two \"special\"\nkeywords, however: \"tag\" and \"note\": When the \"keywords\" header is\nmissing, its default value is set to \"tag\" if a \"tag\" header is\npresent; else the default \"keywords\" value is set to \"note\". The\n\"keywords\" header is meant to be used by porcelains for classifying\ndifferent types of tag objects. This classification may then be used to\nfilter tag objects in the presentation layer (e.g. by implementing\nextra filter options to --decorate, etc.)\n\nSigned-off-by: Johan Herland \n---\n tag.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n tag.h | 1 +\n 2 files changed, 54 insertions(+), 7 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex fb678d7..1caec19 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -69,8 +69,8 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \tunsigned char sha1[20];\n \tchar type[20];\n-\tconst char *type_line, *tag_line, *tagger_line;\n-\tunsigned long type_len, tag_len, tagger_len;\n+\tconst char *type_line, *tag_line, *keywords_line, *tagger_line;\n+\tunsigned long type_len, tag_len, keywords_len, tagger_len;\n \tconst char *header_end;\n \n \tif (item) {\n@@ -103,15 +103,26 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\"Could not find \\\"\\\\n\\\" after \\\"type\\\"\",\n \t\t\ttype_line - data);\n \tif (prefixcmp(tag_line, \"tag \")) /* no tag name given */\n-\t\ttagger_line = tag_line;\n+\t\tkeywords_line = tag_line;\n \telse { /* tag name given */\n-\t\ttagger_line = strchr(tag_line, '\\n');\n-\t\tif (!tagger_line++)\n+\t\tkeywords_line = strchr(tag_line, '\\n');\n+\t\tif (!keywords_line++)\n \t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"\\\\n\\\" after \\\"tag\\\"\",\n \t\t\t\ttag_line - data);\n \t}\n \n+\t/* Verify optional keywords line */\n+\tif (prefixcmp(keywords_line, \"keywords \")) /* no keywords given */\n+\t\ttagger_line = keywords_line;\n+\telse { /* keywords given */\n+\t\ttagger_line = strchr(keywords_line, '\\n');\n+\t\tif (!tagger_line++)\n+\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\t\"Could not find \\\"\\\\n\\\" after \\\"keywords\\\"\",\n+\t\t\t\tkeywords_line - data);\n+\t}\n+\n \t/*\n \t * Verify mandatory tagger line, but only when we're checking\n \t * thoroughly, i.e. on inserting a new tag, and on fsck.\n@@ -142,8 +153,11 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \ttype_len = tag_line > type_line ?\n \t\t\t(tag_line - type_line) - 1 : 0;\n \ttag_line += strlen(\"tag \");\n-\ttag_len = tagger_line > tag_line ?\n-\t\t\t(tagger_line - tag_line) - 1 : 0;\n+\ttag_len = keywords_line > tag_line ?\n+\t\t\t(keywords_line - tag_line) - 1 : 0;\n+\tkeywords_line += strlen(\"keywords \");\n+\tkeywords_len = tagger_line > keywords_line ?\n+\t\t\t(tagger_line - keywords_line) - 1 : 0;\n \ttagger_line += strlen(\"tagger \");\n \ttagger_len = header_end > tagger_line ?\n \t\t\t(header_end - tagger_line) - 1 : 0;\n@@ -176,6 +190,26 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t}\n \t\t}\n \n+\t\t/*\n+\t\t * Verify keywords: disallow control characters, spaces,\n+\t\t * or two subsequent commas\n+\t\t */\n+\t\tif (keywords_len) { /* keywords line was given */\n+\t\t\tfor (i = 0; i < keywords_len; ++i) {\n+\t\t\t\tunsigned char c = keywords_line[i];\n+\t\t\t\tif (c == ',' && keywords_line[i + 1] == ',')\n+\t\t\t\t\t/* consecutive commas */\n+\t\t\t\t\treturn error(\"Tag object (@ char \"\n+\t\t\t\t\t\tPD_FMT \"): Found empty keyword\",\n+\t\t\t\t\t\tkeywords_line + i - data);\n+\t\t\t\tif (c > ' ' && c != 0x7f)\n+\t\t\t\t\tcontinue;\n+\t\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\t\t\"Could not verify keywords\",\n+\t\t\t\t\tkeywords_line + i - data);\n+\t\t\t}\n+\t\t}\n+\n \t\t/* Verify tagger line */\n \t\t/* TODO: check for committer/tagger info */\n \n@@ -193,6 +227,18 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\titem->tag[0] = '\\0';\n \t\t}\n \n+\t\tif (keywords_len) { /* optional keywords string was given */\n+\t\t\titem->keywords = xmalloc(keywords_len + 1);\n+\t\t\tmemcpy(item->keywords, keywords_line, keywords_len);\n+\t\t\titem->keywords[keywords_len] = '\\0';\n+\t\t}\n+\t\telse { /* optional keywords string not given. Set default */\n+\t\t\t/* if tag name is set, use \"tag\"; else use \"note\" */\n+\t\t\tconst char *default_kw = item->tag ? \"tag\" : \"note\";\n+\t\t\titem->keywords = xmalloc(strlen(default_kw) + 1);\n+\t\t\tmemcpy(item->keywords, default_kw, strlen(default_kw) + 1);\n+\t\t}\n+\n \t\tif (!strcmp(type, blob_type)) {\n \t\t\titem->tagged = &lookup_blob(sha1)->object;\n \t\t} else if (!strcmp(type, tree_type)) {\ndiff --git a/tag.h b/tag.h\nindex 2631911..3b0008d 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -9,6 +9,7 @@ struct tag {\n \tstruct object object;\n \tstruct object *tagged;\n \tchar *tag; /* optional, may be empty (\"\") */\n+\tchar *keywords; /* optional, defaults to tag ? \"tag\" : \"note\" */\n \tchar *signature; /* not actually implemented */\n };\n \n-- \n1.5.2\n"},{"id":"44419","messageId":"200706090220.04603.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 17/21] Update comments on tag objects in mktag.c","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:20:04Z","receivedAt":"2007-06-09T00:20:04Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Also update minimum tag object length to the new minimum length after refactoring.\n\nSigned-off-by: Johan Herland \n---\n mktag.c | 30 ++++++++++++++++++++++--------\n tag.c | 2 +-\n 2 files changed, 23 insertions(+), 9 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 31eadd8..af0cfa6 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -2,16 +2,30 @@\n #include \"tag.h\"\n \n /*\n- * A signature file has a very simple fixed format: four lines\n- * of \"object \" + \"type \" + \"tag \" +\n+ * Tag object data has the following format: two mandatory lines of\n+ * \"object \" + \"type \", plus two optional lines of\n+ * \"tag \" + \"keywords \", plus a mandatory line of\n * \"tagger \", followed by a blank line, a free-form tag\n- * message and a signature block that git itself doesn't care about,\n- * but that can be verified with gpg or similar.\n+ * message and an optional signature block that git itself doesn't\n+ * care about, but that can be verified with gpg or similar.\n *\n- * The first three lines are guaranteed to be at least 63 bytes:\n- * \"object \\n\" is 48 bytes, \"type tag\\n\" at 9 bytes is the\n- * shortest possible type-line, and \"tag .\\n\" at 6 bytes is the\n- * shortest single-character-tag line.\n+ * represents the object pointed to by this tag, is\n+ * the type of the object pointed to (\"tag\", \"blob\", \"tree\" or \"commit\"),\n+ * is the name of this tag object (and must correspond to the\n+ * name of the corresponding ref (if any) in '.git/refs/'). is\n+ * a comma-separated list of keywords associated with this tag object, and\n+ * holds the \"name \" of the tag creator and timestamp\n+ * of when the tag object was created (analogous to \"committer\" in commit\n+ * objects).\n+ *\n+ * The first two lines are guaranteed to be at least 57 bytes:\n+ * \"object \\n\" is 48 bytes, and \"type tag\\n\" at 9 bytes is\n+ * the shortest possible \"type\" line. The tagger line is at least\n+ * \"tagger \\n\" (8 bytes). Therefore a tag object _must_ have >= 65 bytes.\n+ *\n+ * If \"tag \" is omitted, defaults to the empty string.\n+ * If \"keywords \" is omitted, defaults to \"tag\" if\n+ * a was given, \"note\" otherwise.\n */\n \n int main(int argc, char **argv)\ndiff --git a/tag.c b/tag.c\nindex 1caec19..af4356e 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -79,7 +79,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\titem->object.parsed = 1;\n \t}\n \n-\tif (size < 64)\n+\tif (size < 65)\n \t\treturn error(\"Tag object failed preliminary size check\");\n \n \t/* Verify mandatory object line */\n-- \n1.5.2\n"},{"id":"44420","messageId":"200706090220.26342.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 18/21] git-fsck: Do thorough verification of tag objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:20:26Z","receivedAt":"2007-06-09T00:20:26Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Teach git-fsck to do the same kind of verification on tag objects that is\nalready done by git-mktag.\n\nSigned-off-by: Johan Herland \n---\n builtin-fsck.c | 15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 944a496..7b4c36b 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -359,11 +359,26 @@ static int fsck_commit(struct commit *commit)\n static int fsck_tag(struct tag *tag)\n {\n \tstruct object *tagged = tag->tagged;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tchar *data = (char *) read_sha1_file(tag->object.sha1, &type, &size);\n \n \tif (verbose)\n \t\tfprintf(stderr, \"Checking tag %s\\n\",\n \t\t\tsha1_to_hex(tag->object.sha1));\n \n+\tif (!data)\n+\t\treturn objerror(&tag->object, \"could not read tag\");\n+\tif (type != OBJ_TAG) {\n+\t\tfree(data);\n+\t\treturn objerror(&tag->object, \"not a tag (internal error)\");\n+\t}\n+\tif (parse_and_verify_tag_buffer(0, data, size, 1)) { /* thoroughly verify tag object */\n+\t\tfree(data);\n+\t\treturn objerror(&tag->object, \"failed thorough tag object verification\");\n+\t}\n+\tfree(data);\n+\n \tif (!tagged) {\n \t\treturn objerror(&tag->object, \"could not load tagged object\");\n \t}\n-- \n1.5.2\n"},{"id":"44421","messageId":"200706090220.55035.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 19/21] Documentation/git-mktag: Document the changes in tag object structure","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:20:54Z","receivedAt":"2007-06-09T00:20:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The new structure of tag objects is documented.\n\nAlso some much-needed cleanup is done. E.g. remove the paragraph on the\n8kB limit, since this limit was removed ages ago.\n\nSigned-off-by: Johan Herland \n---\n Documentation/git-mktag.txt | 41 ++++++++++++++++++++++++++++++-----------\n 1 files changed, 30 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-mktag.txt b/Documentation/git-mktag.txt\nindex 0ac3be1..411105d 100644\n--- a/Documentation/git-mktag.txt\n+++ b/Documentation/git-mktag.txt\n@@ -8,38 +8,57 @@ git-mktag - Creates a tag object\n \n SYNOPSIS\n --------\n-'git-mktag' < signature_file\n+[verse]\n+'git-mktag' < tag_data_file\n+\n \n DESCRIPTION\n -----------\n-Reads a tag contents on standard input and creates a tag object\n+Reads tag object data on standard input and creates a tag object\n that can also be used to sign other objects.\n \n The output is the new tag's identifier.\n \n-Tag Format\n+\n+DISCUSSION\n ----------\n-A tag signature file has a very simple fixed format: three lines of\n+Tag object data has the following format\n \n+[verse]\n object \n type \n- tag \n+ tag (optional)\n+ keywords (optional)\n+ tagger \n+\n+followed by a blank line and a free-form message and an optional signature\n+that git itself doesn't care about, but that may be verified with gpg or\n+similar.\n \n-followed by some 'optional' free-form signature that git itself\n-doesn't care about, but that can be verified with gpg or similar.\n+In the above listing, `` represents the object pointed to by this tag,\n+`` is the type of the object pointed to (\"tag\", \"blob\", \"tree\" or\n+\"commit\"), `` is the name of this tag object (and must correspond\n+to the name of the corresponding ref (if any) in `.git/refs/`). ``\n+is a comma-separated list of keywords associated with this tag object, and\n+`` holds the \"`name `\" of the tag creator and timestamp\n+of when the tag object was created (analogous to \"committer\" in commit\n+objects).\n \n-The size of the full object is artificially limited to 8kB. (Just\n-because I'm a lazy bastard, and if you can't fit a signature in that\n-size, you're doing something wrong)\n+If \"`tag `\" is omitted, defaults to the empty string.\n+If \"`keywords `\" is omitted, defaults to \"`tag`\" if\n+a was given, \"`note`\" otherwise.\n \n \n Author\n ------\n Written by Linus Torvalds \n \n+\n Documentation\n --------------\n-Documentation by David Greaves, Junio C Hamano and the git-list .\n+Documentation by Johan Herland, David Greaves, Junio C Hamano and the\n+git-list .\n+\n \n GIT\n ---\n-- \n1.5.2\n"},{"id":"44422","messageId":"200706090221.35008.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 20/21] git-mktag tests: Expand on mktag selftests according to the new tag object structure","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:21:34Z","receivedAt":"2007-06-09T00:21:34Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Some more tests are added to test the new \"keywords\" header, and to test\nthe more thorough verification routine.\n\nSigned-off-by: Johan Herland \n---\n t/t3800-mktag.sh | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 200 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex 3381239..b3b3121 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -46,6 +46,8 @@ cat >tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <.git/refs/tags/mytag 2>message'\n \n ############################################################\n-# 14. check mytag\n+# 20. check mytag\n \n test_expect_success \\\n 'check mytag' \\\n 'git-tag -l | grep mytag'\n \n+############################################################\n+# 21. create valid tag #2\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n+############################################################\n+# 22. create valid tag #3\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n+############################################################\n+# 23. create valid tag #4\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n+############################################################\n+# 24. create valid tag #5 (with empty message)\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n \n test_done\n-- \n1.5.2\n"},{"id":"44423","messageId":"200706090221.55133.johan@herland.net","threadId":"8491","inReplyTo":"200706090210.36270.johan@herland.net","subject":"[PATCH 21/21] Add fsck_verify_ref_to_tag_object() to verify that refname matches name stored in tag object","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T00:21:55Z","receivedAt":"2007-06-09T00:21:55Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 28 May 2007, Junio C Hamano wrote:\n> However it would be a good\n> idea to add logic to fsck to warn upon inconsistencis (perhaps\n> by mistake) between refname and tag's true name.\n>\n> The check would say something like:\n>\n> If an annotated (signed or unsigned) tag has a \"tag\"\n> line to give it the official $name, and if it is pointed\n> at by a ref, the refname must end with \"/$name\".\n> Otherwise we warn.\n>\n> Trivially, the above rule says that having v2.6.22 tag under\n> refs/tags/v2.6.20 is a mistake we would want to be warned upon.\n\nThis patch adds the check described by Junio.\n\nIt also includes a bugfix when tag object parsing fails, from\nJohannes Schindelin .\n\nSigned-off-by: Johan Herland \n---\n builtin-fsck.c | 26 ++++++++++++++++++++++++++\n 1 files changed, 26 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 7b4c36b..a47976c 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -542,6 +542,30 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, in\n \treturn 0;\n }\n \n+static void fsck_verify_ref_to_tag_object(const char *refname, struct object *obj)\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;\n+\tsize_t refname_len = strlen(refname);\n+\n+\tif (!tagobj->tag) {\n+\t\terror(\"%s: Failed to parse tag object %s\", refname, sha1_to_hex(obj->sha1));\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+\t\t!memcmp(tagobj->tag, refname + (refname_len - tagname_len), tagname_len) &&\n+\t\trefname[(refname_len - tagname_len) - 1] == '/') {\n+\t\t/* OK: tag name is \"$name\", and refname ends with \"/$name\" */\n+\t\treturn;\n+\t}\n+\telse\n+\t\terror(\"%s: Mismatch between tag ref and tag object's name: '%s'\", refname, tagobj->tag);\n+}\n+\n static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n {\n \tstruct object *obj;\n@@ -556,6 +580,8 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f\n \t\t/* We'll continue with the rest despite the error.. */\n \t\treturn 0;\n \t}\n+\tif (obj->type == OBJ_TAG) /* ref to tag object */\n+\t\tfsck_verify_ref_to_tag_object(refname, obj);\n \tdefault_refs++;\n \tobj->used = 1;\n \tmark_reachable(obj, REACHABLE);\n-- \n1.5.2\n"},{"id":"44428","messageId":"Pine.LNX.4.64.0706090339280.4059@racer.site","threadId":"8491","inReplyTo":"200706090213.40785.johan@herland.net","subject":"Re: [PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-09T02:54:11Z","receivedAt":"2007-06-09T02:54:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> diff --git a/mktag.c b/mktag.c\n> index b82e377..0bc20c8 100644\n> --- a/mktag.c\n> +++ b/mktag.c\n> @@ -32,52 +32,48 @@ static int verify_object(unsigned char *sha1, const char *expected_type)\n> \treturn ret;\n> }\n> \n> +static int verify_tag(char *data, unsigned long size)\n> +{\n> #ifdef NO_C99_FORMAT\n> #define PD_FMT \"%d\"\n> #else\n> #define PD_FMT \"%td\"\n> #endif\n> \n> -static int verify_tag(char *buffer, unsigned long size)\n\nWhy this rename from buffer to data?\n\n> -{\n> -\tint typelen;\n> -\tchar type[20];\n> \tunsigned char sha1[20];\n> -\tconst char *object, *type_line, *tag_line, *tagger_line;\n> +\tchar type[20];\n> +\tconst char *type_line, *tag_line, *tagger_line;\n> +\tunsigned long type_len;\n\nWhy this change of order?\n\n> \tif (size < 64)\n> \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n> \n> -\tbuffer[size] = 0;\n\nAre you sure that your buffer is always NUL terminated?\n\n> -\ttype_line = object + 48;\n> +\ttype_line = data + 48;\n\nQuite a lot of changes seem to do this object->data. The patch would have \nbeen much more compact if you just had renamed buffer to object instead of \ndata.\n\n> -\ttypelen = tag_line - type_line - strlen(\"type \\n\");\n> -\tif (typelen >= sizeof(type))\n> -\t\treturn error(\"char\" PD_FMT \": type too long\", type_line+5 - buffer);\n> -\n> -\tmemcpy(type, type_line+5, typelen);\n> -\ttype[typelen] = 0;\n> +\ttype_len = tag_line - type_line - strlen(\"type \\n\");\n> +\tif (type_len >= sizeof(type))\n> +\t\treturn error(\"char\" PD_FMT \": type too long\", type_line + 5 - data);\n> +\tmemcpy(type, type_line + 5, type_len);\n> +\ttype[type_len] = '\\0';\n\nThis renaming variables has nothing to do with refactoring. In fact, I \nhave a hard time to find code changes (which your subject suggests, as you \nwant to make two functions more similar).\n\n> \t/* TODO: check for committer info + blank line? */\n> \t/* Also, the minimum length is probably + \"tagger .\", or 63+8=71 */\n> \n> \t/* The actual stuff afterwards we don't care about.. */\n> \treturn 0;\n> -}\n> \n> #undef PD_FMT\n> +}\n\nAny particular reason for this?\n\n> @@ -124,6 +120,7 @@ int main(int argc, char **argv)\n> \t\tfree(buffer);\n> \t\tdie(\"could not read from stdin\");\n> \t}\n> +\tbuffer[size] = 0;\n\nAh, so you terminate the buffer here. From the patch, it is relatively \nhard to see if this line is always hit _before_ the function is called \nthat evidently relies on NUL termination. By moving it here, I think it is \nmuch more likely to overlook the fact that the function, which made sure \nthat its assumption was met, needs this line. Whereas if you left it where \nit was, the assumption would always be met.\n\n> - if (item->object.parsed)\n> - return 0;\n> - item->object.parsed = 1;\n> +\tif (item->object.parsed)\n> +\t\treturn 0;\n> +\titem->object.parsed = 1;\n\nAgain, this has nothing to do with refactoring.\n\n> @@ -57,39 +57,38 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n> \tif (memcmp(data, \"object \", 7))\n> \t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n> \n> -\tif (get_sha1_hex((char *) data + 7, sha1))\n> +\tif (get_sha1_hex(data + 7, sha1))\n\nIs this really necessary? Even if (which I doubt), it has nothing to do \nwith refactoring.\n\nIf you _want_ to _explicitely_ do arithmetic on a char* instead of void*, \nwhy not DRT and change the function signature?\n\n> -\tsig_line++;\n> +\ttagger_line++;\n\nI am really reluctant with renamings like these. IMHO they don't buy you \nmuch, except for possible confusion. It is evident that sig means the \nsigner (and it is obvious in the case of an unsigned tag, who is meant, \ntoo).\n\n> +int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n> +{\n> +\treturn parse_tag_buffer_internal(item, (const char *) data, size);\n> +}\n\nThis cast (and indeed, this function, if you ask me) is unnecessary.\n\nI reviewed only this patch out of your long series, mostly because I found \nthe subject line interesting. But IMHO the patch does not what the subject \nline suggests.\n\nUnfortunately, it's unlikely that I will have time until Monday night to \ncontinue with this patch series.\n\nCiao,\nDscho\n"},{"id":"44442","messageId":"200706091249.45042.johan@herland.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706090339280.4059@racer.site","subject":"Re: [PATCH 03/21] Refactoring to make verify_tag() and parse_tag_buffer() more similar","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T10:49:44Z","receivedAt":"2007-06-09T10:49:44Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Johannes Schindelin wrote:\n> Hi,\n\nHi. Thanks for taking the time to look at (some of) my patch. Most of your\nquestions below can be answered with a single answer:\n\nThe main purpose of the patch is (as the subject line says) to bring the\ntwo functions more in line with eachother. At the time I made the patch,\nI had made the observation that these function were trying to do much the\nsame thing, albeit in a slightly different form. This patch is therefore\nabout applying a series of (mostly non-functional) refactorings to make\ntheir diff as small as possible. This involves \"stupid\" changes such as\nrenaming variables, tweaking whitespace, reordering the declaration of\nvariables, etc. It's all to make the functions similar to the point where\nI can diff them, get a small and meaningful result, see the remaining\n_real_ differences, and in the end, _merge_ them (see patches 7-9).\nIf this whole exercise didn't end up with merging the two functions into\none, I would _totally_ agree with you that all this refactoring is more\nharmful than beneficial.\n\n> On Sat, 9 Jun 2007, Johan Herland wrote:\n> > \tif (size < 64)\n> > \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n> > \n> > -\tbuffer[size] = 0;\n> \n> Are you sure that your buffer is always NUL terminated?\n\nFirst, (and you'll see this in the commit message) I'm _moving_ (not\nremoving) the NUL termination out of verify_tag() and into main() (which I\ncan be sure is the only caller of verify_tag(), since verify_tag is\ndeclared static, and there is no other call in that file). Two reasons for\ndoing this:\n\n1. Make verify_tag more similar to parse_tag_buffer() (because\nparse_tag_buffer() does not NUL terminate)\n\n2. Do the NUL termination as close to the code that actually populated the\nbuffer with data (the read_pipe() in main())\n\n\nSo now you can ask: Why doesn't parse_tag_buffer() NUL terminate its\ninput? It _that_ safe? And I ran around checking all the callers of\nparse_tag_buffer, and found that all of them use data (most of which\noriginates from read_sha1_file()) that's already NUL terminated.\n\nIn the end, I also put in a comment on the resulting function\n(parse_and_verify_tag_buffer()), explicitly saying the given data _must_\nbe NUL terminated.\n\n\n\nSide note: At first I actually thought the manual NUL termination\ncould cause a buffer overflow (i.e. if the given size was the same as the\nallocated size), so I actually have a version of all of this where I\n_don't_ assume the buffer is NUL-terminated at all, and put in lots of\nbounds checking, replace strchr() with memchr(), etc.\n\nI then took a hard look at read_pipe(), and discovered that if you\nuse it to fill a 4096-byte buffer with 4096 bytes of data, it actually\n_will_ reallocate to 8192 bytes and leave room for the NUL termination\n(and much more) (I believe this should have been documented in read_pipe).\nThus the NUL termination was safe all along.\n\n\n> > -\ttype_line = object + 48;\n> > +\ttype_line = data + 48;\n> \n> Quite a lot of changes seem to do this object->data. The patch would have \n> been much more compact if you just had renamed buffer to object instead of \n> data.\n\nYes, but it would have made the aforementioned diff to parse_tag_buffer()\nlarger.\n\n> > \t/* TODO: check for committer info + blank line? */\n> > \t/* Also, the minimum length is probably + \"tagger .\", or 63+8=71 */\n> > \n> > \t/* The actual stuff afterwards we don't care about.. */\n> > \treturn 0;\n> > -}\n> > \n> > #undef PD_FMT\n> > +}\n> \n> Any particular reason for this?\n\nWell, PD_FMT is only used inside the function, so I found it easier to\nmove the #definition of PD_FMT inside the function to indicate the scope\n(_perceived_ scope; I know it hasn't any effect on the compiled code).\nBut since the whole function is going away in a few patches anyway,\nI should have probably left it out of this patch entirely.\n\n> > @@ -124,6 +120,7 @@ int main(int argc, char **argv)\n> > \t\tfree(buffer);\n> > \t\tdie(\"could not read from stdin\");\n> > \t}\n> > +\tbuffer[size] = 0;\n> \n> Ah, so you terminate the buffer here. From the patch, it is relatively \n> hard to see if this line is always hit _before_ the function is called \n> that evidently relies on NUL termination. By moving it here, I think it is \n> much more likely to overlook the fact that the function, which made sure \n> that its assumption was met, needs this line. Whereas if you left it where \n> it was, the assumption would always be met.\n\nBut if I leave the NUL termination within the function I would have to\nbacktrack out of the function to all of its potential callers and check\nwhether it's safe to write to index size. Since the word \"size\" could\neasily mean \"allocated size\" I would have the initial feeling that this\nmight be a buffer overflow, i.e. _not_ safe.\n\nIn the end, I think the best solution is to make sure NUL termination\nhappens before calling the function, and then documenting explicitly\nthat the function assumes NUL terminated input. Which is exactly what\nI end up with at the end of the patch series.\n\n> > -\tsig_line++;\n> > +\ttagger_line++;\n> \n> I am really reluctant with renamings like these. IMHO they don't buy you \n> much, except for possible confusion. It is evident that sig means the \n> signer (and it is obvious in the case of an unsigned tag, who is meant, \n> too).\n\nHmm. The \"type\" line is found in the variable type_line, the \"tag\" line is\nfound in the variable tag_line, and the \"tagger\" line is found in the\nvariable ... sig_line? Nope, I don't buy it.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44462","messageId":"7vwsydf1m8.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706090213.16031.johan@herland.net","subject":"Re: [PATCH 02/21] Return error messages when parsing fails.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-09T18:01:51Z","receivedAt":"2007-06-09T18:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> This patch brings the already similar tag.c:parse_tag_buffer() and\n> mktag.c:verify_tag() a little bit closer to eachother.\n\nWhile I would agree that it makes sense to have the same\ndefinition of what is and is not a 100% well formatted tag\nobject for producer side and consumer side, I do not necessarily\nthink it is a good idea to make parse_tag_buffer() chattier on\nerrors. mktag.c:verify_tag() can afford to be verbose in its\ndiagnosis, because it is used when the user is _creating_ the\ntag, and it is generally a good idea to be strict when we\ncreate.\n\nOn the other hand, parse_tag_buffer() is on the side of users\nwho use existing tag objects that were produced by somebody\nelse. It is generally a good practice to be more lenient when\nyou are consuming.\n\nAlso, callers of parse_tag_buffer() know the function is silent\non errors (unless there is something seriously wrong with the\nrepository); they do their own error messages when they get an\nerror return.\n"},{"id":"44464","messageId":"7vr6olf1m7.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706090214.11589.johan@herland.net","subject":"Re: [PATCH 04/21] Refactor verification of \"tagger\" line to be more similar to verification of \"type\" and \"tagger\" lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-09T18:01:52Z","receivedAt":"2007-06-09T18:01:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Also update selftests to reflect that verification of \"tagger\" now\n> happens _before_ verification of type name, object sha1 and tag name.\n>\n> Signed-off-by: Johan Herland \n> ---\n> mktag.c | 16 ++++++++--------\n> t/t3800-mktag.sh | 3 +++\n> tag.c | 6 +++---\n> 3 files changed, 14 insertions(+), 11 deletions(-)\n>\n> diff --git a/mktag.c b/mktag.c\n> index 0bc20c8..4dbefab 100644\n> --- a/mktag.c\n> +++ b/mktag.c\n> @@ -62,12 +62,18 @@ static int verify_tag(char *data, unsigned long size)\n> \n> \t/* Verify tag-line */\n> \ttag_line = strchr(type_line, '\\n');\n> -\tif (!tag_line)\n> +\tif (!tag_line++)\n> \t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - data);\n> -\ttag_line++;\n\nCode churn \"while we are at it\" makes reviewing the rest more\ncumbersome. A clean-up like this should be a separate patch.\n\n> diff --git a/tag.c b/tag.c\n> index 8d31603..19c66cd 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -73,10 +73,10 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n> \tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n> \t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - data);\n> \n> +\t/* Verify the tagger line */\n> \ttagger_line = strchr(tag_line, '\\n');\n> -\tif (!tagger_line)\n> -\t\treturn -1;\n> -\ttagger_line++;\n> +\tif (!tagger_line++)\n> +\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", tag_line - data);\n> \n> \t/* Get the actual type */\n> \ttype_len = tag_line - type_line - strlen(\"type \\n\");\n\nSame comments on extra verbosity, as for [2/21].\n"},{"id":"44463","messageId":"7vlketf1m6.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706090214.39337.johan@herland.net","subject":"Re: [PATCH 05/21] Make parse_tag_buffer_internal() handle item == NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-09T18:01:53Z","receivedAt":"2007-06-09T18:01:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> This is in preparation for unifying verify_tag() and\n> parse_tag_buffer_internal().\n>\n> Signed-off-by: Johan Herland \n> ---\n> tag.c | 54 +++++++++++++++++++++++++++++-------------------------\n> 1 files changed, 29 insertions(+), 25 deletions(-)\n>\n> diff --git a/tag.c b/tag.c\n> index 19c66cd..b134967 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n> \tconst char *type_line, *tag_line, *tagger_line;\n> \tunsigned long type_len, tag_len;\n> \n> -\tif (item->object.parsed)\n> -\t\treturn 0;\n> -\titem->object.parsed = 1;\n> +\tif (item) {\n> +\t\tif (item->object.parsed)\n> +\t\t\treturn 0;\n> +\t\titem->object.parsed = 1;\n> +\t}\n> \n> \tif (size < 64)\n> \t\treturn error(\"failed preliminary size check\");\n\nPassing both item and data does not feel right. If you are\ntrying to make the factored out function to do the verification\nof data, then perhaps the caller should do the \"don't handle the\nsame data twice\" optimization using item?\n"},{"id":"44476","messageId":"200706092028.54459.johan@herland.net","threadId":"8491","inReplyTo":"7vwsydf1m8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 02/21] Return error messages when parsing fails.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T18:28:54Z","receivedAt":"2007-06-09T18:28:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Junio C Hamano wrote:\n> Johan Herland writes:\n> \n> > This patch brings the already similar tag.c:parse_tag_buffer() and\n> > mktag.c:verify_tag() a little bit closer to eachother.\n> \n> While I would agree that it makes sense to have the same\n> definition of what is and is not a 100% well formatted tag\n> object for producer side and consumer side, I do not necessarily\n> think it is a good idea to make parse_tag_buffer() chattier on\n> errors. mktag.c:verify_tag() can afford to be verbose in its\n> diagnosis, because it is used when the user is _creating_ the\n> tag, and it is generally a good idea to be strict when we\n> create.\n> \n> On the other hand, parse_tag_buffer() is on the side of users\n> who use existing tag objects that were produced by somebody\n> else. It is generally a good practice to be more lenient when\n> you are consuming.\n> \n> Also, callers of parse_tag_buffer() know the function is silent\n> on errors (unless there is something seriously wrong with the\n> repository); they do their own error messages when they get an\n> error return.\n> \n> \n\nOk. I can make the error messages conditional on 'thorough_verify'.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44481","messageId":"200706092142.05446.johan@herland.net","threadId":"8491","inReplyTo":"200706092028.54459.johan@herland.net","subject":"[PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T19:42:05Z","receivedAt":"2007-06-09T19:42:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"We don't want to print error message on regular parsing of tag objects.\n\nWith this patch error messages are only printed when 'thorough_verify'\nis set, i.e. when creating new or fscking tag objects.\n\nCc: Junio C Hamano \nSigned-off-by: Johan Herland \n---\n\nOn Saturday 09 June 2007, Johan Herland wrote:\n> On Saturday 09 June 2007, Junio C Hamano wrote:\n> > While I would agree that it makes sense to have the same\n> > definition of what is and is not a 100% well formatted tag\n> > object for producer side and consumer side, I do not necessarily\n> > think it is a good idea to make parse_tag_buffer() chattier on\n> > errors. mktag.c:verify_tag() can afford to be verbose in its\n> > diagnosis, because it is used when the user is _creating_ the\n> > tag, and it is generally a good idea to be strict when we\n> > create.\n> > \n> > On the other hand, parse_tag_buffer() is on the side of users\n> > who use existing tag objects that were produced by somebody\n> > else. It is generally a good practice to be more lenient when\n> > you are consuming.\n> > \n> > Also, callers of parse_tag_buffer() know the function is silent\n> > on errors (unless there is something seriously wrong with the\n> > repository); they do their own error messages when they get an\n> > error return.\n> \n> Ok. I can make the error messages conditional on 'thorough_verify'.\n\nIs this one ok?\n\n(It goes on top of the patch series as a whole)\n\n\n...Johan\n\n tag.c | 34 +++++++++++++++++++---------------\n 1 files changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex af4356e..c3a2855 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -67,6 +67,8 @@ int parse_and_verify_tag_buffer(struct tag *item,\n #define PD_FMT \"%td\"\n #endif\n \n+#define FAIL(...) ( thorough_verify ? error(__VA_ARGS__) : -1 )\n+\n \tunsigned char sha1[20];\n \tchar type[20];\n \tconst char *type_line, *tag_line, *keywords_line, *tagger_line;\n@@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t}\n \n \tif (size < 65)\n-\t\treturn error(\"Tag object failed preliminary size check\");\n+\t\treturn FAIL(\"Tag object failed preliminary size check\");\n \n \t/* Verify mandatory object line */\n \tif (prefixcmp(data, \"object \"))\n-\t\treturn error(\"Tag object (@ char 0): \"\n+\t\treturn FAIL(\"Tag object (@ char 0): \"\n \t\t\t\"Does not start with \\\"object \\\"\");\n \n \tif (get_sha1_hex(data + 7, sha1))\n-\t\treturn error(\"Tag object (@ char 7): Could not get SHA1 hash\");\n+\t\treturn FAIL(\"Tag object (@ char 7): Could not get SHA1 hash\");\n \n \t/* Verify mandatory type line */\n \ttype_line = data + 48;\n \tif (prefixcmp(type_line - 1, \"\\ntype \"))\n-\t\treturn error(\"Tag object (@ char 47): \"\n+\t\treturn FAIL(\"Tag object (@ char 47): \"\n \t\t\t\"Could not find \\\"\\\\ntype \\\"\");\n \n \t/* Verify optional tag line */\n \ttag_line = strchr(type_line, '\\n');\n \tif (!tag_line++)\n-\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\"Could not find \\\"\\\\n\\\" after \\\"type\\\"\",\n \t\t\ttype_line - data);\n \tif (prefixcmp(tag_line, \"tag \")) /* no tag name given */\n@@ -107,7 +109,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \telse { /* tag name given */\n \t\tkeywords_line = strchr(tag_line, '\\n');\n \t\tif (!keywords_line++)\n-\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"\\\\n\\\" after \\\"tag\\\"\",\n \t\t\t\ttag_line - data);\n \t}\n@@ -118,7 +120,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \telse { /* keywords given */\n \t\ttagger_line = strchr(keywords_line, '\\n');\n \t\tif (!tagger_line++)\n-\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"\\\\n\\\" after \\\"keywords\\\"\",\n \t\t\t\tkeywords_line - data);\n \t}\n@@ -132,7 +134,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t */\n \tif (prefixcmp(tagger_line, \"tagger \")) { /* no tagger given */\n \t\tif (thorough_verify)\n-\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"tagger \\\"\",\n \t\t\t\ttagger_line - data);\n \t\theader_end = tagger_line;\n@@ -140,7 +142,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \telse { /* tagger given */\n \t\theader_end = strchr(tagger_line, '\\n');\n \t\tif (!header_end++)\n-\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Could not find \\\"\\\\n\\\" after \\\"tagger\\\"\",\n \t\t\t\ttagger_line - data);\n \t}\n@@ -164,7 +166,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \t/* Get the actual type */\n \tif (type_len >= sizeof(type))\n-\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\"Type too long\", type_line - data);\n \tmemcpy(type, type_line, type_len);\n \ttype[type_len] = '\\0';\n@@ -174,7 +176,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \t\t/* Verify that the object matches */\n \t\tif (verify_object(sha1, type))\n-\t\t\treturn error(\"Tag object (@ char 7): \"\n+\t\t\treturn FAIL(\"Tag object (@ char 7): \"\n \t\t\t\t\"Could not verify tagged object %s\",\n \t\t\t\tsha1_to_hex(sha1));\n \n@@ -184,7 +186,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\tunsigned char c = tag_line[i];\n \t\t\t\tif (c > ' ' && c != 0x7f)\n \t\t\t\t\tcontinue;\n-\t\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\t\"Could not verify tag name\",\n \t\t\t\t\ttag_line + i - data);\n \t\t\t}\n@@ -199,12 +201,12 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t\t\tunsigned char c = keywords_line[i];\n \t\t\t\tif (c == ',' && keywords_line[i + 1] == ',')\n \t\t\t\t\t/* consecutive commas */\n-\t\t\t\t\treturn error(\"Tag object (@ char \"\n+\t\t\t\t\treturn FAIL(\"Tag object (@ char \"\n \t\t\t\t\t\tPD_FMT \"): Found empty keyword\",\n \t\t\t\t\t\tkeywords_line + i - data);\n \t\t\t\tif (c > ' ' && c != 0x7f)\n \t\t\t\t\tcontinue;\n-\t\t\t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\t\"Could not verify keywords\",\n \t\t\t\t\tkeywords_line + i - data);\n \t\t\t}\n@@ -248,7 +250,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t} else if (!strcmp(type, tag_type)) {\n \t\t\titem->tagged = &lookup_tag(sha1)->object;\n \t\t} else {\n-\t\t\terror(\"Tag object (@ char \" PD_FMT \"): \"\n+\t\t\tFAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\"Unknown type '%s'\", type_line - data, type);\n \t\t\titem->tagged = NULL;\n \t\t}\n@@ -262,6 +264,8 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n \treturn 0;\n \n+#undef FAIL\n+\n #undef PD_FMT\n }\n \n-- \n1.5.2.1.144.gabc40\n"},{"id":"44489","messageId":"81b0412b0706091426x5cc496aft788376872fc56995@mail.gmail.com","threadId":"8491","inReplyTo":"200706090215.05318.johan@herland.net","subject":"Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T21:26:13Z","receivedAt":"2007-06-09T21:26:13Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> Signed-off-by: Johan Herland \n> ---\n> mktag.c | 29 ++++++++++++++++-------------\n> 1 files changed, 16 insertions(+), 13 deletions(-)\n\nWhat is this change good for?\nHow did you justify the type selection for your\nloop index variable?\n\nIOW, the patch looks very useless.\n"},{"id":"44490","messageId":"81b0412b0706091431h3a786aaew342693a667938d9@mail.gmail.com","threadId":"8491","inReplyTo":"200706090215.31731.johan@herland.net","subject":"Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T21:31:51Z","receivedAt":"2007-06-09T21:31:51Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> + /* Verify the tag-name: we don't allow control characters or spaces in it */\n> + for (i = 4;;) {\n> + unsigned char c = tag_line[i++];\n> + if (c == '\\n')\n> + break;\n> + if (c > ' ')\n> + continue;\n> + return error(\"char\" PD_FMT \": could not verify tag name\", tag_line + i - data);\n> + }\n\nThis looks very familiar. Haven't you just made a very useless patch\nwhich had this very same code? How about putting it in its own\nfunction and just call it from these two places? And what problem\ndo you have with pointers?!\n"},{"id":"44491","messageId":"200706092334.32855.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091426x5cc496aft788376872fc56995@mail.gmail.com","subject":"Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T21:34:32Z","receivedAt":"2007-06-09T21:34:32Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > Signed-off-by: Johan Herland \n> > ---\n> > mktag.c | 29 ++++++++++++++++-------------\n> > 1 files changed, 16 insertions(+), 13 deletions(-)\n> \n> What is this change good for?\n> How did you justify the type selection for your\n> loop index variable?\n> \n> IOW, the patch looks very useless.\n\nI agree. By itself, the patch is useless.\n\nHowever, if you look at the next patch, you'll see that this exact piece of \ncode is moved from verify_tag() to parse_and_verify_tag_buffer(), and in \nthe new context, we can't increment tag_line, since the code that follows \ndepends on tag_line not being moved.\n\nIn other words this patch is here so that the next patch will be easier to \nfollow. because it's _literally_ moving copying code from verify_tag() and \npasting it in parse_and_verify_tag_buffer().\n\nI'm sorry if this is not clear from the patches.\n\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44496","messageId":"81b0412b0706091437j50070e9aw7c258f136f4d5cde@mail.gmail.com","threadId":"8491","inReplyTo":"200706090216.41856.johan@herland.net","subject":"Re: [PATCH 10/21] Free mktag's buffer before dying","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T21:37:26Z","receivedAt":"2007-06-09T21:37:26Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {\n> + free(buffer);\n> + die(\"invalid tag data file\");\n\nThis, and the similar one below are useless. You're destroying the\nprocess, what do you free that buffer for? Either handle the error\ncase or do not needlessly complicate your change, which really\nalso absolutely unneeded.\n"},{"id":"44497","messageId":"81b0412b0706091439q7be180celb939f694b80df159@mail.gmail.com","threadId":"8491","inReplyTo":"200706090216.18823.johan@herland.net","subject":"Re: [PATCH 09/21] Remove unneeded code from mktag.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T21:39:00Z","receivedAt":"2007-06-09T21:39:00Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> Signed-off-by: Johan Herland \n> ---\n> mktag.c | 94 ---------------------------------------------------------------\n> 1 files changed, 0 insertions(+), 94 deletions(-)\n\nSo, you do some useless changes just to remove the\nfunction completely afterwards?\n"},{"id":"44498","messageId":"200706092339.12821.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091431h3a786aaew342693a667938d9@mail.gmail.com","subject":"Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T21:39:12Z","receivedAt":"2007-06-09T21:39:12Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > + /* Verify the tag-name: we don't allow control characters or spaces in it */\n> > + for (i = 4;;) {\n> > + unsigned char c = tag_line[i++];\n> > + if (c == '\\n')\n> > + break;\n> > + if (c > ' ')\n> > + continue;\n> > + return error(\"char\" PD_FMT \": could not verify tag name\", tag_line + i - data);\n> > + }\n> \n> This looks very familiar. Haven't you just made a very useless patch\n> which had this very same code? How about putting it in its own\n> function and just call it from these two places? And what problem\n> do you have with pointers?!\n\nI just answered your comment on the previous patch, and that answer should\napply here as well.\n\nI'm probably splitting this up into too small pieces, since I keep getting\ncomments that fail to see the overall picture of what I'm trying to do,\nnamely taking two similar pieces of code and slowly unifying them to the\npoint where I can replace one of them by a call to the other (see the two\nnext patches).\n\nHope this helps,\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44499","messageId":"81b0412b0706091442m6594260btd3b898bfb63e1fbb@mail.gmail.com","threadId":"8491","inReplyTo":"200706090217.49818.johan@herland.net","subject":"Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T21:42:40Z","receivedAt":"2007-06-09T21:42:40Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> +/*\n> + * Perform parsing and verification of tag object data.\n> + *\n> + * The 'item' parameter may be set to NULL if only verification is desired.\n> + *\n> + * The given data _must_ be null-terminated.\n> + */\n> int parse_and_verify_tag_buffer(struct tag *item,\n> const char *data, const unsigned long size, int thorough_verify)\n\nThis hunk really belongs into commit which introduced the function\nparse_and_verify_tag_buffer.\n"},{"id":"44500","messageId":"200706092342.57273.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091439q7be180celb939f694b80df159@mail.gmail.com","subject":"Re: [PATCH 09/21] Remove unneeded code from mktag.c","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T21:42:57Z","receivedAt":"2007-06-09T21:42:57Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > Signed-off-by: Johan Herland \n> > ---\n> > mktag.c | 94 ---------------------------------------------------------------\n> > 1 files changed, 0 insertions(+), 94 deletions(-)\n> \n> So, you do some useless changes just to remove the\n> function completely afterwards?\n\nYes. Basically so that people can follow my process. If you don't want the\nintermediary/useless states, just look at my first patch series that was\nreplaced by this series because it was too bulky and disruptive to follow.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44501","messageId":"200706092346.12359.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091437j50070e9aw7c258f136f4d5cde@mail.gmail.com","subject":"Re: [PATCH 10/21] Free mktag's buffer before dying","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T21:46:12Z","receivedAt":"2007-06-09T21:46:12Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {\n> > + free(buffer);\n> > + die(\"invalid tag data file\");\n> \n> This, and the similar one below are useless. You're destroying the\n> process, what do you free that buffer for? Either handle the error\n> case or do not needlessly complicate your change, which really\n> also absolutely unneeded.\n\nWell, I was taught to treat my memory with care.\n\nRight now it doesn't make any difference in practice (except that\nValgrind might be a bit happier with it), but in the future -- with\nthe libifaction effort and whatnot -- you never know what might happen\nto this piece of code, and I'd like to stay on the safe side.\n\nFeel free to drop this patch from the series if I'm the only one thinking\nlike this.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44502","messageId":"200706092347.08729.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091442m6594260btd3b898bfb63e1fbb@mail.gmail.com","subject":"Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T21:47:08Z","receivedAt":"2007-06-09T21:47:08Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > +/*\n> > + * Perform parsing and verification of tag object data.\n> > + *\n> > + * The 'item' parameter may be set to NULL if only verification is desired.\n> > + *\n> > + * The given data _must_ be null-terminated.\n> > + */\n> > int parse_and_verify_tag_buffer(struct tag *item,\n> > const char *data, const unsigned long size, int thorough_verify)\n> \n> This hunk really belongs into commit which introduced the function\n> parse_and_verify_tag_buffer.\n\nYes. I'm sorry it slipped out of that patch and into this one.\n\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44503","messageId":"81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com","threadId":"8491","inReplyTo":"200706090219.37289.johan@herland.net","subject":"Re: [PATCH 16/21] Introduce optional \"keywords\" on tag objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T21:52:29Z","receivedAt":"2007-06-09T21:52:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> This patch introduces a new optional header line to the tag object, called\n> \"keywords\". The \"keywords\" line may contain a comma-separated list of\n> custom keywords associated with the tag object.\n\nWhat is the character set for the keywords?\n\n> + for (i = 0; i < keywords_len; ++i) {\n> + unsigned char c = keywords_line[i];\n> + if (c == ',' && keywords_line[i + 1] == ',')\n> + /* consecutive commas */\n> + return error(\"Tag object (@ char \"\n> + PD_FMT \"): Found empty keyword\",\n> + keywords_line + i - data);\n> + if (c > ' ' && c != 0x7f)\n> + continue;\n\nAnd what is so special about 0x7f?\n\n\n> + if (keywords_len) { /* optional keywords string was given */\n> + item->keywords = xmalloc(keywords_len + 1);\n\nWho frees the keywords and what's wrong with strndup?\n"},{"id":"44504","messageId":"81b0412b0706091500o2c0a8f88xd3808d6ee790d777@mail.gmail.com","threadId":"8491","inReplyTo":"200706092346.12359.johan@herland.net","subject":"Re: [PATCH 10/21] Free mktag's buffer before dying","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-06-09T22:00:27Z","receivedAt":"2007-06-09T22:00:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 6/9/07, Johan Herland wrote:\n> On Saturday 09 June 2007, Alex Riesen wrote:\n> > On 6/9/07, Johan Herland wrote:\n> > > + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {\n> > > + free(buffer);\n> > > + die(\"invalid tag data file\");\n> >\n> > This, and the similar one below are useless. You're destroying the\n> > process, what do you free that buffer for? Either handle the error\n> > case or do not needlessly complicate your change, which really\n> > also absolutely unneeded.\n>\n> Well, I was taught to treat my memory with care.\n>\n\nHow do you treat your performance?\nBesides, was that systems with common address space\nwhere you were taught? Like DOS or MacOS, perhaps?\n\n> Right now it doesn't make any difference in practice (except that\n> Valgrind might be a bit happier with it), but in the future -- with\n> the libifaction effort and whatnot -- you never know what might happen\n> to this piece of code, and I'd like to stay on the safe side.\n\nSo that people have to check your free as well (they will have to,\nthey come looking for die-calls). You just made more work for them.\n"},{"id":"44505","messageId":"200706100000.45476.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com","subject":"Re: [PATCH 16/21] Introduce optional \"keywords\" on tag objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T22:00:45Z","receivedAt":"2007-06-09T22:00:45Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 09 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > This patch introduces a new optional header line to the tag object, called\n> > \"keywords\". The \"keywords\" line may contain a comma-separated list of\n> > custom keywords associated with the tag object.\n> \n> What is the character set for the keywords?\n\nHmm. Same as for the \"tag\" line, and the rest of the tag object, I guess.\nI agree this should probably be specified. What are the rules for other\ngit objects?\n\n> > + for (i = 0; i < keywords_len; ++i) {\n> > + unsigned char c = keywords_line[i];\n> > + if (c == ',' && keywords_line[i + 1] == ',')\n> > + /* consecutive commas */\n> > + return error(\"Tag object (@ char \"\n> > + PD_FMT \"): Found empty keyword\",\n> > + keywords_line + i - data);\n> > + if (c > ' ' && c != 0x7f)\n> > + continue;\n> \n> And what is so special about 0x7f?\n\nIsn't DEL a control char?\n\n> > + if (keywords_len) { /* optional keywords string was given */\n> > + item->keywords = xmalloc(keywords_len + 1);\n> \n> Who frees the keywords and what's wrong with strndup?\n\nThe code that should free the tag name should also free the keywords.\nNo code appear to do this at the moment (regardless of whether you use my patch\nseries or not), which is a shame. We should make sure all objects are properly\ndeallocated. This is currently a general problem in git, isn't it?\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44506","messageId":"200706100005.11710.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091500o2c0a8f88xd3808d6ee790d777@mail.gmail.com","subject":"Re: [PATCH 10/21] Free mktag's buffer before dying","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T22:05:11Z","receivedAt":"2007-06-09T22:05:11Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sunday 10 June 2007, Alex Riesen wrote:\n> On 6/9/07, Johan Herland wrote:\n> > On Saturday 09 June 2007, Alex Riesen wrote:\n> > > On 6/9/07, Johan Herland wrote:\n> > > > + if (parse_and_verify_tag_buffer(0, buffer, size, 1)) {\n> > > > + free(buffer);\n> > > > + die(\"invalid tag data file\");\n> > >\n> > > This, and the similar one below are useless. You're destroying the\n> > > process, what do you free that buffer for? Either handle the error\n> > > case or do not needlessly complicate your change, which really\n> > > also absolutely unneeded.\n> >\n> > Well, I was taught to treat my memory with care.\n> \n> How do you treat your performance?\n\nHopefully with care, as well. However, I tend to look at performance _after_\ncorrectness.\n\n> Besides, was that systems with common address space\n> where you were taught? Like DOS or MacOS, perhaps?\n\nNope. Never programmed on either. I thought care with memory was generally\nconsidered a good principle. If I'm wrong, please point me at the relevant\ndocumentation.\n\n> > Right now it doesn't make any difference in practice (except that\n> > Valgrind might be a bit happier with it), but in the future -- with\n> > the libifaction effort and whatnot -- you never know what might happen\n> > to this piece of code, and I'd like to stay on the safe side.\n> \n> So that people have to check your free as well (they will have to,\n> they come looking for die-calls). You just made more work for them.\n\nOk. Drop it. This isn't particularily important to me. I just try to\nfollow good principles when I can.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44508","messageId":"200706100036.43894.johan@herland.net","threadId":"8491","inReplyTo":"81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com","subject":"[PATCH] Use xstrndup() instead of xmalloc() and memcpy(); fix buglet with generating default item->keywords.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-09T22:36:43Z","receivedAt":"2007-06-09T22:36:43Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Using xstrndup() yields more compact and readable code than using\nxmalloc(), memcpy() and manual NUL termination.\nThanks to Alex Riesen for suggesting this.\n\nAlso fixes a buglet where item->keywords would always be set to \"tag\",\neven if item->tag was empty.\n\nSigned-off-by: Johan Herland \n---\n\nOn Saturday 09 June 2007, Alex Riesen wrote:\n> ... and what's wrong with strndup?\n\nNothing. \n\n\n...Johan\n\n tag.c | 27 ++++++++++-----------------\n 1 files changed, 10 insertions(+), 17 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex c3a2855..2307ec9 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -219,26 +219,19 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t}\n \n \tif (item) { /* Store parsed information into item */\n-\t\tif (tag_len) { /* optional tag name was given */\n-\t\t\titem->tag = xmalloc(tag_len + 1);\n-\t\t\tmemcpy(item->tag, tag_line, tag_len);\n-\t\t\titem->tag[tag_len] = '\\0';\n-\t\t}\n-\t\telse { /* optional tag name not given */\n-\t\t\titem->tag = xmalloc(1);\n-\t\t\titem->tag[0] = '\\0';\n-\t\t}\n+\t\tif (tag_len) /* optional tag name was given */\n+\t\t\titem->tag = xstrndup(tag_line, tag_len);\n+\t\telse /* optional tag name not given */\n+\t\t\titem->tag = xstrndup(\"\", 0);\n \n-\t\tif (keywords_len) { /* optional keywords string was given */\n-\t\t\titem->keywords = xmalloc(keywords_len + 1);\n-\t\t\tmemcpy(item->keywords, keywords_line, keywords_len);\n-\t\t\titem->keywords[keywords_len] = '\\0';\n-\t\t}\n+\t\tif (keywords_len) /* optional keywords string was given */\n+\t\t\titem->keywords = xstrndup(keywords_line, keywords_len);\n \t\telse { /* optional keywords string not given. Set default */\n \t\t\t/* if tag name is set, use \"tag\"; else use \"note\" */\n-\t\t\tconst char *default_kw = item->tag ? \"tag\" : \"note\";\n-\t\t\titem->keywords = xmalloc(strlen(default_kw) + 1);\n-\t\t\tmemcpy(item->keywords, default_kw, strlen(default_kw) + 1);\n+\t\t\tif (*(item->tag))\n+\t\t\t\titem->keywords = xstrndup(\"tag\", 3);\n+\t\t\telse\n+\t\t\t\titem->keywords = xstrndup(\"note\", 4);\n \t\t}\n \n \t\tif (!strcmp(type, blob_type)) {\n-- \n1.5.2.1.144.gabc40\n"},{"id":"44515","messageId":"7vfy50d67q.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com","subject":"Re: [PATCH 16/21] Introduce optional \"keywords\" on tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-10T00:05:29Z","receivedAt":"2007-06-10T00:05:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> On 6/9/07, Johan Herland wrote:\n>> This patch introduces a new optional header line to the tag object, called\n>> \"keywords\". The \"keywords\" line may contain a comma-separated list of\n>> custom keywords associated with the tag object.\n>\n> What is the character set for the keywords?\n>\n>> + for (i = 0; i < keywords_len; ++i) {\n>> + unsigned char c = keywords_line[i];\n>> + if (c == ',' && keywords_line[i + 1] == ',')\n>> + /* consecutive commas */\n>> + return error(\"Tag object (@ char \"\n>> + PD_FMT \"): Found empty keyword\",\n>> + keywords_line + i - data);\n>> + if (c > ' ' && c != 0x7f)\n>> + continue;\n>\n> And what is so special about 0x7f?\n\nIt is DEL, but as the code uses uchar, it probably also error on\n0x80 or higher, if the intent is \"printable ASCII\".\n"},{"id":"44519","messageId":"200706100235.24358.johan@herland.net","threadId":"8491","inReplyTo":"7vfy50d67q.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Fail if tag name and keywords is not within \"printable ASCII\"","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T00:35:24Z","receivedAt":"2007-06-10T00:35:24Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n\nOn Sunday 10 June 2007, Junio C Hamano wrote:\n> \"Alex Riesen\" writes:\n> > And what is so special about 0x7f?\n> \n> It is DEL, but as the code uses uchar, it probably also error on\n> 0x80 or higher, if the intent is \"printable ASCII\".\n\nIs this better?\n\n\n...Johan\n\n tag.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 2307ec9..2b92465 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -183,8 +183,8 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t/* Verify tag name: disallow control characters or spaces */\n \t\tif (tag_len) { /* tag name was given */\n \t\t\tfor (i = 0; i < tag_len; ++i) {\n-\t\t\t\tunsigned char c = tag_line[i];\n-\t\t\t\tif (c > ' ' && c != 0x7f)\n+\t\t\t\tchar c = tag_line[i];\n+\t\t\t\tif (c > ' ' && c < 0x7f)\n \t\t\t\t\tcontinue;\n \t\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\t\"Could not verify tag name\",\n@@ -198,13 +198,13 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \t\t */\n \t\tif (keywords_len) { /* keywords line was given */\n \t\t\tfor (i = 0; i < keywords_len; ++i) {\n-\t\t\t\tunsigned char c = keywords_line[i];\n+\t\t\t\tchar c = keywords_line[i];\n \t\t\t\tif (c == ',' && keywords_line[i + 1] == ',')\n \t\t\t\t\t/* consecutive commas */\n \t\t\t\t\treturn FAIL(\"Tag object (@ char \"\n \t\t\t\t\t\tPD_FMT \"): Found empty keyword\",\n \t\t\t\t\t\tkeywords_line + i - data);\n-\t\t\t\tif (c > ' ' && c != 0x7f)\n+\t\t\t\tif (c > ' ' && c < 0x7f)\n \t\t\t\t\tcontinue;\n \t\t\t\treturn FAIL(\"Tag object (@ char \" PD_FMT \"): \"\n \t\t\t\t\t\"Could not verify keywords\",\n-- \n1.5.2.1.144.gabc40\n\n\t\n"},{"id":"44520","messageId":"200706100245.22292.johan@herland.net","threadId":"8491","inReplyTo":"7vlketf1m6.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Move check for already parsed tag object to parse_tag_buffer() wrapper function","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T00:45:22Z","receivedAt":"2007-06-10T00:45:22Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This effectively disables the check when creating and fscking tag objects,\nwhich is desirable because we don't want the fact that the object is\nalready parsed to stop us from verifying thoroughly.\n\nSigned-off-by: Johan Herland \n---\n\nOn Saturday 09 June 2007, Junio C Hamano wrote:\n> Johan Herland writes:\n> > diff --git a/tag.c b/tag.c\n> > index 19c66cd..b134967 100644\n> > --- a/tag.c\n> > +++ b/tag.c\n> > @@ -46,9 +46,11 @@ static int parse_tag_buffer_internal(struct tag *item, const char *data, const u\n> > \tconst char *type_line, *tag_line, *tagger_line;\n> > \tunsigned long type_len, tag_len;\n> > \n> > -\tif (item->object.parsed)\n> > -\t\treturn 0;\n> > -\titem->object.parsed = 1;\n> > +\tif (item) {\n> > +\t\tif (item->object.parsed)\n> > +\t\t\treturn 0;\n> > +\t\titem->object.parsed = 1;\n> > +\t}\n>\n> Passing both item and data does not feel right. If you are\n> trying to make the factored out function to do the verification\n> of data, then perhaps the caller should do the \"don't handle the\n> same data twice\" optimization using item?\n\nYou mean, like this?\n\n\n...Johan\n\n tag.c | 10 ++++------\n 1 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 2b92465..4e79943 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -75,12 +75,6 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \tunsigned long type_len, tag_len, keywords_len, tagger_len;\n \tconst char *header_end;\n \n-\tif (item) {\n-\t\tif (item->object.parsed)\n-\t\t\treturn 0;\n-\t\titem->object.parsed = 1;\n-\t}\n-\n \tif (size < 65)\n \t\treturn FAIL(\"Tag object failed preliminary size check\");\n \n@@ -264,6 +258,10 @@ int parse_and_verify_tag_buffer(struct tag *item,\n \n int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n {\n+\tif (item->object.parsed)\n+\t\treturn 0;\n+\titem->object.parsed = 1;\n+\n \treturn parse_and_verify_tag_buffer(item, (const char *) data, size, 0);\n }\n \n-- \n1.5.2.1.144.gabc40\n"},{"id":"44523","messageId":"7vwsycbnk1.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706100235.24358.johan@herland.net","subject":"Re: [PATCH] Fail if tag name and keywords is not within \"printable ASCII\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-10T01:33:50Z","receivedAt":"2007-06-10T01:33:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> Signed-off-by: Johan Herland \n> ---\n>\n> On Sunday 10 June 2007, Junio C Hamano wrote:\n>> \"Alex Riesen\" writes:\n>> > And what is so special about 0x7f?\n>> \n>> It is DEL, but as the code uses uchar, it probably also error on\n>> 0x80 or higher, if the intent is \"printable ASCII\".\n>\n> Is this better?\n\nI said \"*if* the intent is to limit to printable ASCII\".\n\nIt is still unclear what use cases the \"keywords\" need to\nsupport (e.g. do we want to have \"pick tags that have keyword\nthat matches this pattern\"? if so, what kind of pattern\nlanguage do we want to use? Glob? Regexp? Would it be more\nconvenient if the keywords are treated case insensitively? Is\nthere a useful use case if you are allowed to use people's names\nas keywords? Is it reasonable to assume that the keywords can\nbe split with a comma? Do we want to allow a comma as part of\nkeywords? If so, how would we quote a comma that is inside a\nkeyword? etc.). It depends on the answers to these questions\nif \"printable ASCII\" is a good set.\n\nAs a general rule, if you make the initially allowed set of\nvalues too wide, it gets _extremely_ hard to tighten it later.\nSo if we are to stick to printable ASCII (iow, \"no people's\nnames or names of location as keywords\"), I would even suggest\nto limit the valid set further, say \"^[-A-Za-z0-9_+.]+$\", at\nleast initially.\n"},{"id":"44528","messageId":"Pine.LNX.4.64.0706100741310.4059@racer.site","threadId":"8491","inReplyTo":"200706092142.05446.johan@herland.net","subject":"Re: [PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T06:48:35Z","receivedAt":"2007-06-10T06:48:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> +#define FAIL(...) ( thorough_verify ? error(__VA_ARGS__) : -1 )\n> +\n> \tunsigned char sha1[20];\n> \tchar type[20];\n> \tconst char *type_line, *tag_line, *keywords_line, *tagger_line;\n> @@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,\n> \t}\n> \n> \tif (size < 65)\n> -\t\treturn error(\"Tag object failed preliminary size check\");\n> +\t\treturn FAIL(\"Tag object failed preliminary size check\");\n\nThis is ugly.\n\nIf you _have_ to output the error message in one case, and not in the \nother, I'd rather do\n\n\tenum tag_error { TAG_SIZE_CHECK, TAG_BLA_BLUB, ... };\n\tconst char **tag_error_strings = { \"tag: size error\", ... };\n\nOf course, you'd lose the ability to output some numbers. But those \nnumbers that you output are even uglier than the code. Guess how surprised \n_I_ was, when I hit the error message which made me go mad.\n\nHaving said that, I still do not agree in this unifying.\n\nYour rationale seems to be: use the same checking for the tag creation as \nfor the tag validation.\n\nBut this is _wrong_. We _do_ have tags that do not conform to the strict \nstandards of git-tag, and even if we did _not_, it would _still_ be wrong \nto be that strict when _reading_ tags.\n\nTo drive that point home: strict checking when creating tags is good. \nStrict checking when reading tags is bad.\n\nI strongly encourage keeping both validations separate.\n\nYou'd also avoid having that many lines which are well over the encouraged \n80 character limit.\n\nCiao,\nDscho\n"},{"id":"44540","messageId":"Pine.LNX.4.64.0706100846250.4059@racer.site","threadId":"8491","inReplyTo":"7vr6olf1m7.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 04/21] Refactor verification of \"tagger\" line to be more similar to verification of \"type\" and \"tagger\" lines","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T07:49:35Z","receivedAt":"2007-06-10T07:49:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Junio C Hamano wrote:\n\n> Johan Herland writes:\n> \n> > +\tif (!tag_line++)\n> > \t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - data);\n> > -\ttag_line++;\n\nBTW if you _are_ verbosing the output, you might just as well make it \nuseful.\n\nThe common format is \"filename:line[:column]:message\", not \"char[n]\", \nwhich is a misnomer to begin with, since you are talking about an offset, \nnot a char (remember, characters are those things that are displayed in \nplace for a given number, so I fully expected char32 to be a space).\n\nCiao,\nDscho\n"},{"id":"44543","messageId":"Pine.LNX.4.64.0706100905210.4059@racer.site","threadId":"8491","inReplyTo":"200706090214.39337.johan@herland.net","subject":"Re: [PATCH 05/21] Make parse_tag_buffer_internal() handle item == NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:06:37Z","receivedAt":"2007-06-10T08:06:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> -\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n> -\titem->tag = xmalloc(tag_len + 1);\n> -\tmemcpy(item->tag, tag_line + 4, tag_len);\n> -\titem->tag[tag_len] = '\\0';\n> -\n> -\tif (!strcmp(type, blob_type)) {\n> -\t\titem->tagged = &lookup_blob(sha1)->object;\n> -\t} else if (!strcmp(type, tree_type)) {\n> -\t\titem->tagged = &lookup_tree(sha1)->object;\n> -\t} else if (!strcmp(type, commit_type)) {\n> -\t\titem->tagged = &lookup_commit(sha1)->object;\n> -\t} else if (!strcmp(type, tag_type)) {\n> -\t\titem->tagged = &lookup_tag(sha1)->object;\n> -\t} else {\n> -\t\terror(\"Unknown type %s\", type);\n> -\t\titem->tagged = NULL;\n> -\t}\n> -\n> -\tif (item->tagged && track_object_refs) {\n> -\t\tstruct object_refs *refs = alloc_object_refs(1);\n> -\t\trefs->ref[0] = item->tagged;\n> -\t\tset_object_refs(&item->object, refs);\n> +\tif (item) {\n> +\t\ttag_len = tagger_line - tag_line - strlen(\"tag \\n\");\n> +\t\titem->tag = xmalloc(tag_len + 1);\n> +\t\tmemcpy(item->tag, tag_line + 4, tag_len);\n> +\t\titem->tag[tag_len] = '\\0';\n> +\n> +\t\tif (!strcmp(type, blob_type)) {\n> +\t\t\titem->tagged = &lookup_blob(sha1)->object;\n> +\t\t} else if (!strcmp(type, tree_type)) {\n> +\t\t\titem->tagged = &lookup_tree(sha1)->object;\n> +\t\t} else if (!strcmp(type, commit_type)) {\n> +\t\t\titem->tagged = &lookup_commit(sha1)->object;\n> +\t\t} else if (!strcmp(type, tag_type)) {\n> +\t\t\titem->tagged = &lookup_tag(sha1)->object;\n> +\t\t} else {\n> +\t\t\terror(\"Unknown type %s\", type);\n> +\t\t\titem->tagged = NULL;\n> +\t\t}\n> +\n> +\t\tif (item->tagged && track_object_refs) {\n> +\t\t\tstruct object_refs *refs = alloc_object_refs(1);\n> +\t\t\trefs->ref[0] = item->tagged;\n> +\t\t\tset_object_refs(&item->object, refs);\n> +\t\t}\n> \t}\n> \n> \treturn 0;\n\nThe patch would have been so much simpler, and so much more descriptive of \nwhat you're doing, had you just said\n\n\tif (!item)\n\t\treturn 0;\n\nCiao,\nDscho\n"},{"id":"44545","messageId":"Pine.LNX.4.64.0706100909211.4059@racer.site","threadId":"8491","inReplyTo":"200706092334.32855.johan@herland.net","subject":"Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:14:40Z","receivedAt":"2007-06-10T08:14:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> On Saturday 09 June 2007, Alex Riesen wrote:\n> > On 6/9/07, Johan Herland wrote:\n> > > Signed-off-by: Johan Herland \n> > > ---\n> > > mktag.c | 29 ++++++++++++++++-------------\n> > > 1 files changed, 16 insertions(+), 13 deletions(-)\n> > \n> > What is this change good for?\n> > How did you justify the type selection for your\n> > loop index variable?\n> > \n> > IOW, the patch looks very useless.\n> \n> I agree. By itself, the patch is useless.\n\nThen it shouldn't be there.\n\nIt seems that you do not place the cuts between patches at the \n_conceptual_ layer. Therefore, they seem intrusive and often the meaning \nevades me.\n\nSo, if I understood the purpose of this patch series correctly, namely to \nuse the same verification routines both for creation as for validation of \ntags, you could have\n\n\t- moved one function into the library (the stricter one), saying \n\t \"move this_function() into libgit.a to make it usable from \n\t git-bla\" in the commit body,\n\n\t- used that from the other program, removing the now-unused \n\t function,\n\n\t- and then changed the behaviour to be more chatty or some such.\n\nAs it is, you have a mix of conceptually different changes in almost every \npatch, and some changes that conceptually belong into the same patch, are \nnot.\n\nBe that as may, I think it is not a good change to reuse the same function \nlike you did, exactly because one version _should_ be more forgiving than \nthe other.\n\nCiao,\nDscho\n"},{"id":"44544","messageId":"7vwsyc8bt3.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706100741310.4059@racer.site","subject":"Re: [PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-10T08:15:52Z","receivedAt":"2007-06-10T08:15:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sat, 9 Jun 2007, Johan Herland wrote:\n> ...\n>> @@ -80,26 +82,26 @@ int parse_and_verify_tag_buffer(struct tag *item,\n>> \t}\n>> \n>> \tif (size < 65)\n>> -\t\treturn error(\"Tag object failed preliminary size check\");\n>> +\t\treturn FAIL(\"Tag object failed preliminary size check\");\n>\n> This is ugly.\n\n... quite a bit. A less uglier alternative we seem to use in\nother places is not much better (return NULL on failure or an\nerror message string on error).\n\n> ... Guess how surprised \n> _I_ was, when I hit the error message which made me go mad.\n\nTo be fair, that ugly \"char%d\" was taken from mktag and not\nJohan's invention.\n\n> To drive that point home: strict checking when creating tags is good. \n> Strict checking when reading tags is bad.\n>\n> I strongly encourage keeping both validations separate.\n\nWhile I tend ot think that keeping two separate versions is\nprobably better for this particular case, the above statement\nhas a leap in its logic. With your \"error code\" scheme, you\ncould implement a single, verifier/parser that defines the\nconcrete and complete rule of how the data should look like.\nThat unified verifier/parser itself should be silent. Then, you\ncan have each of the callers decide how lenient it wants to be,\ndepending on the seriousness of the error. You can make\nproducer very strict and chatty while leaving consumer liberal\nand more silent.\n\nThere are pros-and-cons, however.\n\n - Such a scheme to return error codes and have two callers that\n have different behaviours is cumbersome to set up and use.\n\n A good example of this is the switch/case mess in each of the\n callers of run_command_v_opt() in builtin-push.c,\n builtin-revert.c, receive-pack.c etc. For run_command, the\n mess is justifiable because the function has enough number of\n different callers, but in the current thread, we are only\n talking about two callers (parsing vs verifying of tag\n objects).\n\n - It has a risk to introduce inconsitent definition of the data\n format to have completely separate producer and consumer\n implementations; this is especially true when the data in\n question is complex.\n\n However, a tag is sufficiently simple that my personal\n feeling is that, combined with the cumbersomeness argument\n against the unified verifier, separate producer and consumer\n implementations would be easier to manage for this particular\n case.\n"},{"id":"44547","messageId":"Pine.LNX.4.64.0706100917290.4059@racer.site","threadId":"8491","inReplyTo":"200706092339.12821.johan@herland.net","subject":"Re: [PATCH 07/21] Copy the remaining differences from verify_tag() to parse_tag_buffer_internal()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:22:28Z","receivedAt":"2007-06-10T08:22:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> I'm probably splitting this up into too small pieces, since I keep \n> getting comments that fail to see the overall picture of what I'm trying \n> to do, [...]\n\nMaybe I said that your patch was too large. But then, I said something \nmuch more important: hard-to-review.\n\nThese small patches, split in a manner making it even more difficult to \nunderstand what you want to accomplish, do not help.\n\nYes, you should make small patches. Even small patch series. But in such a \nfashion that a reviewer can see that it is a good patch[*1*]. Just lean \nback, look at your patches, and ask yourself how you would have reacted if \nyou had reviewed them.\n\nCiao,\nDscho\n\n*1* A good patch follows the immortal words of Saint Exupery: A designer \nknows he has achieved perfection not when there is nothing left to add, \nbut when there is nothing left to take away.\n"},{"id":"44554","messageId":"Pine.LNX.4.64.0706100927050.4059@racer.site","threadId":"8491","inReplyTo":"200706090216.41856.johan@herland.net","subject":"Re: [PATCH 10/21] Free mktag's buffer before dying","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:38:47Z","receivedAt":"2007-06-10T08:38:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\nHow does this:\n\n> -\t\tusage(\"git-mktag < signaturefile\");\n> +\t\tusage(\"git-mktag < tag_data_file\");\n\nand this:\n\n> -\n\nrelate with your commit message?\n\nYou might understand that people _could_ get the impression that the \npatches were not very carefully crafted. Or even worse, the impression, \nthat it was tried to slip some changes by, with a totally unrelated \n\"official\" purpose. (Again, reminds me of politics: it's like \nslipping an intrusive privacy law into an agriculture related law at the \n11th hour, just before Christmas, when people do not really pay \nattention.)\n\nCiao,\nDscho\n"},{"id":"44555","messageId":"Pine.LNX.4.64.0706100933270.4059@racer.site","threadId":"8491","inReplyTo":"200706090217.17795.johan@herland.net","subject":"Re: [PATCH 11/21] Rewrite error messages; fix up line lengths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:38:55Z","receivedAt":"2007-06-10T08:38:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> Also update selftests to reflect new error messages.\n\nHad you split the patch conceptually, the changes to the tests would have \nbeen included where appropriate.\n\nI consider a patch _breaking_ a test case, with a follow up patch to the \ntest case, a _bug_.\n\nAnd I consider a change in error messages not good, _unless_ the changes \nhave a real value in practice. Which I maintain they do not, in this \npatch-series' case.\n\nCiao,\nDscho\n"},{"id":"44557","messageId":"Pine.LNX.4.64.0706100939460.4059@racer.site","threadId":"8491","inReplyTo":"200706090217.49818.johan@herland.net","subject":"Re: [PATCH 12/21] Use prefixcmp() instead of memcmp() for cleaner code with less magic numbers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:41:06Z","receivedAt":"2007-06-10T08:41:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n>\n> -\tif (memcmp(data, \"object \", 7))\n> +\tif (prefixcmp(data, \"object \"))\n\nFWIW I think that _these_ changes are actually somewhat worth it. And they \nshould have come _instead_ of moving the order of the memcmp() around \n(you know which patch that was).\n\nCiao,\nDscho\n"},{"id":"44558","messageId":"Pine.LNX.4.64.0706100942510.4059@racer.site","threadId":"8491","inReplyTo":"200706090218.16281.johan@herland.net","subject":"Re: [PATCH 13/21] Collect skipping of header field names and calculation of line lengths in one place","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:45:09Z","receivedAt":"2007-06-10T08:45:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> +\t/*\n> +\t * Advance header field pointers past their initial identifier.\n> +\t * Calculate lengths of header fields.\n> +\t */\n> +\ttype_line += strlen(\"type \");\n> +\ttype_len = tag_line - type_line - 1;\n> +\ttag_line += strlen(\"tag \");\n> +\ttag_len = tagger_line - tag_line - 1;\n> +\n> \t/* Get the actual type */\n> -\ttype_len = tag_line - type_line - strlen(\"type \\n\");\n> \tif (type_len >= sizeof(type))\n> \t\treturn error(\"Tag object (@ char \" PD_FMT \"): \"\n> -\t\t\t\"Type too long\", type_line + 5 - data);\n> -\tmemcpy(type, type_line + 5, type_len);\n> +\t\t\t\"Type too long\", type_line - data);\n> +\tmemcpy(type, type_line, type_len);\n\nThis change does not clarify anything. It is exactly as confusing as \nbefore.\n\n> -\t\tfor (i = 4;;) {\n> +\t\tfor (i = 0;;) {\n\nI know you introduced this in another patch. This is an ugly construct. If \nyou want people to review your patches, you might as well put in the \neffort to make the reviewing more pleasant.\n\nCiao,\nDscho\n"},{"id":"44560","messageId":"Pine.LNX.4.64.0706100945290.4059@racer.site","threadId":"8491","inReplyTo":"200706090218.41941.johan@herland.net","subject":"Re: [PATCH 14/21] Add proper parsing of \"tagger\" line, but only when thorough_verify is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:52:01Z","receivedAt":"2007-06-10T08:52:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> Be explicit about the fact that the \"tagger\" line is now considered a \n> mandatory part of the tag object. There are however old tags (from \n> before July 2005) that don't have a \"tagger\" line. We therefore consider \n> the \"tagger\" line _optional_ when parsing tags without thorough_verify \n> set.\n\nNo. The \"before July 2005\" part is _not_ the reason that we consider this \nline optional.\n\nThe fact that it is bad to fail on a fetch, just because you happen to \nhave an invalid tag in your repository, is a good reason not to.\n\nThe fact that it is bad to fail on a git branch, just because you happen \nto have an invalid tag in your repository, is a good reason not to.\n\nThe fact that it is bad to fail on an fsck, just because you happen to \nhave an invalid tag in your repository, is a good reason not to.\n\nAnd yes, if I remember correctly, your original patch did exactly that.\n\nThe paradigm to follow is: fail gracefully. I could have an invalid \n_commit_ in my repository, and would still want _every_ Git operation to \nsucceed, _as long_ as it does not touch that bad object.\n\nAnd I damned well want git-fsck to not crash, just because some \nassumptions are made.\n\nSince this is a fundamental critique on your patch series, I will do the \ndetailed review on _this_ patch in another mail.\n\nCiao,\nDscho\n"},{"id":"44563","messageId":"Pine.LNX.4.64.0706100952130.4059@racer.site","threadId":"8491","inReplyTo":"200706090218.41941.johan@herland.net","subject":"Re: [PATCH 14/21] Add proper parsing of \"tagger\" line, but only when thorough_verify is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T08:58:51Z","receivedAt":"2007-06-10T08:58:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> -\tconst char *type_line, *tag_line, *tagger_line;\n> -\tunsigned long type_len, tag_len;\n> +\tconst char *type_line, *tag_line, *tagger_line;\n> +\tunsigned long type_len, tag_len, tagger_len;\n> +\tconst char *header_end;\n\nThis is ugly. Really ugly. Besides, it breaks the minimal patch paradigm.\n\n> @@ -81,7 +82,7 @@ int parse_and_verify_tag_buffer(struct tag *item,\n> \tif (size < 64)\n> \t\treturn error(\"Tag object failed preliminary size check\");\n> \n> -\t/* Verify object line */\n> +\t/* Verify mandatory object line */\n> \tif (prefixcmp(data, \"object \"))\n> \t\treturn error(\"Tag object (@ char 0): \"\n> \t\t\t\"Does not start with \\\"object \\\"\");\n\nHmph. I think everybody reading C code understands that this is mandatory. \nI even think that the comment is useless. It is sort of a \ncode-in-human-language duplicated code.\n\n> -\t/* Verify the tagger line */\n> +\t/*\n> +\t * Verify mandatory tagger line, but only when we're checking\n> +\t * thoroughly, i.e. on inserting a new tag, and on fsck.\n> +\t * There are existing tag objects without a tagger line (most\n> +\t * notably the \"v0.99\" tag in the main git repo), and we don't\n> +\t * want to fail parsing on these.\n> +\t */\n\nI maintain that even with thorough checking, it is _wrong_ to error on a \nmissing tagger. Since we have to deal with tagger-less tags _anyway_, and \nsince it is not like you could really do something about it (the tag is \nimmutable), you should go with a warning.\n\n> -\t * Calculate lengths of header fields.\n> +\t * Calculate lengths of header fields (0 for fields that are not given).\n\nDoes that really make sense? You effectively treat a missing field as if \nit were empty. IMHO that is wrong. Besides, this\n\n> +\ttagger_len = header_end > tagger_line ?\n> +\t\t\t(header_end - tagger_line) - 1 : 0;\n\nis really ugly, what with in-line spaces, _and_ special casing.\n\n> -\t\t/* Verify the tagger line */\n> +\t\t/* Verify tagger line */\n\nHmpf.\n\nCiao,\nDscho\n"},{"id":"44565","messageId":"Pine.LNX.4.64.0706100959420.4059@racer.site","threadId":"8491","inReplyTo":"200706090215.05318.johan@herland.net","subject":"Re: [PATCH 06/21] Refactor tag name verification loop to use index 'i' instead of incrementing pointer 'tag_line'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T09:01:44Z","receivedAt":"2007-06-10T09:01:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> diff --git a/mktag.c b/mktag.c\n> index 4dbefab..2e70504 100644\n> --- a/mktag.c\n> +++ b/mktag.c\n> @@ -81,19 +81,22 @@ static int verify_tag(char *data, unsigned long size)\n> \tmemcpy(type, type_line + 5, type_len);\n> \ttype[type_len] = '\\0';\n> \n> -\t/* Verify that the object matches */\n> -\tif (verify_object(sha1, type))\n> -\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n> -\n> -\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n> -\ttag_line += 4;\n> -\tfor (;;) {\n> -\t\tunsigned char c = *tag_line++;\n> -\t\tif (c == '\\n')\n> -\t\t\tbreak;\n> -\t\tif (c > ' ')\n> -\t\t\tcontinue;\n> -\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - data);\n> +\t{\n> +\t\tunsigned long i;\n\nDo you realize that half of your diff consists of reindenting, just \nbecause you introduced this ugly construct, instead of being a good boy \nand put the declarations where they belong -- at the beginning of the \nfunction (or if it exists, block)?\n\n> +\t\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n> +\t\tfor (i = 4;;) {\n\nYes, you can write this construct. That does not change the fact that it \ngives me eye cancer.\n\nCiao,\nDscho\n"},{"id":"44566","messageId":"Pine.LNX.4.64.0706101004440.4059@racer.site","threadId":"8491","inReplyTo":"200706090219.12011.johan@herland.net","subject":"Re: [PATCH 15/21] Make tag names (i.e. the tag object's \"tag\" line) optional","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T09:07:19Z","receivedAt":"2007-06-10T09:07:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Jun 2007, Johan Herland wrote:\n\n> The tag line is now optional. If not given in the tag object data, it\n> defaults to the empty string (\"\") in the parsed tag object.\n> \n> The patch also adds a change to git-show; when asked to display a tag\n> object with no name (missing \"tag\" header), we will show the tag's sha1\n> instead of an empty string.\n> \n> Finally the patch includes some tweaks to the selftests to make them \n> work with optional tag names.\n\nIf you don't actually _test_ missing tag names, you might just as well \nleave the tests alone.\n\n> -\t\t\t\t\tt->tag,\n> +\t\t\t\t\t*(t->tag) ? t->tag : name,\n\nThis is misleading. What you wanted to say is t->tag[0] == '\\0', or \n*(t->tag) == '\\0'.\n\nAs you wrote it, you have to think a couple of times why it is okay to \ndereference t->tag, to check if you say t->tag.\n\nBesides, it breaks if you _do_ have an empty tag. In that case, I _want_ \nto see that it is actually empty, and _not_ the SHA1 substituted for it.\n\nCiao,\nDscho\n"},{"id":"44580","messageId":"Pine.LNX.4.64.0706101103160.4059@racer.site","threadId":"8491","inReplyTo":"7vwsyc8bt3.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T10:08:31Z","receivedAt":"2007-06-10T10:08:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 10 Jun 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > ... Guess how surprised \n> > _I_ was, when I hit the error message which made me go mad.\n> \n> To be fair, that ugly \"char%d\" was taken from mktag and not\n> Johan's invention.\n\nYes, I should have said that. I tried to hint to this by \"you could just \nas well clean the code up\", meaning the existing code.\n\nNow, _that_ would be a patch I'd be really thankful for.\n\nAs for the general direction of implementing notes as tags: If you want to \nmake them fetchable, you have to deal with conflicts. If you want to be \nable to amend notes, _especially_ when they should be fetchable, you want \na history on them.\n\nWhich makes me think that tags are not the right object type for notes.\n\nBut I guess I'll just wait if somebody actually comments on my RFC for \nlightweight commit annotations (that's what I put into that discussion). \nBTW I just realized that I marked it [PATCH], while it should have been \n[RFC]. Sorry.\n\nCiao,\nDscho\n"},{"id":"44585","messageId":"200706101347.57023.johan@herland.net","threadId":"8491","inReplyTo":"7vwsyc8bt3.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH 0/4] Restructure the tag object","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T11:47:56Z","receivedAt":"2007-06-10T11:47:56Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Ok, I'm pulling the 21-part patch series from hell. It's just not worth all\nthe flak. Here's a 4-part patch series that tries to do the changes needed\nwithout all the crap^Wrefactoring.\n\nObviously this patch series does none of the much needed cleanup in this\npart of the code (e.g. better error messages, specifying encodings of header\nfields, possibly unifying the common parts between the parser and the\nverifier). I'll leave that cleanup to someone who writes less crappy code.\n\nHere's the shortlog for the series:\n\nJohan Herland (4):\n Make tag names (i.e. the tag object's \"tag\" line) optional\n Introduce optional \"keywords\" on tag objects\n Documentation/git-mktag: Document the changes in tag object structure\n git-mktag tests: Expand on mktag selftests according to the new tag object structure\n\n Documentation/git-mktag.txt | 38 +++++++---\n mktag.c | 65 +++++++++++-----\n t/t3800-mktag.sh | 172 ++++++++++++++++++++++++++++++++++++++++---\n tag.c | 44 +++++++++--\n tag.h | 3 +-\n 5 files changed, 270 insertions(+), 52 deletions(-)\n\n\n...Johan\n"},{"id":"44586","messageId":"200706101349.33280.johan@herland.net","threadId":"8491","inReplyTo":"200706101347.57023.johan@herland.net","subject":"[PATCH 1/4] Make tag names (i.e. the tag object's \"tag\" line) optional","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T11:49:33Z","receivedAt":"2007-06-10T11:49:33Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The tag line is now optional. If not given in the tag object data, it\ndefaults to the empty string (\"\") in the parsed tag object.\n\nAlso includes selftest tweaks to make them work with optional tag names.\n\nSigned-off-by: Johan Herland \n---\n mktag.c | 37 ++++++++++++++++++++-----------------\n t/t3800-mktag.sh | 16 +++++++++-------\n tag.c | 18 ++++++++++++------\n tag.h | 2 +-\n 4 files changed, 42 insertions(+), 31 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 070bc96..5e80d3d 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -2,16 +2,15 @@\n #include \"tag.h\"\n \n /*\n- * A signature file has a very simple fixed format: four lines\n- * of \"object \" + \"type \" + \"tag \" +\n+ * A signature file has a very simple format: 3-4 lines\n+ * of \"object \" + \"type \" + \"tag \" (optional) +\n * \"tagger \", followed by a blank line, a free-form tag\n * message and a signature block that git itself doesn't care about,\n * but that can be verified with gpg or similar.\n *\n- * The first three lines are guaranteed to be at least 63 bytes:\n- * \"object \\n\" is 48 bytes, \"type tag\\n\" at 9 bytes is the\n- * shortest possible type-line, and \"tag .\\n\" at 6 bytes is the\n- * shortest single-character-tag line.\n+ * The first two lines are guaranteed to be at least 57 bytes:\n+ * \"object \\n\" is 48 bytes and \"type tag\\n\" at 9 bytes is the\n+ * shortest possible type-line.\n *\n * We also artificially limit the size of the full object to 8kB.\n * Just because I'm a lazy bastard, and if you can't fit a signature\n@@ -52,7 +51,7 @@ static int verify_tag(char *buffer, unsigned long size)\n \tunsigned char sha1[20];\n \tconst char *object, *type_line, *tag_line, *tagger_line;\n \n-\tif (size < 64)\n+\tif (size < 58)\n \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n \n \tbuffer[size] = 0;\n@@ -75,8 +74,6 @@ static int verify_tag(char *buffer, unsigned long size)\n \tif (!tag_line)\n \t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - buffer);\n \ttag_line++;\n-\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n-\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - buffer);\n \n \t/* Get the actual type */\n \ttypelen = tag_line - type_line - strlen(\"type \\n\");\n@@ -91,14 +88,20 @@ static int verify_tag(char *buffer, unsigned long size)\n \t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n \n \t/* Verify the tag-name: we don't allow control characters or spaces in it */\n-\ttag_line += 4;\n-\tfor (;;) {\n-\t\tunsigned char c = *tag_line++;\n-\t\tif (c == '\\n')\n-\t\t\tbreak;\n-\t\tif (c > ' ')\n-\t\t\tcontinue;\n-\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - buffer);\n+\tif (!memcmp(tag_line, \"tag \", 4)) {\n+\t\tif (tag_line[4] == '\\n')\n+\t\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\",\n+\t\t\t\t\ttag_line - buffer);\n+\t\ttag_line += 4;\n+\t\tfor (;;) {\n+\t\t\tunsigned char c = *tag_line++;\n+\t\t\tif (c == '\\n')\n+\t\t\t\tbreak;\n+\t\t\tif (c > ' ')\n+\t\t\t\tcontinue;\n+\t\t\treturn error(\"char\" PD_FMT \": could not verify tag name\",\n+\t\t\t\t\ttag_line - buffer);\n+\t\t}\n \t}\n \n \t/* Verify the tagger line */\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex 7c7e433..ca90662 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -100,13 +100,14 @@ check_verify_failure '\"type\" line eol check'\n # 6. tag line label check #1\n \n cat >tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <object.parsed = 1;\n \n-\tif (size < 64)\n+\tif (size < 58)\n \t\treturn -1;\n \tif (memcmp(\"object \", data, 7) || get_sha1_hex((char *) data + 7, sha1))\n \t\treturn -1;\n@@ -54,13 +54,17 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \t\treturn -1;\n \n \ttag_line = strchr(type_line, '\\n');\n-\tif (!tag_line || memcmp(\"tag \", ++tag_line, 4))\n+\tif (!tag_line)\n \t\treturn -1;\n \n-\tsig_line = strchr(tag_line, '\\n');\n-\tif (!sig_line)\n-\t\treturn -1;\n-\tsig_line++;\n+\tif (!memcmp(\"tag \", ++tag_line, 4)) {\n+\t\tsig_line = strchr(tag_line, '\\n');\n+\t\tif (!sig_line)\n+\t\t\treturn -1;\n+\t\tsig_line++;\n+\t}\n+\telse\n+\t\tsig_line = tag_line;\n \n \ttypelen = tag_line - type_line - strlen(\"type \\n\");\n \tif (typelen >= 20)\n@@ -68,6 +72,8 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \tmemcpy(type, type_line + 5, typelen);\n \ttype[typelen] = '\\0';\n \ttaglen = sig_line - tag_line - strlen(\"tag \\n\");\n+\tif (taglen < 0) /* missing tag name */\n+\t\ttaglen = 0;\n \titem->tag = xmalloc(taglen + 1);\n \tmemcpy(item->tag, tag_line + 4, taglen);\n \titem->tag[taglen] = '\\0';\ndiff --git a/tag.h b/tag.h\nindex 7a0cb00..7e0abbe 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -8,7 +8,7 @@ extern const char *tag_type;\n struct tag {\n \tstruct object object;\n \tstruct object *tagged;\n-\tchar *tag;\n+\tchar *tag; /* optional, may be empty (\"\") */\n \tchar *signature; /* not actually implemented */\n };\n \n-- \n1.5.2.1.144.gabc40\n"},{"id":"44587","messageId":"200706101350.00271.johan@herland.net","threadId":"8491","inReplyTo":"200706101347.57023.johan@herland.net","subject":"[PATCH 2/4] Introduce optional \"keywords\" on tag objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T11:50:00Z","receivedAt":"2007-06-10T11:50:00Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"This patch introduces a new optional header line to the tag object, called\n\"keywords\". The \"keywords\" line may contain a comma-separated list of\ncustom keywords associated with the tag object. There are two \"special\"\nkeywords, however: \"tag\" and \"note\": When the \"keywords\" header is\nmissing, its default value is set to \"tag\" if a \"tag\" header is\npresent; else the default \"keywords\" value is set to \"note\". The\n\"keywords\" header is meant to be used by porcelains for classifying\ndifferent types of tag objects. This classification may then be used to\nfilter tag objects in the presentation layer (e.g. by implementing\nextra filter options to --decorate, etc.).\n\nThe encoding rules for keywords are identical to those of tag names.\n\nSigned-off-by: Johan Herland \n---\n mktag.c | 30 ++++++++++++++++++++++++++----\n tag.c | 30 +++++++++++++++++++++++++-----\n tag.h | 1 +\n 3 files changed, 52 insertions(+), 9 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 5e80d3d..37e10c6 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -2,9 +2,10 @@\n #include \"tag.h\"\n \n /*\n- * A signature file has a very simple format: 3-4 lines\n+ * A signature file has a very simple format: 3-5 lines\n * of \"object \" + \"type \" + \"tag \" (optional) +\n- * \"tagger \", followed by a blank line, a free-form tag\n+ * \"keywords \" (optional) + \"tagger \",\n+ * followed by a blank line, a free-form tag\n * message and a signature block that git itself doesn't care about,\n * but that can be verified with gpg or similar.\n *\n@@ -49,7 +50,7 @@ static int verify_tag(char *buffer, unsigned long size)\n \tint typelen;\n \tchar type[20];\n \tunsigned char sha1[20];\n-\tconst char *object, *type_line, *tag_line, *tagger_line;\n+\tconst char *object, *type_line, *tag_line, *keywords_line, *tagger_line;\n \n \tif (size < 58)\n \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n@@ -104,8 +105,29 @@ static int verify_tag(char *buffer, unsigned long size)\n \t\t}\n \t}\n \n+\t/* Verify the keywords: disallow ctrl chars, spaces and double commas */\n+\tkeywords_line = tag_line;\n+\n+\tif (!memcmp(tag_line, \"keywords \", 9)) {\n+\t\tif (tag_line[9] == '\\n')\n+\t\t\treturn error(\"char\" PD_FMT \": no \\\"keywords \\\" found\",\n+\t\t\t\t\tkeywords_line - buffer);\n+\t\tkeywords_line += 9;\n+\t\tfor (;;) {\n+\t\t\tunsigned char c = *keywords_line++;\n+\t\t\tif (c == '\\n')\n+\t\t\t\tbreak;\n+\t\t\tif (c == ',' && *keywords_line == ',')\n+\t\t\t\t/* double commas. fall through to error() */;\n+\t\t\telse if (c > ' ')\n+\t\t\t\tcontinue;\n+\t\t\treturn error(\"char\" PD_FMT \": could not verify keywords\",\n+\t\t\t\t\tkeywords_line - buffer);\n+\t\t}\n+\t}\n+\n \t/* Verify the tagger line */\n-\ttagger_line = tag_line;\n+\ttagger_line = keywords_line;\n \n \tif (memcmp(tagger_line, \"tagger\", 6) || (tagger_line[6] == '\\n'))\n \t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger\\\"\", tagger_line - buffer);\ndiff --git a/tag.c b/tag.c\nindex a7a3454..b74f09f 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -35,9 +35,9 @@ struct tag *lookup_tag(const unsigned char *sha1)\n \n int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n {\n-\tint typelen, taglen;\n+\tint typelen, taglen, keywordslen;\n \tunsigned char sha1[20];\n-\tconst char *type_line, *tag_line, *sig_line;\n+\tconst char *type_line, *tag_line, *keywords_line, *sig_line;\n \tchar type[20];\n \n if (item->object.parsed)\n@@ -58,25 +58,45 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \t\treturn -1;\n \n \tif (!memcmp(\"tag \", ++tag_line, 4)) {\n-\t\tsig_line = strchr(tag_line, '\\n');\n+\t\tkeywords_line = strchr(tag_line, '\\n');\n+\t\tif (!keywords_line)\n+\t\t\treturn -1;\n+\t\tkeywords_line++;\n+\t}\n+\telse\n+\t\tkeywords_line = tag_line;\n+\n+\tif (!memcmp(\"keywords \", keywords_line, 9)) {\n+\t\tsig_line = strchr(keywords_line, '\\n');\n \t\tif (!sig_line)\n \t\t\treturn -1;\n \t\tsig_line++;\n \t}\n \telse\n-\t\tsig_line = tag_line;\n+\t\tsig_line = keywords_line;\n \n \ttypelen = tag_line - type_line - strlen(\"type \\n\");\n \tif (typelen >= 20)\n \t\treturn -1;\n \tmemcpy(type, type_line + 5, typelen);\n \ttype[typelen] = '\\0';\n-\ttaglen = sig_line - tag_line - strlen(\"tag \\n\");\n+\ttaglen = keywords_line - tag_line - strlen(\"tag \\n\");\n \tif (taglen < 0) /* missing tag name */\n \t\ttaglen = 0;\n \titem->tag = xmalloc(taglen + 1);\n \tmemcpy(item->tag, tag_line + 4, taglen);\n \titem->tag[taglen] = '\\0';\n+\tkeywordslen = sig_line - keywords_line - strlen(\"keywords \\n\");\n+\tif (keywordslen > 0)\n+\t\tkeywords_line += strlen(\"keywords \");\n+\telse { /* missing keywords */\n+\t\tif (taglen) /* tag name given */\n+\t\t\tkeywords_line = \"tag\";\n+\t\telse\n+\t\t\tkeywords_line = \"note\";\n+\t\tkeywordslen = strlen(keywords_line);\n+\t}\n+\titem->keywords = xstrndup(keywords_line, keywordslen);\n \n \tif (!strcmp(type, blob_type)) {\n \t\titem->tagged = &lookup_blob(sha1)->object;\ndiff --git a/tag.h b/tag.h\nindex 7e0abbe..6e687b2 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -9,6 +9,7 @@ struct tag {\n \tstruct object object;\n \tstruct object *tagged;\n \tchar *tag; /* optional, may be empty (\"\") */\n+\tchar *keywords; /* optional, defaults to (tag ? \"tag\" : \"note\") */\n \tchar *signature; /* not actually implemented */\n };\n \n-- \n1.5.2.1.144.gabc40\n"},{"id":"44588","messageId":"200706101350.27936.johan@herland.net","threadId":"8491","inReplyTo":"200706101347.57023.johan@herland.net","subject":"[PATCH 3/4] Documentation/git-mktag: Document the changes in tag object structure","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T11:50:27Z","receivedAt":"2007-06-10T11:50:27Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The new structure of tag objects is documented.\n\nAlso some much-needed cleanup is done. E.g. remove the paragraph on the\n8kB limit, since this limit was removed ages ago.\n\nSigned-off-by: Johan Herland \n---\n Documentation/git-mktag.txt | 38 +++++++++++++++++++++++++++-----------\n 1 files changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-mktag.txt b/Documentation/git-mktag.txt\nindex 0ac3be1..e6dfed6 100644\n--- a/Documentation/git-mktag.txt\n+++ b/Documentation/git-mktag.txt\n@@ -8,29 +8,44 @@ git-mktag - Creates a tag object\n \n SYNOPSIS\n --------\n-'git-mktag' < signature_file\n+[verse]\n+'git-mktag' < tag_data_file\n \n DESCRIPTION\n -----------\n-Reads a tag contents on standard input and creates a tag object\n+Reads tag object data on standard input and creates a tag object\n that can also be used to sign other objects.\n \n The output is the new tag's identifier.\n \n-Tag Format\n+DISCUSSION\n ----------\n-A tag signature file has a very simple fixed format: three lines of\n+Tag object data has the following format\n \n+[verse]\n object \n type \n- tag \n+ tag (optional)\n+ keywords (optional)\n+ tagger \n \n-followed by some 'optional' free-form signature that git itself\n-doesn't care about, but that can be verified with gpg or similar.\n+followed by a blank line and a free-form message and an optional\n+signature that git itself doesn't care about, but that may be\n+verified with gpg or similar.\n \n-The size of the full object is artificially limited to 8kB. (Just\n-because I'm a lazy bastard, and if you can't fit a signature in that\n-size, you're doing something wrong)\n+In the above listing, `` represents the object pointed to\n+by this tag, `` is the type of the object pointed to\n+(\"tag\", \"blob\", \"tree\" or \"commit\"), `` is the name of\n+this tag object (and must correspond to the name of the corresponding\n+ref (if any) in `.git/refs/`). `` is a comma-separated\n+list of keywords associated with this tag object, and ``\n+holds the \"`name `\" of the tag creator and timestamp of when\n+the tag object was created (analogous to \"committer\" in commit\n+objects).\n+\n+If \"`tag `\" is omitted, defaults to the empty\n+string. If \"`keywords `\" is omitted, defaults\n+to \"`tag`\" if a was given, \"`note`\" otherwise.\n \n \n Author\n@@ -39,7 +54,8 @@ Written by Linus Torvalds \n \n Documentation\n --------------\n-Documentation by David Greaves, Junio C Hamano and the git-list .\n+Documentation by Johan Herland, David Greaves, Junio C Hamano\n+and the git-list .\n \n GIT\n ---\n-- \n1.5.2.1.144.gabc40\n"},{"id":"44589","messageId":"200706101350.55062.johan@herland.net","threadId":"8491","inReplyTo":"200706101347.57023.johan@herland.net","subject":"[PATCH 4/4] git-mktag tests: Expand on mktag selftests according to the new tag object structure","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T11:50:54Z","receivedAt":"2007-06-10T11:50:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Some more tests are added to test the new \"keywords\" header.\n\nSigned-off-by: Johan Herland \n---\n t/t3800-mktag.sh | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 152 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex ca90662..cc2f246 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -174,7 +174,95 @@ EOF\n check_verify_failure 'verify tag-name check'\n \n ############################################################\n-# 11. tagger line label check #1\n+# 11. keywords line label check #1\n+\n+cat >tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <expect.pat <tag.sig <tag.sig <tag.sig <.git/refs/tags/mytag 2>message'\n \n ############################################################\n-# 14. check mytag\n+# 19. check mytag\n \n test_expect_success \\\n 'check mytag' \\\n 'git-tag -l | grep mytag'\n \n+############################################################\n+# 20. create valid tag #2\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n+############################################################\n+# 21. create valid tag #3\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n+############################################################\n+# 22. create valid tag #4\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n+############################################################\n+# 23. create valid tag #5 (with empty message)\n+\n+cat >tag.sig <.git/refs/tags/mytag 2>message'\n+\n \n test_done\n-- \n1.5.2.1.144.gabc40\n"},{"id":"44590","messageId":"200706101410.17771.johan@herland.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706101103160.4059@racer.site","subject":"Re: [PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T12:10:17Z","receivedAt":"2007-06-10T12:10:17Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sunday 10 June 2007, Johannes Schindelin wrote:\n> As for the general direction of implementing notes as tags: If you want to \n> make them fetchable, you have to deal with conflicts. If you want to be \n> able to amend notes, _especially_ when they should be fetchable, you want \n> a history on them.\n\nI'm not sure what kind of notes you're talking about here. If you're talking \nabout my git-note concept, I designed notes to be immutable (thus not \namendable) and there is therefore _no_ merging or potential for conflicts \nbetween notes. The only resolution needed is to figure out which order the \nnotes for a given object should be presented. The default here is \nchronological sorting.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44626","messageId":"Pine.LNX.4.64.0706101934550.4059@racer.site","threadId":"8491","inReplyTo":"200706101347.57023.johan@herland.net","subject":"Re: [PATCH 0/4] Restructure the tag object","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T18:35:15Z","receivedAt":"2007-06-10T18:35:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 10 Jun 2007, Johan Herland wrote:\n\n> Johan Herland (4):\n> Make tag names (i.e. the tag object's \"tag\" line) optional\n> Introduce optional \"keywords\" on tag objects\n> Documentation/git-mktag: Document the changes in tag object structure\n> git-mktag tests: Expand on mktag selftests according to the new tag object structure\n\nMuch nicer, thank you.\n\nCiao,\nDscho\n"},{"id":"44627","messageId":"Pine.LNX.4.64.0706101937250.4059@racer.site","threadId":"8491","inReplyTo":"200706101350.00271.johan@herland.net","subject":"Re: [PATCH 2/4] Introduce optional \"keywords\" on tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T18:42:41Z","receivedAt":"2007-06-10T18:42:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 10 Jun 2007, Johan Herland wrote:\n\n> +\t/* Verify the keywords: disallow ctrl chars, spaces and double commas */\n\nWhat about Junio's suggestion, making it really strict at first, and only \nloosening it if we need to? IIRC it was alnum + '_', maybe even '-'.\n\nOther than that, looks good to me. I trust that the test cases are \nexhaustive enough to support the patch from the practical side.\n\nBTW this patch is exactly what I meant by conceptually closed. Thank you.\n\nAnd please accept my apologies for my language. Reading some of it, I have \nto admit that it sounded as harsh as Junio suggested it to be. My only \nexcuse is that I had an unplanned stay at the Paris airport for more than \n9 hours (after a night in the plane where I could hardly sleep), so I \nshould really have stayed away from writing emails. But since you \naddressed your emails to me, I wanted to reply to you as soon as I had the \nchance to.\n\nCiao,\nDscho\n"},{"id":"44628","messageId":"Pine.LNX.4.64.0706101943350.4059@racer.site","threadId":"8491","inReplyTo":"200706101410.17771.johan@herland.net","subject":"Re: [PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-10T18:51:22Z","receivedAt":"2007-06-10T18:51:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 10 Jun 2007, Johan Herland wrote:\n\n> On Sunday 10 June 2007, Johannes Schindelin wrote:\n>\n> > As for the general direction of implementing notes as tags: If you \n> > want to make them fetchable, you have to deal with conflicts. If you \n> > want to be able to amend notes, _especially_ when they should be \n> > fetchable, you want a history on them.\n> \n> I'm not sure what kind of notes you're talking about here. If you're \n> talking about my git-note concept, I designed notes to be immutable \n> (thus not amendable) and there is therefore _no_ merging or potential \n> for conflicts between notes.\n\nOkay, that is one way you can go about implementing notes.\n\n> The only resolution needed is to figure out which order the notes for a \n> given object should be presented. The default here is chronological \n> sorting.\n\nThere are several problems with that approach I'd like to point out:\n\n- In distributed environments, you can not rely on timestamps. Ever.\n\n- If a note is deleted, you will fetch it again as long as the other side \n did not delete it.\n\n- You cannot undo a typo (since the notes are immutable, you would see \n both versions), once the typoed note was fetched.\n\nBasically, everything I see as a problem here suggests that note writing \nis very much like working on a branch. That's why I suggest to treat it \nexactly like a branch to begin with.\n\nCiao,\nDscho\n"},{"id":"44630","messageId":"200706102104.48440.johan@herland.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706101937250.4059@racer.site","subject":"Re: [PATCH 2/4] Introduce optional \"keywords\" on tag objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T19:04:48Z","receivedAt":"2007-06-10T19:04:48Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sunday 10 June 2007, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sun, 10 Jun 2007, Johan Herland wrote:\n> \n> > +\t/* Verify the keywords: disallow ctrl chars, spaces and double commas */\n> \n> What about Junio's suggestion, making it really strict at first, and only \n> loosening it if we need to? IIRC it was alnum + '_', maybe even '-'.\n\nFor now, I couldn't find a good reason why the set of allowed characters\nfor keywords should be smaller than for the tag name.\nFeel free to tighten the set of characters before this makes it into a\nrelease. However, if you do, the same tightening should be considered\nfor the tag name as well, I guess. Can't see any good reasons for why\none should be tighter than the other.\n\n> And please accept my apologies for my language. Reading some of it, I have \n> to admit that it sounded as harsh as Junio suggested it to be. My only \n> excuse is that I had an unplanned stay at the Paris airport for more than \n> 9 hours (after a night in the plane where I could hardly sleep), so I \n> should really have stayed away from writing emails. But since you \n> addressed your emails to me, I wanted to reply to you as soon as I had the \n> chance to.\n\nApology accepted. I'm sorry my patch-series-from-hell came at such an\ninconvenient time for you. \n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44631","messageId":"200706102116.19414.johan@herland.net","threadId":"8491","inReplyTo":"Pine.LNX.4.64.0706101943350.4059@racer.site","subject":"Re: [PATCH] Silence error messages unless 'thorough_verify' is set","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T19:16:19Z","receivedAt":"2007-06-10T19:16:19Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sunday 10 June 2007, Johannes Schindelin wrote:\n> On Sun, 10 Jun 2007, Johan Herland wrote:\n> > On Sunday 10 June 2007, Johannes Schindelin wrote:\n> > > As for the general direction of implementing notes as tags: If you \n> > > want to make them fetchable, you have to deal with conflicts. If you \n> > > want to be able to amend notes, _especially_ when they should be \n> > > fetchable, you want a history on them.\n> > \n> > I'm not sure what kind of notes you're talking about here. If you're \n> > talking about my git-note concept, I designed notes to be immutable \n> > (thus not amendable) and there is therefore _no_ merging or potential \n> > for conflicts between notes.\n> \n> Okay, that is one way you can go about implementing notes.\n> \n> > The only resolution needed is to figure out which order the notes for a \n> > given object should be presented. The default here is chronological \n> > sorting.\n> \n> There are several problems with that approach I'd like to point out:\n> \n> - In distributed environments, you can not rely on timestamps. Ever.\n\nNot really, but that doesn't stop many programs from trying anyway...\n(e.g. email clients). And still, it's not like the date (or sorting)\nis crucial to the 'notes' concept or implementation.\n\n> - If a note is deleted, you will fetch it again as long as the other side \n> did not delete it.\n\nYep. This was considered an acceptable tradeoff in the design. But I\nunderstand that some people won't like it.\n\n> - You cannot undo a typo (since the notes are immutable, you would see \n> both versions), once the typoed note was fetched.\n\nYep. Also a tradeoff in the design. Also going to piss off some\npeople, I guess.\n\n> Basically, everything I see as a problem here suggests that note writing \n> is very much like working on a branch. That's why I suggest to treat it \n> exactly like a branch to begin with.\n\nI see you point.\n\n\nBTW, I have some patches implementing the 'notes' concept on top\nof the softrefs patches. They're just lying around now waiting\nto be cleaned up and sent to the list, but I'm not sure it's worth\nit, since they don't add anything that's not in your lightweight\nannotation patch...\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44648","messageId":"7vabv77ael.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706102104.48440.johan@herland.net","subject":"Re: [PATCH 2/4] Introduce optional \"keywords\" on tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-10T21:43:46Z","receivedAt":"2007-06-10T21:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> For now, I couldn't find a good reason why the set of allowed characters\n> for keywords should be smaller than for the tag name.\n\nThe set of allowed tag names excludes shell metacharacters,\nprimarily to help scripting. I think keywords can share the\nsame reasoning to exclude them.\n\nIt also excludes '^', '~' and ':', because tag names can be used\nin revision range expressions (i.e. prefix '^' is the \"exclude\nfrom the resulting set\" operation, postfix \"~\" is the\n\"Nth generation ancestor\" operation) and general SHA-1\nexpression (i.e. infix ':' is the \"find in the tree-ish the\nobject at path\" operation). These reasons would not apply to \nkeywords.\n\nHaving said all of that, I suspect it is premature to talk about\nkeywords, as it is unclear what their intended use is. What\nkind of operations are useful on them? \n\nIt does not count that \"git cat-file tag\" would show \"keywords\nblah\" on the header instead of in body. It is not a compelling\nenough reason to introduce a new header type. grep would work\njust fine for such a use.\n\nOn the other hand, for example, if (the syntax is totally made\nup) we make '::keywords=foo::' expand to set of all tags that\nhave the specified keyword 'foo', and it turns out to be useful\nto be able to say \"git show ::keywords=foo::\" instead of listing\nindividual tags, that kind of use case may make it a good reason\nto add such a new header type.\n"},{"id":"44656","messageId":"7vps435sy0.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706101349.33280.johan@herland.net","subject":"Re: [PATCH 1/4] Make tag names (i.e. the tag object's \"tag\" line) optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-10T22:46:15Z","receivedAt":"2007-06-10T22:46:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> The tag line is now optional. If not given in the tag object data, it\n> defaults to the empty string (\"\") in the parsed tag object.\n\nSorry, I may have missed the discussion. I recall that we\ntalked about tagger line which may not exist for historical\nreasons, but have we heard any reasoning behind this?\n"},{"id":"44658","messageId":"200706110101.08664.johan@herland.net","threadId":"8491","inReplyTo":"7vps435sy0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/4] Make tag names (i.e. the tag object's \"tag\" line) optional","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T23:01:08Z","receivedAt":"2007-06-10T23:01:08Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 11 June 2007, Junio C Hamano wrote:\n> Johan Herland writes:\n> \n> > The tag line is now optional. If not given in the tag object data, it\n> > defaults to the empty string (\"\") in the parsed tag object.\n> \n> Sorry, I may have missed the discussion. I recall that we\n> talked about tagger line which may not exist for historical\n> reasons, but have we heard any reasoning behind this?\n\nIn the discussion that followed the first proof-of-concept of 'notes',\nI argued that the \"tag\" header is _really_ only needed for\nsigned tag objects (although we want it for annotated tags as well),\nand that it doesn't make sense to specify a tag name for unnamed 'note'\nobjects.\n\nIf the \"tag\" line remains mandatory, I'll have to construct an\nartificial name for unnamed 'notes'...\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44661","messageId":"200706110116.15673.johan@herland.net","threadId":"8491","inReplyTo":"7vabv77ael.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/4] Introduce optional \"keywords\" on tag objects","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-06-10T23:16:15Z","receivedAt":"2007-06-10T23:16:15Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sunday 10 June 2007, Junio C Hamano wrote:\n> Johan Herland writes:\n> > For now, I couldn't find a good reason why the set of allowed characters\n> > for keywords should be smaller than for the tag name.\n> \n> The set of allowed tag names excludes shell metacharacters,\n> primarily to help scripting. \n\nIt already does? Or are you proposing this? Right now the code doesn't \nenforce anything like this, AFAICS...\n\n> I think keywords can share the same reasoning to exclude them.\n> \n> It also excludes '^', '~' and ':', because tag names can be used \n> in revision range expressions (i.e. prefix '^' is the \"exclude\n> from the resulting set\" operation, postfix \"~\" is the\n> \"Nth generation ancestor\" operation) and general SHA-1\n> expression (i.e. infix ':' is the \"find in the tree-ish the\n> object at path\" operation). These reasons would not apply to \n> keywords.\n\nI have nothing against limiting keywords to fairly small set, say\nalphanumerics plus a couple of \"safe\" symbols. It just didn't make\nsense to do this when I made the patch without doing it to the\ntag name at the same time, and I'm not sure what that restricted\nset should be, so I held off on it. Feel free to fix.\n\n> Having said all of that, I suspect it is premature to talk about\n> keywords, as it is unclear what their intended use is. What\n> kind of operations are useful on them? \n> \n> It does not count that \"git cat-file tag\" would show \"keywords\n> blah\" on the header instead of in body. It is not a compelling\n> enough reason to introduce a new header type. grep would work\n> just fine for such a use.\n> \n> On the other hand, for example, if (the syntax is totally made\n> up) we make '::keywords=foo::' expand to set of all tags that\n> have the specified keyword 'foo', and it turns out to be useful\n> to be able to say \"git show ::keywords=foo::\" instead of listing\n> individual tags, that kind of use case may make it a good reason\n> to add such a new header type.\n\nYes, this is what I'm thinking; using keywords to filter tag objects in \nvarious settings. Haven't thought much about the syntax yet, but as it \nwould have to work on the command-line (possibly together with the other \nweird characters git uses for specifying revisions), I imagine a character \nset similar to the one for tag names should be good.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"44680","messageId":"7vejkj4840.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706110116.15673.johan@herland.net","subject":"Re: [PATCH 2/4] Introduce optional \"keywords\" on tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-11T01:01:35Z","receivedAt":"2007-06-11T01:01:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Sunday 10 June 2007, Junio C Hamano wrote:\n>> Johan Herland writes:\n>> > For now, I couldn't find a good reason why the set of allowed characters\n>> > for keywords should be smaller than for the tag name.\n>> \n>> The set of allowed tag names excludes shell metacharacters,\n>> primarily to help scripting. \n>\n> It already does? Or are you proposing this? Right now the code doesn't \n> enforce anything like this, AFAICS...\n\nAh, I was being silly. I somehow thought you were talking about\nvalid names you can have under refs/tags/.\n"},{"id":"44682","messageId":"7vy7ir2t3t.fsf@assigned-by-dhcp.cox.net","threadId":"8491","inReplyTo":"200706110101.08664.johan@herland.net","subject":"Re: [PATCH 1/4] Make tag names (i.e. the tag object's \"tag\" line) optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-11T01:11:02Z","receivedAt":"2007-06-11T01:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> If the \"tag\" line remains mandatory, I'll have to construct an\n> artificial name for unnamed 'notes'...\n\nAh, silly me, now I remember.\n\nBut let's put this series on hold and see how versioned set of\nnotes from Johannes turns out. It seems that people are in\nfavor of its simplicity, and even though it may be much less\nexpressive, it might be sufficient for certain class of\napplications. It one certainly is a much smaller and isolated\nchange that we could easily drop more easily if it does not work\nout.\n"}]}