{"thread":{"id":"11860","subject":"[PATCH 1/3] git-fsck: report missing author/commit line in a commit as an error","startedAt":"2008-02-03T21:22:37Z","lastAt":"2008-02-04T07:33:06Z","messageCount":7,"participants":["Martin Koegler","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"67260","messageId":"12020737593747-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11860","inReplyTo":null,"subject":"[PATCH 1/3] git-fsck: report missing author/commit line in a commit as an error","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-03T21:22:37Z","receivedAt":"2008-02-03T21:22:37Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"A zero commit date could be caused by:\n* a missing author line\n* a missing commiter line\n* a malformed email address in the commiter line\n* a malformed commit date\n\nSimply reporting it as zero commit date is missleading.\n\nAdditionally, it upgrades the message to an error (instead of an printf).\n\nSigned-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n builtin-fsck.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 6fc9525..cc7524b 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -360,6 +360,9 @@ static int fsck_commit(struct commit *commit)\n \t\tfprintf(stderr, \"Checking commit %s\\n\",\n \t\t\tsha1_to_hex(commit->object.sha1));\n \n+\tif (!commit->date)\n+\t\treturn objerror(&commit->object, \"invalid author/committer line\");\n+\n \tif (memcmp(buffer, \"tree \", 5))\n \t\treturn objerror(&commit->object, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n@@ -378,9 +381,6 @@ static int fsck_commit(struct commit *commit)\n \t\treturn objerror(&commit->object, \"could not load commit's tree %s\", tree_sha1);\n \tif (!commit->parents && show_root)\n \t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\n-\tif (!commit->date)\n-\t\tprintf(\"bad commit date in %s\\n\",\n-\t\t       sha1_to_hex(commit->object.sha1));\n \treturn 0;\n }\n \n-- \n1.5.4.g22bc\n"},{"id":"67261","messageId":"12020737601683-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11860","inReplyTo":"12020737593747-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH 2/3] fsck_commit: remove duplicate tests","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-03T21:22:38Z","receivedAt":"2008-02-03T21:22:38Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"All tests on the commit buffer in fsck_cmd are ready done by\nparse_commit_buffer.\n\nThis patch rips out all redundant tests. It still leaves the check for\nauthor, as this is can be used as starting point for validation the\nauthor/committer information.\n\nSigned-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n builtin-fsck.c |   13 ++++---------\n 1 files changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex cc7524b..ba785ec 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -354,7 +354,7 @@ static int fsck_tree(struct tree *item)\n static int fsck_commit(struct commit *commit)\n {\n \tchar *buffer = commit->buffer;\n-\tunsigned char tree_sha1[20], sha1[20];\n+\tunsigned char tree_sha1[20];\n \n \tif (verbose)\n \t\tfprintf(stderr, \"Checking commit %s\\n\",\n@@ -363,22 +363,17 @@ static int fsck_commit(struct commit *commit)\n \tif (!commit->date)\n \t\treturn objerror(&commit->object, \"invalid author/committer line\");\n \n-\tif (memcmp(buffer, \"tree \", 5))\n-\t\treturn objerror(&commit->object, \"invalid format - expected 'tree' line\");\n-\tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n+\tif (get_sha1_hex(buffer+5, tree_sha1))\n \t\treturn objerror(&commit->object, \"invalid 'tree' line format - bad sha1\");\n \tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n-\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n-\t\t\treturn objerror(&commit->object, \"invalid 'parent' line format - bad sha1\");\n+\twhile (!memcmp(buffer, \"parent \", 7))\n \t\tbuffer += 48;\n-\t}\n \tif (memcmp(buffer, \"author \", 7))\n \t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\n \tfree(commit->buffer);\n \tcommit->buffer = NULL;\n \tif (!commit->tree)\n-\t\treturn objerror(&commit->object, \"could not load commit's tree %s\", tree_sha1);\n+\t\treturn objerror(&commit->object, \"could not load commit's tree %s\", sha1_to_hex(tree_sha1));\n \tif (!commit->parents && show_root)\n \t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\n \treturn 0;\n-- \n1.5.4.g22bc\n"},{"id":"67262","messageId":"12020737603543-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11860","inReplyTo":"12020737601683-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH 3/3] parse_object_buffer: don't ignore errors from the object specific parsing functions","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-03T21:22:39Z","receivedAt":"2008-02-03T21:22:39Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"In the case of an malformed object, the object specific parsing functions\nwould return an error, which is currently ignored. The object can be partial\ninitialized in this case.\n\nThis patch make parse_object_buffer propagate such errors.\n\nSigned-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n object.c |   12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 5a5ebe2..4e7f27a 100644\n--- a/object.c\n+++ b/object.c\n@@ -140,7 +140,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \tif (type == OBJ_BLOB) {\n \t\tstruct blob *blob = lookup_blob(sha1);\n \t\tif (blob) {\n-\t\t\tparse_blob_buffer(blob, buffer, size);\n+\t\t\tif (parse_blob_buffer(blob, buffer, size))\n+\t\t\t\treturn NULL;\n \t\t\tobj = &blob->object;\n \t\t}\n \t} else if (type == OBJ_TREE) {\n@@ -148,14 +149,16 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tif (tree) {\n \t\t\tobj = &tree->object;\n \t\t\tif (!tree->object.parsed) {\n-\t\t\t\tparse_tree_buffer(tree, buffer, size);\n+\t\t\t\tif (parse_tree_buffer(tree, buffer, size))\n+\t\t\t\t\treturn NULL;\n \t\t\t\teaten = 1;\n \t\t\t}\n \t\t}\n \t} else if (type == OBJ_COMMIT) {\n \t\tstruct commit *commit = lookup_commit(sha1);\n \t\tif (commit) {\n-\t\t\tparse_commit_buffer(commit, buffer, size);\n+\t\t\tif (parse_commit_buffer(commit, buffer, size))\n+\t\t\t\treturn NULL;\t\n \t\t\tif (!commit->buffer) {\n \t\t\t\tcommit->buffer = buffer;\n \t\t\t\teaten = 1;\n@@ -165,7 +168,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t} else if (type == OBJ_TAG) {\n \t\tstruct tag *tag = lookup_tag(sha1);\n \t\tif (tag) {\n-\t\t\tparse_tag_buffer(tag, buffer, size);\n+\t\t\tif (parse_tag_buffer(tag, buffer, size))\n+\t\t\t       return NULL;\n \t\t\tobj = &tag->object;\n \t\t}\n \t} else {\n-- \n1.5.4.g22bc\n"},{"id":"67282","messageId":"7v3as9r3r2.fsf@gitster.siamese.dyndns.org","threadId":"11860","inReplyTo":"12020737601683-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH 2/3] fsck_commit: remove duplicate tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-03T23:58:57Z","receivedAt":"2008-02-03T23:58:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> All tests on the commit buffer in fsck_cmd are ready done by\n> parse_commit_buffer.\n>\n> This patch rips out all redundant tests.\n\nAs I believe in belt-and-suspenders when it comes to validation,\nI am somewhat uneasy with this change.  If it ever turns out\nthat we would need to make parse_commit_buffer() more lenient\nduring the normal operation, some of this would need to be\nreverted (for example, you may not need to have a valid \"tree\"\nto salvage the log messages from a corrupt history by running\n\"git log\"), and forgetting to do so will result in fsck that\ndoes not validate enough.\n\nThe validation done by parse_commit_buffer() and fsck serve two\ndifferent purposes, and they can have different leniency\nrequirements.  I do not necessarily agree that it is a good idea\nto tie them together like this patch.\n"},{"id":"67283","messageId":"alpine.LSU.1.00.0802040003360.7372@racer.site","threadId":"11860","inReplyTo":"7v3as9r3r2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] fsck_commit: remove duplicate tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-04T00:04:31Z","receivedAt":"2008-02-04T00:04:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 3 Feb 2008, Junio C Hamano wrote:\n\n> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n> \n> > All tests on the commit buffer in fsck_cmd are ready done by \n> > parse_commit_buffer.\n> >\n> > This patch rips out all redundant tests.\n> \n> As I believe in belt-and-suspenders when it comes to validation, I am \n> somewhat uneasy with this change.\n\nBesides, should we really change fsck?  It's not _that_ much of a \nperformance-critical operation.  Accuracy is much more important.\n\nfsck is the reason I trust git with my data.\n\nCiao,\nDscho\n"},{"id":"67323","messageId":"20080204070600.GA30099@auto.tuwien.ac.at","threadId":"11860","inReplyTo":"alpine.LSU.1.00.0802040003360.7372@racer.site","subject":"Re: [PATCH 2/3] fsck_commit: remove duplicate tests","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-04T07:06:00Z","receivedAt":"2008-02-04T07:06:00Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Mon, Feb 04, 2008 at 12:04:31AM +0000, Johannes Schindelin wrote:\n> On Sun, 3 Feb 2008, Junio C Hamano wrote:\n> > Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n> > \n> > > All tests on the commit buffer in fsck_cmd are ready done by \n> > > parse_commit_buffer.\n> > >\n> > > This patch rips out all redundant tests.\n> > \n> > As I believe in belt-and-suspenders when it comes to validation, I am \n> > somewhat uneasy with this change.\n> \n> Besides, should we really change fsck?  It's not _that_ much of a \n> performance-critical operation.  Accuracy is much more important.\n> \n> fsck is the reason I trust git with my data.\n\nThen please drop this patch. I hope the other two patches are OK.\n\nmfg Martin Kögler\n"},{"id":"67328","messageId":"7vodaxkwgd.fsf@gitster.siamese.dyndns.org","threadId":"11860","inReplyTo":"20080204070600.GA30099@auto.tuwien.ac.at","subject":"Re: [PATCH 2/3] fsck_commit: remove duplicate tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-04T07:33:06Z","receivedAt":"2008-02-04T07:33:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n\n> On Mon, Feb 04, 2008 at 12:04:31AM +0000, Johannes Schindelin wrote:\n>> On Sun, 3 Feb 2008, Junio C Hamano wrote:\n>> > Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n>> > \n>> > > All tests on the commit buffer in fsck_cmd are ready done by \n>> > > parse_commit_buffer.\n>> > >\n>> > > This patch rips out all redundant tests.\n>> > \n>> > As I believe in belt-and-suspenders when it comes to validation, I am \n>> > somewhat uneasy with this change.\n>> \n>> Besides, should we really change fsck?  It's not _that_ much of a \n>> performance-critical operation.  Accuracy is much more important.\n>> \n>> fsck is the reason I trust git with my data.\n>\n> Then please drop this patch. I hope the other two patches are OK.\n\nYeah, thanks for the other two, which I've queued already.\n"}]}