{"thread":{"id":"35092","subject":"[PATCH 4/5] use parse_commit_or_die instead of custom message","startedAt":"2013-10-08T13:48:43Z","lastAt":"2013-10-08T14:00:06Z","messageCount":6,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"228659","messageId":"20131008134843.GA7530@sigill.intra.peff.net","threadId":"35092","inReplyTo":"20131006044856.GA12301@sigill.intra.peff.net","subject":"[PATCH 0/5] parse_commit cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-08T13:48:43Z","receivedAt":"2013-10-08T13:48:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 06, 2013 at 12:48:56AM -0400, Jeff King wrote:\n\n> Instead of a segfault, let's print an error message and die\n> a little more gracefully.\n> [...]\n> ---\n> Not a huge deal, since we are terminating the program either way. There\n> are other places in the code with a bare parse_commit that could\n> probably use the same treatment. I didn't investigate them, but they\n> could easily build on the parse_commit_or_die here if somebody wants to\n> follow up.\n\nHere are some follow-up patches that go on top. The first two are noop\ncleanups. The third and fourth are not strictly noops, but are minor\nbehavior changes that should be strict improvements.\n\nThere are a number of unchecked parse_commit calls whose fallout is more\ncomplicated. In many cases, we want to ignore an error (keeping in mind\nthat parse_commit will already have printed a message to stderr) and\nkeep going, in order to allow users to to get as much done as they can\nin a broken repository.\n\nThe fifth patch deals with one of these cases. There are 8 more\nunchecked calls after this series, some of which may want to die(), or\nmay want to be left alone; I didn't investigate deeply.\n\nThere are also some cases where we do not die, but perhaps should. In\ngeneral, I think it makes sense to let fetch-pack and rev-list work\naround broken history, but probably not things like blame, which would\nthen provide subtly wrong answers. Still, I doubt it's a big deal in\npractice, since corrupted repositories are relatively rare (and the\nmessage to stderr does inform the user that something is wrong).\n\n  [1/5]: assume parse_commit checks commit->object.parsed\n  [2/5]: assume parse_commit checks for NULL commit\n  [3/5]: use parse_commit_or_die instead of segfaulting\n  [4/5]: use parse_commit_or_die instead of custom message\n  [5/5]: checkout: do not die when leaving broken detached HEAD\n\n-Peff\n"},{"id":"228657","messageId":"20131008135213.GA7650@sigill.intra.peff.net","threadId":"35092","inReplyTo":"20131008134843.GA7530@sigill.intra.peff.net","subject":"[PATCH 1/5] assume parse_commit checks commit->object.parsed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-08T13:52:14Z","receivedAt":"2013-10-08T13:52:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_commit function will check the \"parsed\" flag of\nthe object and do nothing if it is set. There is no need\nfor callers to check the flag themselves, and doing so only\nclutters the code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c       | 3 +--\n builtin/name-rev.c    | 3 +--\n builtin/show-branch.c | 3 +--\n fetch-pack.c          | 8 +++-----\n 4 files changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 6da7233..5f1cb09 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1549,8 +1549,7 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\t */\n \t\torigin_incref(suspect);\n \t\tcommit = suspect->commit;\n-\t\tif (!commit->object.parsed)\n-\t\t\tparse_commit(commit);\n+\t\tparse_commit(commit);\n \t\tif (reverse ||\n \t\t    (!(commit->object.flags & UNINTERESTING) &&\n \t\t     !(revs->max_age != -1 && commit->date < revs->max_age)))\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 20fcf8c..23daaa7 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -27,8 +27,7 @@ static void name_rev(struct commit *commit,\n \tstruct commit_list *parents;\n \tint parent_number = 1;\n \n-\tif (!commit->object.parsed)\n-\t\tparse_commit(commit);\n+\tparse_commit(commit);\n \n \tif (commit->date < cutoff)\n \t\treturn;\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 001f29c..46902c3 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -227,8 +227,7 @@ static void join_revs(struct commit_list **list_p,\n \t\t\tparents = parents->next;\n \t\t\tif ((this_flag & flags) == flags)\n \t\t\t\tcontinue;\n-\t\t\tif (!p->object.parsed)\n-\t\t\t\tparse_commit(p);\n+\t\t\tparse_commit(p);\n \t\t\tif (mark_seen(p, seen_p) && !still_interesting)\n \t\t\t\textra--;\n \t\t\tp->object.flags |= flags;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a0e0350..a141eb4 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -47,9 +47,8 @@ static void rev_list_push(struct commit *commit, int mark)\n \tif (!(commit->object.flags & mark)) {\n \t\tcommit->object.flags |= mark;\n \n-\t\tif (!(commit->object.parsed))\n-\t\t\tif (parse_commit(commit))\n-\t\t\t\treturn;\n+\t\tif (parse_commit(commit))\n+\t\t\treturn;\n \n \t\tprio_queue_put(&rev_list, commit);\n \n@@ -128,8 +127,7 @@ static const unsigned char *get_rev(void)\n \t\t\treturn NULL;\n \n \t\tcommit = prio_queue_get(&rev_list);\n-\t\tif (!commit->object.parsed)\n-\t\t\tparse_commit(commit);\n+\t\tparse_commit(commit);\n \t\tparents = commit->parents;\n \n \t\tcommit->object.flags |= POPPED;\n-- \n1.8.4.1.4.gf327177\n"},{"id":"228660","messageId":"20131008135625.GB7650@sigill.intra.peff.net","threadId":"35092","inReplyTo":"20131008134843.GA7530@sigill.intra.peff.net","subject":"[PATCH 2/5] assume parse_commit checks for NULL commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-08T13:56:25Z","receivedAt":"2013-10-08T13:56:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_commit function will check whether it was passed a\nNULL commit pointer, and if so, return an error. There is no\nneed for callers to check this separately.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThere are a few cases left untouched, because they actually produce a\ndifferent result for the NULL case (e.g., printing a different error\nmessage, as in read_tree_1).\n\nNote also the case of:\n\n  if (parent && parse_commit(parent) < 0)\n\nin sequencer.c, which looks at first glance like it falls in this class,\nbut doesn't (it is specifically avoiding the NULL check and reporting an\nerror only when we actually have something to parse at all).\n\n builtin/branch.c | 2 +-\n builtin/commit.c | 4 ++--\n commit.c         | 2 +-\n notes-utils.c    | 2 +-\n sha1_name.c      | 2 --\n 5 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ad0f86d..491090f 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -496,7 +496,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n \tconst char *sub = _(\" **** invalid ref ****\");\n \tstruct commit *commit = item->commit;\n \n-\tif (commit && !parse_commit(commit)) {\n+\tif (!parse_commit(commit)) {\n \t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);\n \t\tsub = subject.buf;\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 6ab4605..e89c519 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1338,7 +1338,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \tcommit = lookup_commit(sha1);\n \tif (!commit)\n \t\tdie(_(\"couldn't look up newly created commit\"));\n-\tif (!commit || parse_commit(commit))\n+\tif (parse_commit(commit))\n \t\tdie(_(\"could not parse newly created commit\"));\n \n \tstrbuf_addstr(&format, \"format:%h] %s\");\n@@ -1525,7 +1525,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tcurrent_head = NULL;\n \telse {\n \t\tcurrent_head = lookup_commit_or_die(sha1, \"HEAD\");\n-\t\tif (!current_head || parse_commit(current_head))\n+\t\tif (parse_commit(current_head))\n \t\t\tdie(_(\"could not parse HEAD commit\"));\n \t}\n \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\ndiff --git a/commit.c b/commit.c\nindex 51a9bbc..11509ff 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -79,7 +79,7 @@ struct commit *lookup_commit_reference_by_name(const char *name)\n \tif (get_sha1_committish(name, sha1))\n \t\treturn NULL;\n \tcommit = lookup_commit_reference(sha1);\n-\tif (!commit || parse_commit(commit))\n+\tif (parse_commit(commit))\n \t\treturn NULL;\n \treturn commit;\n }\ndiff --git a/notes-utils.c b/notes-utils.c\nindex 9107c37..7bb3473 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -18,7 +18,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n \t\tunsigned char parent_sha1[20];\n \t\tif (!read_ref(t->ref, parent_sha1)) {\n \t\t\tstruct commit *parent = lookup_commit(parent_sha1);\n-\t\t\tif (!parent || parse_commit(parent))\n+\t\t\tif (parse_commit(parent))\n \t\t\t\tdie(\"Failed to find/parse commit %s\", t->ref);\n \t\t\tcommit_list_insert(parent, &parents);\n \t\t}\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 0e5fe7f..1dfc401 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -582,8 +582,6 @@ static int get_parent(const char *name, int len,\n \tif (ret)\n \t\treturn ret;\n \tcommit = lookup_commit_reference(sha1);\n-\tif (!commit)\n-\t\treturn -1;\n \tif (parse_commit(commit))\n \t\treturn -1;\n \tif (!idx) {\n-- \n1.8.4.1.4.gf327177\n"},{"id":"228658","messageId":"20131008135716.GC7650@sigill.intra.peff.net","threadId":"35092","inReplyTo":"20131008134843.GA7530@sigill.intra.peff.net","subject":"[PATCH 3/5] use parse_commit_or_die instead of segfaulting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-08T13:57:16Z","receivedAt":"2013-10-08T13:57:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some unchecked calls to parse_commit should obviously die on\nerror, because their next step is to start looking at the\nparsed fields, which will cause a segfault. These are\nobvious candidates for parse_commit_or_die, which will be a\nstrict improvement in behavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c    | 4 ++--\n builtin/fast-export.c | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0f57397..34a2e43 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -789,7 +789,7 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tnew->commit = old.commit;\n \t\tif (!new->commit)\n \t\t\tdie(_(\"You are on a branch yet to be born\"));\n-\t\tparse_commit(new->commit);\n+\t\tparse_commit_or_die(new->commit);\n \t}\n \n \tret = merge_working_tree(opts, &old, new, &writeout_error);\n@@ -952,7 +952,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t/* not a commit */\n \t\t*source_tree = parse_tree_indirect(rev);\n \t} else {\n-\t\tparse_commit(new->commit);\n+\t\tparse_commit_or_die(new->commit);\n \t\t*source_tree = new->commit->tree;\n \t}\n \ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 78250ea..ea63052 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -287,7 +287,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \n \trev->diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \n-\tparse_commit(commit);\n+\tparse_commit_or_die(commit);\n \tauthor = strstr(commit->buffer, \"\\nauthor \");\n \tif (!author)\n \t\tdie (\"Could not find author in commit %s\",\n@@ -308,7 +308,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \tif (commit->parents &&\n \t    get_object_mark(&commit->parents->item->object) != 0 &&\n \t    !full_tree) {\n-\t\tparse_commit(commit->parents->item);\n+\t\tparse_commit_or_die(commit->parents->item);\n \t\tdiff_tree_sha1(commit->parents->item->tree->object.sha1,\n \t\t\t       commit->tree->object.sha1, \"\", &rev->diffopt);\n \t}\n-- \n1.8.4.1.4.gf327177\n"},{"id":"228656","messageId":"20131008135750.GD7650@sigill.intra.peff.net","threadId":"35092","inReplyTo":"20131008134843.GA7530@sigill.intra.peff.net","subject":"[PATCH 4/5] use parse_commit_or_die instead of custom message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-08T13:57:50Z","receivedAt":"2013-10-08T13:57:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many calls to parse_commit detect errors and die. In some\ncases, the custom error messages are more useful than what\nparse_commit_or_die could produce, because they give some\ncontext, like which ref the commit came from. Some, however,\njust say \"invalid commit\". Let's convert the latter to use\nparse_commit_or_die; its message is slightly more informative,\nand it makes the error more consistent throughout git.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n shallow.c     | 3 +--\n upload-pack.c | 3 +--\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex cdf37d6..961cf6f 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -90,8 +90,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \t\t\t\tcur_depth = *(int *)commit->util;\n \t\t\t}\n \t\t}\n-\t\tif (parse_commit(commit))\n-\t\t\tdie(\"invalid commit\");\n+\t\tparse_commit_or_die(commit);\n \t\tcur_depth++;\n \t\tif (cur_depth >= depth) {\n \t\t\tcommit_list_insert(commit, &result);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex a6c54e0..abe6636 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -649,8 +649,7 @@ static void receive_needs(void)\n \t\t\t\t/* make sure the real parents are parsed */\n \t\t\t\tunregister_shallow(object->sha1);\n \t\t\t\tobject->parsed = 0;\n-\t\t\t\tif (parse_commit((struct commit *)object))\n-\t\t\t\t\tdie(\"invalid commit\");\n+\t\t\t\tparse_commit_or_die((struct commit *)object);\n \t\t\t\tparents = ((struct commit *)object)->parents;\n \t\t\t\twhile (parents) {\n \t\t\t\t\tadd_object_array(&parents->item->object,\n-- \n1.8.4.1.4.gf327177\n"},{"id":"228661","messageId":"20131008140005.GE7650@sigill.intra.peff.net","threadId":"35092","inReplyTo":"20131008134843.GA7530@sigill.intra.peff.net","subject":"[PATCH 5/5] checkout: do not die when leaving broken detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-08T14:00:06Z","receivedAt":"2013-10-08T14:00:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we move away from a detached HEAD that has broken or\ncorrupted commits, we might die in two places:\n\n  1. Printing the \"old HEAD was...\" message.\n\n  2. Printing the list of orphaned commits.\n\nIn both cases, we ignore the return value of parse_commit\nand feed the resulting commit to the pretty-print machinery,\nwhich will die() upon failing to read the commit object\nitself.\n\nSince both cases are ancillary to the real operation being\nperformed, let's be more robust and keep going. This lets\nuser more easily checkout away from broken history.\n\nNote that the call in describe_detached_head is also used to\ndescribe the new commit we are moving to. We would want to\ndie in that case, but that is already handled much earlier,\nwhen we parse the commit to get the tree to move to.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 34a2e43..1df55c0 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -380,8 +380,8 @@ static void describe_detached_head(const char *msg, struct commit *commit)\n static void describe_detached_head(const char *msg, struct commit *commit)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tparse_commit(commit);\n-\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);\n+\tif (!parse_commit(commit))\n+\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);\n \tfprintf(stderr, \"%s %s... %s\\n\", msg,\n \t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV), sb.buf);\n \tstrbuf_release(&sb);\n@@ -677,12 +677,12 @@ static void describe_one_orphan(struct strbuf *sb, struct commit *commit)\n \n static void describe_one_orphan(struct strbuf *sb, struct commit *commit)\n {\n-\tparse_commit(commit);\n \tstrbuf_addstr(sb, \"  \");\n \tstrbuf_addstr(sb,\n \t\tfind_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));\n \tstrbuf_addch(sb, ' ');\n-\tpp_commit_easy(CMIT_FMT_ONELINE, commit, sb);\n+\tif (!parse_commit(commit))\n+\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, sb);\n \tstrbuf_addch(sb, '\\n');\n }\n \n-- \n1.8.4.1.4.gf327177\n"}]}