Re: [PATCH v2 1/4] test-read-graph: include extra post-parse info

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Feb 28 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
>
> It can be helpful to verify that the 'struct commit_graph' that results
> from parsing a commit-graph is correctly structured. The existence of
> different chunks is not enough to verify that all of the optional
> features are correctly enabled.
>
> Update 'test-tool read-graph' to output an "options:" line that includes
> information for different parts of the struct commit_graph.
>
> In particular, this change demonstrates that the read_generation_data
> option is never being enabled, which will be fixed in a later change.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  t/helper/test-read-graph.c    | 13 +++++++++++++
>  t/t4216-log-bloom.sh          |  1 +
>  t/t5318-commit-graph.sh       |  1 +
>  t/t5324-split-commit-graph.sh |  5 +++++
>  4 files changed, 20 insertions(+)
>
> diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
> index 75927b2c81d..c3b6b8d1734 100644
> --- a/t/helper/test-read-graph.c
> +++ b/t/helper/test-read-graph.c
> @@ -3,6 +3,7 @@
>  #include "commit-graph.h"
>  #include "repository.h"
>  #include "object-store.h"
> +#include "bloom.h"
>  
>  int cmd__read_graph(int argc, const char **argv)
>  {
> @@ -45,6 +46,18 @@ int cmd__read_graph(int argc, const char **argv)
>  		printf(" bloom_data");
>  	printf("\n");
>  
> +	printf("options:");
> +	if (graph->bloom_filter_settings)
> +		printf(" bloom(%d,%d,%d)",

I think this is probably unportable, as other code (including in
commit-graph.c) uses "%"PRIu32 when printing uint32_t.

Does this work on our Linux32 job? I was going to quickly check the PR
CI, but it appears the run was skipped for some reason.

> +		       graph->bloom_filter_settings->hash_version,
> +		       graph->bloom_filter_settings->bits_per_entry,
> +		       graph->bloom_filter_settings->num_hashes);
> +	if (graph->read_generation_data)
> +		printf(" read_generation_data");
> +	if (graph->topo_levels)
> +		printf(" topo_levels");
> +	printf("\n");
> +
>  	UNLEAK(graph);
>  
>  	return 0;
> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> index cc3cebf6722..5ed6d2a21c1 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -48,6 +48,7 @@ graph_read_expect () {
>  	header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
>  	num_commits: $1
>  	chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data
> +	options: bloom(1,10,7)
>  	EOF
>  	test-tool read-graph >actual &&
>  	test_cmp expect actual
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index edb728f77c3..2b05026cf6d 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -104,6 +104,7 @@ graph_read_expect() {
>  	header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0
>  	num_commits: $1
>  	chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL
> +	options:
>  	EOF
>  	test-tool read-graph >output &&
>  	test_cmp expect output
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 847b8097109..778fa418de2 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -34,6 +34,7 @@ graph_read_expect() {
>  	header: 43475048 1 $(test_oid oid_version) 4 $NUM_BASE
>  	num_commits: $1
>  	chunks: oid_fanout oid_lookup commit_metadata generation_data
> +	options:
>  	EOF
>  	test-tool read-graph >output &&
>  	test_cmp expect output
> @@ -508,6 +509,7 @@ test_expect_success 'setup repo for mixed generation commit-graph-chain' '
>  		header: 43475048 1 $(test_oid oid_version) 4 1
>  		num_commits: $NUM_SECOND_LAYER_COMMITS
>  		chunks: oid_fanout oid_lookup commit_metadata
> +		options:
>  		EOF
>  		test_cmp expect output &&
>  		git commit-graph verify &&
> @@ -540,6 +542,7 @@ test_expect_success 'do not write generation data chunk if not present on existi
>  		header: 43475048 1 $(test_oid oid_version) 4 2
>  		num_commits: $NUM_THIRD_LAYER_COMMITS
>  		chunks: oid_fanout oid_lookup commit_metadata
> +		options:
>  		EOF
>  		test_cmp expect output &&
>  		git commit-graph verify
> @@ -581,6 +584,7 @@ test_expect_success 'do not write generation data chunk if the topmost remaining
>  		header: 43475048 1 $(test_oid oid_version) 4 2
>  		num_commits: $(($NUM_THIRD_LAYER_COMMITS + $NUM_FOURTH_LAYER_COMMITS))
>  		chunks: oid_fanout oid_lookup commit_metadata
> +		options:
>  		EOF
>  		test_cmp expect output &&
>  		git commit-graph verify
> @@ -620,6 +624,7 @@ test_expect_success 'write generation data chunk if topmost remaining layer has
>  		header: 43475048 1 $(test_oid oid_version) 5 1
>  		num_commits: $(($NUM_SECOND_LAYER_COMMITS + $NUM_THIRD_LAYER_COMMITS + $NUM_FOURTH_LAYER_COMMITS + $NUM_FIFTH_LAYER_COMMITS))
>  		chunks: oid_fanout oid_lookup commit_metadata generation_data
> +		options:
>  		EOF
>  		test_cmp expect output
>  	)

I think the rest of this all looks good and obviously correct.