Re: [PATCH 1/3] cache-tree: refactor verification to return error codes
From: Eric Sunshine <hidden>
Date: 2024-09-17 17:06:02
On Tue, Sep 17, 2024 at 3:13 AM Patrick Steinhardt [off-list ref] wrote:
The function `cache_tree_verify()` will `BUG()` whenever it finds that the cache-tree extension of the index is corrupt. The function is thus inherently untestable because the resulting call to `abort()` will be detected by our testing framework and labelled an error. And rightfully so: it shouldn't ever be possible to hit bugs, as they should indicate a programming error rather than corruption of on-disk state. Refactor the function to instead return error codes. This also ensures that the function can be used e.g. by git-fsck(1) without the whole process dying.
Makes sense.
quoted hunk ↗ jump to hunk
Signed-off-by: Patrick Steinhardt <redacted> ---diff --git a/cache-tree.c b/cache-tree.c@@ -890,18 +892,23 @@ static int verify_one(struct repository *r, struct strbuf tree_buf = STRBUF_INIT; for (i = 0; i < it->subtree_nr; i++) { strbuf_addf(path, "%s/", it->down[i]->name); - if (verify_one(r, istate, it->down[i]->cache_tree, path)) - return 1; + ret = verify_one(r, istate, it->down[i]->cache_tree, path); + if (ret) + goto out;
Assuming I am understanding correctly that the original code was leaking the strbuf by returning early, I was surprised that the commit message didn't mention that the patch is also fixing the leak. (Probably not worth a reroll, though.)