On 2/20/2020 1:48 PM, Jakub Narebski wrote:
> "Garima Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Garima Singh <garima.singh@xxxxxxxxxxxxx>
>>
>> Read previously computed Bloom filters from the commit-graph file if
>> possible to avoid recomputing during commit-graph write.
>
> All right, what is written makes sense for this point in patch series.
>
> But it my opinion it is more important to state that this commit adds
> "parsing" of the Bloom filter data from commit-graph file. This means
> that it needs to be calculated only once, then stored in commit-graph,
> ready to be re-used.
>
Good point. Incorporated in v3.
>>
>> See Documentation/technical/commit-graph-format for the format in which
>> the Bloom filter information is written to the commit graph file.
>>
>> To read Bloom filter for a given commit with lexicographic position
>> 'i' we need to:
>> 1. Read BIDX[i] which essentially gives us the starting index in BDAT for
>> filter of commit i+1. It is essentially the index past the end
>> of the filter of commit i. It is called end_index in the code.
>>
>> 2. For i>0, read BIDX[i-1] which will give us the starting index in BDAT
>> for filter of commit i. It is called the start_index in the code.
>> For the first commit, where i = 0, Bloom filter data starts at the
>> beginning, just past the header in the BDAT chunk. Hence, start_index
>> will be 0.
>>
>> 3. The length of the filter will be end_index - start_index, because
>> BIDX[i] gives the cumulative 8-byte words including the ith
>> commit's filter.
>>
>> We toggle whether Bloom filters should be recomputed based on the
>> compute_if_null flag.
>
> Nitpick: the flag (the parameter) is called compute_if_not_present, not
> compute_if_null.
>
Oops. Fixed in v3.
>> +
>> + end_index = get_be32(g->chunk_bloom_indexes + 4 * lex_pos);
>> +
>> + if (lex_pos)
>
> Wouldn't it be better to be more explicit, and write
>
> + if (lex_pos > 0)
>
>
Sure.
>> + start_index = get_be32(g->chunk_bloom_indexes + 4 * (lex_pos - 1));
>> + else
>> + start_index = 0;
>
> All right, here we find start_index and end_index.
>
> It might be good idea to at least assert() that start_index <= end_index,
> though that should not happen (that is why I propose for this check to
> be compiled on only for debug builds).
>
I will look into this. Thanks!
>> @@ -1304,7 +1304,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
>>
>> for (i = 0; i < ctx->commits.nr; i++) {
>> struct commit *c = sorted_by_pos[i];
>> - struct bloom_filter *filter = get_bloom_filter(ctx->r, c);
>> + struct bloom_filter *filter = get_bloom_filter(ctx->r, c, 1);
>> ctx->total_bloom_filter_data_size += sizeof(uint64_t) * filter->len;
>> display_progress(progress, i + 1);
>> }
>> @@ -2314,6 +2314,7 @@ void free_commit_graph(struct commit_graph *g)
>> g->data = NULL;
>> close(g->graph_fd);
>> }
>> + free(g->bloom_filter_settings);
>> free(g->filename);
>> free(g);
>
> Shouldn't this fixup be added to earlier commit?
>
Yes.
>> }
>> diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
>> index 331957011b..9b4be97f75 100644
>> --- a/t/helper/test-bloom.c
>> +++ b/t/helper/test-bloom.c
>> @@ -47,7 +47,7 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)
>> struct bloom_filter *filter;
>> setup_git_directory();
>> c = lookup_commit(the_repository, commit_oid);
>> - filter = get_bloom_filter(the_repository, c);
>> + filter = get_bloom_filter(the_repository, c, 1);
>> print_bloom_filter(filter);
>> }
>
> I would like to see some tests, but that needs to wait for patch that
> adds --changed-paths option to the 'write' subcommand.
>
> Things to be tested:
> 1. That after reading commit-graph with Bloom filter:
> - that commit(s) in commit-graph have Bloom filter
> - that commits outside commit-graph do not have Bloom filter
> 2. That incremental commit-graph feature works:
> - for commits in deeper layer that have Bloom filter chunks
> - for commits in deeper layer that do not have Bloom filter chunks
>
Included in later commits.
> Best,
>
