Re: [PATCH 9/9] commit-graph: add GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS test flag

[Jakub Narebski <jnareb@gmail.com>]

"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Garima Singh <garima.singh@microsoft.com>
>
> 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@microsoft.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
>  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