On 1/7/2020 11:01 AM, Jakub Narebski wrote:
> "Garima Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>> +
>> + if (hash_version != 1)
>> + break;
>
> What does it mean for Git? Behave as if there were no Bloom filter
> data?
>
Yes. We choose to use Bloom filters best effort, and in such cases, we will
just fall back to the original code path.
>> if (ctx->num_commit_graphs_after > 1 &&
>> write_graph_chunk_base(f, ctx)) {
>> return -1;
>> diff --git a/commit-graph.h b/commit-graph.h
>> index 952a4b83be..2202ad91ae 100644
>> --- a/commit-graph.h
>> +++ b/commit-graph.h
>> @@ -10,6 +10,7 @@
>> #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
>>
>> struct commit;
>> +struct bloom_filter_settings;
>>
>> char *get_commit_graph_filename(const char *obj_dir);
>> int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
>> @@ -58,6 +59,10 @@ struct commit_graph {
>> const unsigned char *chunk_commit_data;
>> const unsigned char *chunk_extra_edges;
>> const unsigned char *chunk_base_graphs;
>> + const unsigned char *chunk_bloom_indexes;
>> + const unsigned char *chunk_bloom_data;
>> +
>> + struct bloom_filter_settings *settings;
>
> Should this be part of `struct commit_graph`? Shouldn't we free() this
> data, or is it a pointer into xmmap-ped file... no it isn't -- we
> xalloc() it, so we should free() it.
>
> I think it should be done in 'cleanup:' section of write_commit_graph(),
> but I am not entirely sure.
>
Thanks for calling this out! This is definitely a bug in how commit-graph.c
frees up the graph. The right way to free the graph would be to call
free_commit_graph() instead of free(graph) like many places in that file.
Cleaning up this entire pattern would be orthogonal to this series, so
I will follow up with a separate series that cleans it up overall.
For now, I will free up `bloom_filter_settings` in free_commit_graph().
Cheers!
Garima Singh
