On Tue, Sep 26, 2023 at 01:54:52AM -0400, Jeff King wrote:
> [1/6]: commit-graph: factor out chain opening function
> [2/6]: commit-graph: check mixed generation validation when loading chain file
> [3/6]: t5324: harmonize sha1/sha256 graph chain corruption
> [4/6]: commit-graph: detect read errors when verifying graph chain
> [5/6]: commit-graph: tighten chain size check
> [6/6]: commit-graph: report incomplete chains during verification
Here's a re-roll that fixes a missed case in patch 6 (I sent more
details elsewhere in the thread).
Range-diff is below, but the changes are:
- patch 3 extends its tests to cover corrupting both the first and
second lines of the file. The test and directory renaming spills
over into the range-diff context for the other patches (and I
renamed the directory used in the "too-short chain file" test in
patch 5 to align better with the others).
- patch 6 covers the case that get_oid_hex() fails (previously it only
detected that a parsed hex failed to result in us loading a valid
graph file). The range-diff is IMHO quite hard to read here, but the
patch itself is quite simple.
1: baa20e6391 = 1: a4893f325f commit-graph: factor out chain opening function
2: b77cc15584 = 2: 722181d8ed commit-graph: check mixed generation validation when loading chain file
3: af350d1d41 < -: ---------- t5324: harmonize sha1/sha256 graph chain corruption
-: ---------- > 3: 7de47054c7 t5324: harmonize sha1/sha256 graph chain corruption
4: 54ce3863f8 ! 4: 313c051a38 commit-graph: detect read errors when verifying graph chain
@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv, con
free_commit_graph(graph);
## t/t5324-split-commit-graph.sh ##
-@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption' '
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (base)' '
(
- cd verify-chain &&
+ cd verify-chain-base &&
corrupt_file "$graphdir/commit-graph-chain" 30 "G" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
5: 273f5e8b87 ! 5: 6d9efd761a commit-graph: tighten chain size check
@@ commit-graph.c: int open_commit_graph_chain(const char *chain_file,
return 1;
## t/t5324-split-commit-graph.sh ##
-@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption' '
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (tip)' '
)
'
+test_expect_success 'verify notices too-short chain file' '
-+ git clone --no-hardlinks . verify-chain-garbage &&
++ git clone --no-hardlinks . verify-chain-short &&
+ (
-+ cd verify-chain-garbage &&
++ cd verify-chain-short &&
+ git commit-graph verify &&
+ echo "garbage" >$graphdir/commit-graph-chain &&
+ test_must_fail git commit-graph verify 2>test_err &&
6: 2661efed03 ! 6: 8fc82e2254 commit-graph: report incomplete chains during verification
@@ Commit message
"tip" case is what is fixed by this patch (it complains to stderr but
still returns the base slice).
+ Likewise the existing tests for corruption of the commit-graph-chain
+ file itself need to be updated. We already exited non-zero correctly for
+ the "base" case, but the "tip" case can now do so, too.
+
Note that this also causes us to adjust a test later in the file that
similarly corrupts a tip (though confusingly the test script calls this
"base"). It checks stderr but erroneously expects the whole "verify"
@@ commit-graph.c: int open_commit_graph_chain(const char *chain_file,
{
struct commit_graph *graph_chain = NULL;
struct strbuf line = STRBUF_INIT;
- struct object_id *oids;
- int i = 0, valid = 1, count;
- FILE *fp = xfdopen(fd, "r");
-
-+ *incomplete_chain = 0;
-+
- count = st->st_size / (the_hash_algo->hexsz + 1);
- CALLOC_ARRAY(oids, count);
-
@@ commit-graph.c: struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
+ fclose(fp);
+ strbuf_release(&line);
+
++ *incomplete_chain = !valid;
+ return graph_chain;
+ }
- if (!valid) {
- warning(_("unable to find all commit-graph files"));
-+ *incomplete_chain = 1;
- break;
- }
- }
@@ commit-graph.c: static struct commit_graph *load_commit_graph_chain(struct repository *r,
struct commit_graph *g = NULL;
@@ t/t5324-split-commit-graph.sh: test_expect_success 'warn on base graph chunk inc
grep -v "^+" test_err >err &&
test_i18ngrep "commit-graph chain does not match" err
)
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (tip)' '
+ (
+ cd verify-chain-tip &&
+ corrupt_file "$graphdir/commit-graph-chain" 70 "G" &&
+- git commit-graph verify 2>test_err &&
++ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ test_i18ngrep "invalid commit-graph chain" err &&
+ corrupt_file "$graphdir/commit-graph-chain" 70 "A" &&
+- git commit-graph verify 2>test_err &&
++ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ test_i18ngrep "unable to find all commit-graph files" err
+ )
[1/6]: commit-graph: factor out chain opening function
[2/6]: commit-graph: check mixed generation validation when loading chain file
[3/6]: t5324: harmonize sha1/sha256 graph chain corruption
[4/6]: commit-graph: detect read errors when verifying graph chain
[5/6]: commit-graph: tighten chain size check
[6/6]: commit-graph: report incomplete chains during verification
builtin/commit-graph.c | 31 +++++++---
commit-graph.c | 109 +++++++++++++++++++++-------------
commit-graph.h | 4 ++
t/t5324-split-commit-graph.sh | 69 ++++++++++++++++++---
4 files changed, 158 insertions(+), 55 deletions(-)
-Peff
