"Garima Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Garima Singh <garima.singh@xxxxxxxxxxxxx> > > Add GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS test flag to the test setup suite in > order to toggle writing bloom filters when running any of the git tests. If set > to true, we will compute and write bloom filters every time a test calls > `git commit-graph write`. OK, so it works in addition to GIT_TEST_COMMIT_GRAPH. > > The test suite passes when GIT_TEST_COMMIT_GRAPH and > GIT_COMMIT_GRAPH_BLOOM_FILTERS are enabled. Good. Very good. No errors found by Continuous Integration setup either? > > Helped-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> > Signed-off-by: Garima Singh <garima.singh@xxxxxxxxxxxxx> > --- > builtin/commit-graph.c | 2 +- > ci/run-build-and-tests.sh | 1 + > commit-graph.h | 1 + > t/README | 3 +++ > t/t4216-log-bloom.sh | 3 +++ > t/t5318-commit-graph.sh | 2 ++ > t/t5324-split-commit-graph.sh | 1 + > t/t5325-commit-graph-bloom.sh | 3 +++ > 8 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 9bd1e11161..97167959b2 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -146,7 +146,7 @@ static int graph_write(int argc, const char **argv) > flags |= COMMIT_GRAPH_WRITE_SPLIT; > if (opts.progress) > flags |= COMMIT_GRAPH_WRITE_PROGRESS; > - if (opts.enable_bloom_filters) > + if (opts.enable_bloom_filters || git_env_bool(GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS, 0)) > flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS; > Very minor nitpick: not to make this line long, I would break it at the boolean operator, that is write - if (opts.enable_bloom_filters) + if (opts.enable_bloom_filters || + git_env_bool(GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS, 0)) flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS; I agree that this is a good place to put this check, by pretending that `--changed-paths` option was given on command line. Looks good. > read_replace_refs = 0; > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index ff0ef7f08e..19d0846d34 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -19,6 +19,7 @@ linux-gcc) > export GIT_TEST_OE_SIZE=10 > export GIT_TEST_OE_DELTA_SIZE=5 > export GIT_TEST_COMMIT_GRAPH=1 > + export GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=1 > export GIT_TEST_MULTI_PACK_INDEX=1 > make test > ;; All right, adding this to CI would certainly exercise this feature. > diff --git a/commit-graph.h b/commit-graph.h > index 2202ad91ae..d914e6abf1 100644 > --- a/commit-graph.h > +++ b/commit-graph.h > @@ -8,6 +8,7 @@ > > #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH" > #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD" > +#define GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS "GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS" > All right (the ordering is a mater of taste). > struct commit; > struct bloom_filter_settings; > diff --git a/t/README b/t/README > index caa125ba9a..399b190437 100644 > --- a/t/README > +++ b/t/README > @@ -378,6 +378,9 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to > be written after every 'git commit' command, and overrides the > 'core.commitGraph' setting to true. > > +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=<boolean>, when true, forces commit-graph > +write to compute and write bloom filters for every 'git commit-graph write' > + Thanks for documenting this. Missing full stop '.' at the end of the sentence. (Minor nit). We might want to add ", as if '--changed-paths' option was given.", but it is not strictly necessary. > GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor > code path for utilizing a file system monitor to speed up detecting > new or changed files. > diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh > index d42f077998..0e092b387c 100755 > --- a/t/t4216-log-bloom.sh > +++ b/t/t4216-log-bloom.sh > @@ -3,6 +3,9 @@ > test_description='git log for a path with bloom filters' > . ./test-lib.sh > > +GIT_TEST_COMMIT_GRAPH=0 > +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0 > + OK, neither of those setting increase the coverage for this test. On the other hand they won't make tests fail (or at least they shouldn't, I think). The t5318-commit-graph.sh doesn't include GIT_TEST_COMMIT_GRAPH=0, after all. > test_expect_success 'setup repo' ' > git init && > git config core.commitGraph true && > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > index 3f03de6018..613228bb12 100755 > --- a/t/t5318-commit-graph.sh > +++ b/t/t5318-commit-graph.sh > @@ -3,6 +3,8 @@ > test_description='commit graph' > . ./test-lib.sh > > +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0 > + All right, adding Bloom filters to commit-graph file would make test cases utilizing graph_read_expect() fail. > test_expect_success 'setup full repo' ' > mkdir full && > cd "$TRASH_DIRECTORY/full" && > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh > index c24823431f..181ca7e0cb 100755 > --- a/t/t5324-split-commit-graph.sh > +++ b/t/t5324-split-commit-graph.sh > @@ -4,6 +4,7 @@ test_description='split commit graph' > . ./test-lib.sh > > GIT_TEST_COMMIT_GRAPH=0 > +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0 All right, same here: adding Bloom filters to commit-graph would make test cases utilizing graph_read_expect() fail. Sidenote: Here without GIT_TEST_COMMIT_GRAPH=0 tests that rely on precise timing of writing commit-graph to create split commit-graph would fail. > > test_expect_success 'setup repo' ' > git init && > diff --git a/t/t5325-commit-graph-bloom.sh b/t/t5325-commit-graph-bloom.sh > index d7ef0e7fb3..a9c9e9fef6 100755 > --- a/t/t5325-commit-graph-bloom.sh > +++ b/t/t5325-commit-graph-bloom.sh > @@ -3,6 +3,9 @@ > test_description='commit graph with bloom filters' > . ./test-lib.sh > > +GIT_TEST_COMMIT_GRAPH=0 > +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0 > + This test also includes some split commit-graph test cases, so the above is necessary. All right. > test_expect_success 'setup repo' ' > git init && > git config core.commitGraph true && Best, -- Jakub Narębski
