From 7059dccc6c60a872a314b19ac17702065a71d6bd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Oct 2013 04:52:36 -0400 Subject: log_tree_diff: die when we fail to parse a commit We currently call parse_commit and then assume we can dereference the resulting "tree" struct field. If parsing failed, however, that field is NULL and we end up segfaulting. Instead of a segfault, let's print an error message and die a little more gracefully. Note that this should never happen in practice, but may happen in a corrupt repository. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/commit.c b/commit.c index a575564..26c1d54 100644 --- a/commit.c +++ b/commit.c @@ -341,6 +341,13 @@ int parse_commit(struct commit *item) return ret; } +void parse_commit_or_die(struct commit *item) +{ + if (parse_commit(item)) + die("unable to parse commit %s", + item ? sha1_to_hex(item->object.sha1) : "(null)"); +} + int find_commit_subject(const char *commit_buffer, const char **subject) { const char *eol; diff --git a/commit.h b/commit.h index d912a9d..a3645da 100644 --- a/commit.h +++ b/commit.h @@ -49,6 +49,7 @@ struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size); int parse_commit(struct commit *item); +void parse_commit_or_die(struct commit *item); /* Find beginning and length of commit subject. */ int find_commit_subject(const char *commit_buffer, const char **subject); diff --git a/log-tree.c b/log-tree.c index a49d8e8..11604b1 100644 --- a/log-tree.c +++ b/log-tree.c @@ -734,7 +734,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)) return 0; - parse_commit(commit); + parse_commit_or_die(commit); sha1 = commit->tree->object.sha1; /* Root commit? */ @@ -759,7 +759,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log * parent, showing summary diff of the others * we merged _in_. */ - parse_commit(parents->item); + parse_commit_or_die(parents->item); diff_tree_sha1(parents->item->tree->object.sha1, sha1, "", &opt->diffopt); log_tree_diff_flush(opt); @@ -774,7 +774,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log for (;;) { struct commit *parent = parents->item; - parse_commit(parent); + parse_commit_or_die(parent); diff_tree_sha1(parent->tree->object.sha1, sha1, "", &opt->diffopt); log_tree_diff_flush(opt); -- cgit v0.10.2-6-g49f6 From 0064053bd76ba385e1b5d51b6175bc17bc507804 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Oct 2013 04:53:01 -0400 Subject: assume parse_commit checks commit->object.parsed The parse_commit function will check the "parsed" flag of the object and do nothing if it is set. There is no need for callers to check the flag themselves, and doing so only clutters the code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/blame.c b/builtin/blame.c index 079dcd3..4d25466 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1554,8 +1554,7 @@ static void assign_blame(struct scoreboard *sb, int opt) */ origin_incref(suspect); commit = suspect->commit; - if (!commit->object.parsed) - parse_commit(commit); + parse_commit(commit); if (reverse || (!(commit->object.flags & UNINTERESTING) && !(revs->max_age != -1 && commit->date < revs->max_age))) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 0aaa19e..26f4033 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -27,8 +27,7 @@ static void name_rev(struct commit *commit, struct commit_list *parents; int parent_number = 1; - if (!commit->object.parsed) - parse_commit(commit); + parse_commit(commit); if (commit->date < cutoff) return; diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 9788eb1..3afc79b 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -227,8 +227,7 @@ static void join_revs(struct commit_list **list_p, parents = parents->next; if ((this_flag & flags) == flags) continue; - if (!p->object.parsed) - parse_commit(p); + parse_commit(p); if (mark_seen(p, seen_p) && !still_interesting) extra--; p->object.flags |= flags; diff --git a/fetch-pack.c b/fetch-pack.c index 6684348..86b6977 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -46,9 +46,8 @@ static void rev_list_push(struct commit *commit, int mark) if (!(commit->object.flags & mark)) { commit->object.flags |= mark; - if (!(commit->object.parsed)) - if (parse_commit(commit)) - return; + if (parse_commit(commit)) + return; prio_queue_put(&rev_list, commit); @@ -127,8 +126,7 @@ static const unsigned char *get_rev(void) return NULL; commit = prio_queue_get(&rev_list); - if (!commit->object.parsed) - parse_commit(commit); + parse_commit(commit); parents = commit->parents; commit->object.flags |= POPPED; -- cgit v0.10.2-6-g49f6 From 5e7d4d3e932432131d0f8f4195e0061ecf644865 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Oct 2013 04:53:19 -0400 Subject: assume parse_commit checks for NULL commit The parse_commit function will check whether it was passed a NULL commit pointer, and if so, return an error. There is no need for callers to check this separately. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/branch.c b/builtin/branch.c index 0836890..8db095f 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -490,7 +490,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item, const char *sub = _(" **** invalid ref ****"); struct commit *commit = item->commit; - if (commit && !parse_commit(commit)) { + if (!parse_commit(commit)) { pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject); sub = subject.buf; } diff --git a/builtin/commit.c b/builtin/commit.c index 10acc53..89f65f2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1311,7 +1311,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1, commit = lookup_commit(sha1); if (!commit) die(_("couldn't look up newly created commit")); - if (!commit || parse_commit(commit)) + if (parse_commit(commit)) die(_("could not parse newly created commit")); strbuf_addstr(&format, "format:%h] %s"); @@ -1503,7 +1503,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) current_head = NULL; else { current_head = lookup_commit_or_die(sha1, "HEAD"); - if (!current_head || parse_commit(current_head)) + if (parse_commit(current_head)) die(_("could not parse HEAD commit")); } argc = parse_and_validate_options(argc, argv, builtin_commit_options, diff --git a/commit.c b/commit.c index 26c1d54..8535e5c 100644 --- a/commit.c +++ b/commit.c @@ -79,7 +79,7 @@ struct commit *lookup_commit_reference_by_name(const char *name) if (get_sha1_committish(name, sha1)) return NULL; commit = lookup_commit_reference(sha1); - if (!commit || parse_commit(commit)) + if (parse_commit(commit)) return NULL; return commit; } diff --git a/notes-utils.c b/notes-utils.c index 9107c37..7bb3473 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -18,7 +18,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents, unsigned char parent_sha1[20]; if (!read_ref(t->ref, parent_sha1)) { struct commit *parent = lookup_commit(parent_sha1); - if (!parent || parse_commit(parent)) + if (parse_commit(parent)) die("Failed to find/parse commit %s", t->ref); commit_list_insert(parent, &parents); } diff --git a/sha1_name.c b/sha1_name.c index 65ad066..729ab14 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -582,8 +582,6 @@ static int get_parent(const char *name, int len, if (ret) return ret; commit = lookup_commit_reference(sha1); - if (!commit) - return -1; if (parse_commit(commit)) return -1; if (!idx) { -- cgit v0.10.2-6-g49f6 From 683ff884cce955cc331929d857fac9d6bd69f46a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Oct 2013 04:53:46 -0400 Subject: use parse_commit_or_die instead of segfaulting Some unchecked calls to parse_commit should obviously die on error, because their next step is to start looking at the parsed fields, which will cause a segfault. These are obvious candidates for parse_commit_or_die, which will be a strict improvement in behavior. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 7025938..88ab948 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -796,7 +796,7 @@ static int switch_branches(const struct checkout_opts *opts, new->commit = old.commit; if (!new->commit) die(_("You are on a branch yet to be born")); - parse_commit(new->commit); + parse_commit_or_die(new->commit); } ret = merge_working_tree(opts, &old, new, &writeout_error); @@ -959,7 +959,7 @@ static int parse_branchname_arg(int argc, const char **argv, /* not a commit */ *source_tree = parse_tree_indirect(rev); } else { - parse_commit(new->commit); + parse_commit_or_die(new->commit); *source_tree = new->commit->tree; } diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 8e19058..7785c22 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -286,7 +286,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) rev->diffopt.output_format = DIFF_FORMAT_CALLBACK; - parse_commit(commit); + parse_commit_or_die(commit); author = strstr(commit->buffer, "\nauthor "); if (!author) die ("Could not find author in commit %s", @@ -307,7 +307,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) if (commit->parents && get_object_mark(&commit->parents->item->object) != 0 && !full_tree) { - parse_commit(commit->parents->item); + parse_commit_or_die(commit->parents->item); diff_tree_sha1(commit->parents->item->tree->object.sha1, commit->tree->object.sha1, "", &rev->diffopt); } -- cgit v0.10.2-6-g49f6 From 367068e0dd1b7d6b5b4029749b326c6bad172498 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Oct 2013 04:54:01 -0400 Subject: use parse_commit_or_die instead of custom message Many calls to parse_commit detect errors and die. In some cases, the custom error messages are more useful than what parse_commit_or_die could produce, because they give some context, like which ref the commit came from. Some, however, just say "invalid commit". Let's convert the latter to use parse_commit_or_die; its message is slightly more informative, and it makes the error more consistent throughout git. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/shallow.c b/shallow.c index 8a9c96d..a273685 100644 --- a/shallow.c +++ b/shallow.c @@ -89,8 +89,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth, cur_depth = *(int *)commit->util; } } - if (parse_commit(commit)) - die("invalid commit"); + parse_commit_or_die(commit); cur_depth++; if (cur_depth >= depth) { commit_list_insert(commit, &result); diff --git a/upload-pack.c b/upload-pack.c index 127e59a..c107686 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -694,8 +694,7 @@ static void receive_needs(void) /* make sure the real parents are parsed */ unregister_shallow(object->sha1); object->parsed = 0; - if (parse_commit((struct commit *)object)) - die("invalid commit"); + parse_commit_or_die((struct commit *)object); parents = ((struct commit *)object)->parents; while (parents) { add_object_array(&parents->item->object, -- cgit v0.10.2-6-g49f6 From 3c62183929080c17299d5b404eb092e3d53c161a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Oct 2013 04:54:53 -0400 Subject: checkout: do not die when leaving broken detached HEAD If we move away from a detached HEAD that has broken or corrupted commits, we might die in two places: 1. Printing the "old HEAD was..." message. 2. Printing the list of orphaned commits. In both cases, we ignore the return value of parse_commit and feed the resulting commit to the pretty-print machinery, which will die() upon failing to read the commit object itself. Since both cases are ancillary to the real operation being performed, let's be more robust and keep going. This lets users more easily checkout away from broken history. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 88ab948..4d6bb0c 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -387,8 +387,8 @@ static void show_local_changes(struct object *head, static void describe_detached_head(const char *msg, struct commit *commit) { struct strbuf sb = STRBUF_INIT; - parse_commit(commit); - pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb); + if (!parse_commit(commit)) + pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb); fprintf(stderr, "%s %s... %s\n", msg, find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV), sb.buf); strbuf_release(&sb); @@ -684,12 +684,12 @@ static int add_pending_uninteresting_ref(const char *refname, static void describe_one_orphan(struct strbuf *sb, struct commit *commit) { - parse_commit(commit); strbuf_addstr(sb, " "); strbuf_addstr(sb, find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV)); strbuf_addch(sb, ' '); - pp_commit_easy(CMIT_FMT_ONELINE, commit, sb); + if (!parse_commit(commit)) + pp_commit_easy(CMIT_FMT_ONELINE, commit, sb); strbuf_addch(sb, '\n'); } -- cgit v0.10.2-6-g49f6