@matheusbsbs / matheus-bagliano / commits / 228c78fbd4
Fork 0

commit, tag: don't set parsed bit for parse failures

If we can't parse a commit, then parse_commit() will return an error code. But it _also_ sets the "parsed" flag, which tells us not to bother trying to re-parse the object. That means that subsequent parses have no idea that the information in the struct may be bogus. I.e., doing this: parse_commit(commit); ... if (parse_commit(commit) < 0) die("commit is broken"); will never trigger the die(). The second parse_commit() will see the "parsed" flag and quietly return success. There are two obvious ways to fix this: 1. Stop setting "parsed" until we've successfully parsed. 2. Keep a second "corrupt" flag to indicate that we saw an error (and when the parsed flag is set, return 0/-1 depending on the corrupt flag). This patch does option 1. The obvious downside versus option 2 is that we might continually re-parse a broken object. But in practice, corruption like this is rare, and we typically die() or return an error in the caller. So it's OK not to worry about optimizing for corruption. And it's much simpler: we don't need to use an extra bit in the object struct, and callers which check the "parsed" flag don't need to learn about the corrupt bit, too. There's no new test here, because this case is already covered in t5318. Note that we do need to update the expected message there, because we now detect the problem in the return from "parse_commit()", and not with a separate check for a NULL tree. In fact, we can now ditch that explicit tree check entirely, as we're covered robustly by this change (and the previous recent change to treat a NULL tree as a parse error). We'll also give tags the same treatment. I don't know offhand of any cases where the problem can be triggered (it implies somebody ignoring a parse error earlier in the process), but consistently returning an error should cause the least surprise. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 25, 2019 at 17:20 UTC 228c78fbd42b58ebf43477290432c149358b04b1
4 files changed +25 -6
commit-graph.c
-3
@@ -855,9 +855,6 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
855 die(_("unable to parse commit %s"),
856 oid_to_hex(&(*list)->object.oid));
857 tree = get_commit_tree_oid(*list);
858 - if (!tree)
859 - die(_("unable to get tree for %s"),
860 - oid_to_hex(&(*list)->object.oid));
858 hashwrite(f, tree->hash, hash_len);
859
860 parent = (*list)->parents;
commit.c
+13 -1
@@ -405,7 +405,18 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
405
406 if (item->object.parsed)
407 return 0;
408 - item->object.parsed = 1;
408 +
409 + if (item->parents) {
410 + /*
411 + * Presumably this is leftover from an earlier failed parse;
412 + * clear it out in preparation for us re-parsing (we'll hit the
413 + * same error, but that's good, since it lets our caller know
414 + * the result cannot be trusted.
415 + */
416 + free_commit_list(item->parents);
417 + item->parents = NULL;
418 + }
419 +
420 tail += size;
421 if (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, "tree ", 5) ||
422 bufptr[tree_entry_len] != '\n')
@@ -462,6 +473,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
473 if (check_graph)
474 load_commit_graph_info(r, item);
475
476 + item->object.parsed = 1;
477 return 0;
478 }
479
t/t5318-commit-graph.sh
+1 -1
@@ -660,7 +660,7 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '
660 git commit-tree -p "$broken" -m "good" "$tree" >good &&
661 test_must_fail git commit-graph write --stdin-commits \
662 <good 2>test_err &&
663 - test_i18ngrep "unable to get tree for" test_err
663 + test_i18ngrep "unable to parse commit" test_err
664 )
665 '
666
tag.c
+11 -1
@@ -141,7 +141,16 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u
141
142 if (item->object.parsed)
143 return 0;
144 - item->object.parsed = 1;
144 +
145 + if (item->tag) {
146 + /*
147 + * Presumably left over from a previous failed parse;
148 + * clear it out in preparation for re-parsing (we'll probably
149 + * hit the same error, which lets us tell our current caller
150 + * about the problem).
151 + */
152 + FREE_AND_NULL(item->tag);
153 + }
154
155 if (size < the_hash_algo->hexsz + 24)
156 return -1;
@@ -192,6 +201,7 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u
201 else
202 item->date = 0;
203
204 + item->object.parsed = 1;
205 return 0;
206 }
207