{"thread":{"id":"12300","subject":"[PATCH v3] builtin-fsck: reports missing parent commits","startedAt":"2008-02-24T20:58:56Z","lastAt":"2008-02-25T07:24:55Z","messageCount":4,"participants":["Martin Koegler","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"69818","messageId":"12038867362489-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"12300","inReplyTo":null,"subject":"[PATCH v3] builtin-fsck: reports missing parent commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-24T20:58:56Z","receivedAt":"2008-02-24T20:58:56Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler \n---\n builtin-fsck.c | 24 ++++++++++++++++++++++++\n commit.c | 2 +-\n commit.h | 1 +\n 3 files changed, 26 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 512346a..fae7e22 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -394,6 +394,8 @@ static int fsck_commit(struct commit *commit)\n {\n \tchar *buffer = commit->buffer;\n \tunsigned char tree_sha1[20], sha1[20];\n+\tstruct commit_graft *graft;\n+\tint parents = 0;\n \n \tif (verbose)\n \t\tfprintf(stderr, \"Checking commit %s\\n\",\n@@ -411,6 +413,28 @@ static int fsck_commit(struct commit *commit)\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 \t\tbuffer += 48;\n+\t\tparents++;\n+\t}\n+\tgraft = lookup_commit_graft(commit->object.sha1);\n+\tif (graft) {\n+\t\tstruct commit_list *p = commit->parents;\n+\t\tparents = 0;\n+\t\twhile (p) {\n+\t\t\tp = p->next;\n+\t\t\tparents++;\n+\t\t}\n+\t\tif (graft->nr_parent == -1 && !parents)\n+\t\t\t; /* shallow commit */\n+\t\telse if (graft->nr_parent != parents)\n+\t\t\treturn objerror(&commit->object, \"graft objects missing\");\n+\t} else {\n+\t\tstruct commit_list *p = commit->parents;\n+\t\twhile (p && parents) {\n+\t\t\tp = p->next;\n+\t\t\tparents--;\n+\t\t}\n+\t\tif (p || parents)\n+\t\t\treturn objerror(&commit->object, \"parent objects missing\");\n \t}\n \tif (memcmp(buffer, \"author \", 7))\n \t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\ndiff --git a/commit.c b/commit.c\nindex 6684c4e..94d5b3d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -193,7 +193,7 @@ static void prepare_commit_graft(void)\n \tcommit_graft_prepared = 1;\n }\n \n-static struct commit_graft *lookup_commit_graft(const unsigned char *sha1)\n+struct commit_graft *lookup_commit_graft(const unsigned char *sha1)\n {\n \tint pos;\n \tprepare_commit_graft();\ndiff --git a/commit.h b/commit.h\nindex 80d65b9..a1e9591 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -116,6 +116,7 @@ struct commit_graft {\n struct commit_graft *read_graft_line(char *buf, int len);\n int register_commit_graft(struct commit_graft *, int);\n int read_graft_file(const char *graft_file);\n+struct commit_graft *lookup_commit_graft(const unsigned char *sha1);\n \n extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);\n \n-- \n1.5.4.2.g01825.dirty\n"},{"id":"69844","messageId":"7vlk59pwaj.fsf@gitster.siamese.dyndns.org","threadId":"12300","inReplyTo":"12038867362489-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH v3] builtin-fsck: reports missing parent commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T03:08:52Z","receivedAt":"2008-02-25T03:08:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler writes:\n\n> Signed-off-by: Martin Koegler \n\nAs far as I can tell, the new test is not testing the commit\nobject we are looking at from the object database. What it is\ntesting is if the code that parsed and prepared the information\nin \"struct commit\" found the same number of parents an extra\ncheck we are doing here by hand (if not grafted --- but\npresumably whoever gave the struct commit we are handling here\nwould have obtained that information by doing the same parsing),\nor the parsing of the graft file (when grafted --- but\npresumably whoever gave the struct commit we are handling here\nwould have obtained that information by calling the same\nllokup_commit_graft()).\n\nSo I am not sure what problems in the repository objects these\nnew checks are designed to catch.\n\nThis needs a lot of explanation than what's in your commit log\nmessage.\n"},{"id":"69854","messageId":"20080225071642.GA15761@auto.tuwien.ac.at","threadId":"12300","inReplyTo":"7vlk59pwaj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v3] builtin-fsck: reports missing parent commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-25T07:16:42Z","receivedAt":"2008-02-25T07:16:42Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 24, 2008 at 07:08:52PM -0800, Junio C Hamano wrote:\n> Martin Koegler writes:\n> As far as I can tell, the new test is not testing the commit\n> object we are looking at from the object database. What it is\n> testing is if the code that parsed and prepared the information\n> in \"struct commit\" found the same number of parents an extra\n> check we are doing here by hand (if not grafted --- but\n> presumably whoever gave the struct commit we are handling here\n> would have obtained that information by doing the same parsing),\n> or the parsing of the graft file (when grafted --- but\n> presumably whoever gave the struct commit we are handling here\n> would have obtained that information by calling the same\n> llokup_commit_graft()).\n> \n> So I am not sure what problems in the repository objects these\n> new checks are designed to catch.\n> \n> This needs a lot of explanation than what's in your commit log\n> message.\n\nIf we have already parsed a non-commit object and parse_commit_buffer\nhits such a sha1 in a parent line, it simply drops it (same for grafts).\n\nI hope that you agree with me, that fsck should catch such an error.\n\nAs you don't want to tighten parse_commit_buffer, it must be in fsck.\n\nmfg Martin Kögler\n"},{"id":"69856","messageId":"7v7igto5vc.fsf@gitster.siamese.dyndns.org","threadId":"12300","inReplyTo":"20080225071642.GA15761@auto.tuwien.ac.at","subject":"Re: [PATCH v3] builtin-fsck: reports missing parent commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T07:24:55Z","receivedAt":"2008-02-25T07:24:55Z","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 Sun, Feb 24, 2008 at 07:08:52PM -0800, Junio C Hamano wrote:\n> ...\n>> So I am not sure what problems in the repository objects these\n>> new checks are designed to catch.\n>> \n>> This needs a lot of explanation than what's in your commit log\n>> message.\n>\n> If we have already parsed a non-commit object and parse_commit_buffer\n> hits such a sha1 in a parent line, it simply drops it (same for grafts).\n>\n> I hope that you agree with me, that fsck should catch such an error.\n\nSee? That's exactly my point.\n\nYou would need to _explain that_ in your commit log message.\nWho on the list other than you noticed that it is what you are\nchecking?\n"}]}