git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 12/12] builtin/commit-graph.c: introduce '--max-new-filters=<n>'

From
Derrick Stolee <stolee@gmail.com>
Date
Sep 14, 2020, 20:31 UTC
Message-ID
<134d64a0-abb6-bdc9-2c05-7aded01a906a@gmail.com>
In-Reply-To
<20200914201258.GA12431@nand.local>
On 9/14/2020 4:12 PM, Taylor Blau wrote:
Show 29 quoted lines
> On Fri, Sep 11, 2020 at 03:25:55PM -0400, Taylor Blau wrote:
>> On Fri, Sep 11, 2020 at 02:59:34PM -0400, Taylor Blau wrote:
>>> On Fri, Sep 11, 2020 at 01:52:16PM -0400, Jeff King wrote:
>>>> On Wed, Sep 09, 2020 at 11:24:00AM -0400, Taylor Blau wrote:
>>>>> +With the `--max-new-filters=<n>` option, generate at most `n` new Bloom
>>>>> +filters (if `--changed-paths` is specified). If `n` is `-1`, no limit is
>>>>> +enforced. Commits whose filters are not calculated are stored as a
>>>>> +length zero Bloom filter, and their bit is marked in the `BFXL` chunk.
>>>>> +Overrides the `commitGraph.maxNewFilters` configuration.
>>>>
>>>> The BFXL chunk doesn't exist anymore in this iteration, right?
>>>
>>> Ack; I'll have to drop that.
>>
>> Junio, I know that I've already sent one replacement patch. If you don't
>> mind, here's another (and if you do mind, I'm happy to re-roll the
>> series).
> 
> Just kidding. Let's use *this* version which fixes a bug reading the
> commitGraph.maxNewFilters configuration. At this point, the
> fix-ups are:
> 
>   - This patch (attached below the scisors) instead of 12/12, and
> 
>   - This [1] patch instead of 10/12.
> 
> [1]: https://lore.kernel.org/git/20200910154516.GA32117@nand.local/
> 
> Let me know if you'd rather have a full re-roll.

It's getting a bit difficult to track all of these "use this instead" patches. But, I'm not the one applying them, so maybe that's not actually a problem.

You might need a re-roll, anyway, as I have a few comments here:
> --- 8< ---
> 
> Subject: [PATCH] builtin/commit-graph.c: introduce '--max-new-filters=<n>'

You also introduce commitGraph.maxNewFitlers here, which is not mentioned in the commit message anywhere. In fact, it might be good to include it as a separate patch so its implementation and tests can be isolated from the command-line functionality.

> +length zero Bloom filter. Overrides the `commitGraph.maxNewFilters`
> +configuration.

We have found it valuable to demonstrate these overrides in tests. Let's inspect your tests for this.

Show 9 quoted lines
> +test_bloom_filters_computed () {
> +	commit_graph_args=$1
> +	rm -f "$TRASH_DIRECTORY/trace.event" &&
> +	GIT_TRACE2_EVENT="$TRASH_DIRECTORY/trace.event" git commit-graph write \
> +		$commit_graph_args &&
> +	grep "\"filter_not_computed\":$2" "$TRASH_DIRECTORY/trace.event" &&
> +	grep "\"filter_trunc_large\":$3" "$TRASH_DIRECTORY/trace.event" &&
> +	grep "\"filter_computed\":$4" "$TRASH_DIRECTORY/trace.event"
> +}

If the arguments were moved to the last parameter, then we could do a few interesting things here.

test_bloom_filters_computed () {
	NOT_COMPUTED="\"filter_not_computed\":$1" &&
	shift &&
	TRUNCATED="\"filter_trunc_large\":$1" &&
	shift &&
	COMPUTED="\"filter_computed\":$1" &&
	shift &&
	rm -f "$TRASH_DIRECTORY/trace.event" &&
	GIT_TRACE2_EVENT="$TRASH_DIRECTORY/trace.event" git commit-graph write $@ &&
	grep "$NOT_COMPUTED" "$TRASH_DIRECTORY/trace.event" &&
	grep "$TRUNCATED" "$TRASH_DIRECTORY/trace.event" &&
	grep "$COMPUTED" "$TRASH_DIRECTORY/trace.event"
}

(I have not tested this script. It might need some work.) This would make your callers a bit cleaner-looking, for example:

test_expect_success 'Bloom generation is limited by --max-new-filters' '
	(
		cd limits &&
		test_commit c2 filter &&
		test_commit c3 filter &&
		test_commit c4 no-filter &&
		test_bloom_filters_computed 3 0 2 \
			--reachable --changed-paths --split=replace --max-new-filters=2
	)
'
At least, this looks nicer to me.
Show 9 quoted lines
> +test_expect_success 'Bloom generation backfills previously-skipped filters' '
> +	# Check specifying commitGraph.maxNewFilters over "git config" works.
> +	test_config -C limits commitGraph.maxNewFilters 1 &&
> +	(
> +		cd limits &&
> +		test_bloom_filters_computed "--reachable --changed-paths --split=replace" \
> +			4 0 1
> +	)
> +'

Adding a case for `commitGraph.maxNewFilters=1` and `--max-new-filters=2` might be interesting for the override rules.

Show 18 quoted lines
> +
> +test_expect_success 'Bloom generation backfills empty commits' '
> +	git init empty &&
> +	test_when_finished "rm -fr empty" &&
> +	(
> +		cd empty &&
> +		for i in $(test_seq 1 6)
> +		do
> +			git commit --allow-empty -m "$i"
> +		done &&
> +
> +		# Generate Bloom filters for empty commits 1-6, two at a time.
> +		test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \
> +			4 0 2 &&
> +		test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \
> +			4 0 2 &&
> +		test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \
> +			4 0 2 &&

I'm concerned that the max-new-filters limit (2) is a divisor of the full number of commits (6). It might be good to add one more commit here and test again with a limit of 2. That would handle both "equal to limit" and "less than limit" cases.

Show 6 quoted lines
> +		# Finally, make sure that once all commits have filters, that
> +		# none are subsequently recomputed.
> +		test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \
> +			6 0 0
> +	)
> +'

Thanks, -Stolee

Previous: Taylor BlauNext: Taylor Blau
Message 9 of 75 in “more miscellaneous Bloom filter improvements, redux”
  1. 00/12 more miscellaneous Bloom filter improvements, reduxTaylor Blau, Sep 9, 2020
  2. 03/12 commit-graph: pass a 'struct repository *' in more placesTaylor Blau, Sep 9, 2020
  3. 11/12 commit-graph: rename 'split_commit_graph_opts'Taylor Blau, Sep 9, 2020
  4. 12/12 builtin/commit-graph.c: introduce '--max-new-filters=<n>'Taylor Blau, Sep 9, 2020
  5. Jeff KingSep 11, 2020
  6. Taylor BlauSep 11, 2020
  7. Taylor BlauSep 11, 2020
  8. Taylor BlauSep 14, 2020
  9. Derrick StoleeSep 14, 2020
  10. Taylor BlauSep 14, 2020
  11. Derrick StoleeSep 15, 2020
  12. Taylor BlauSep 15, 2020
  13. Junio C HamanoSep 15, 2020
  14. Taylor BlauSep 15, 2020
  15. Jeff KingSep 11, 2020
  16. Junio C HamanoSep 11, 2020
  17. 04/12 t/helper/test-read-graph.c: prepare repo settingsTaylor Blau, Sep 9, 2020
  18. 05/12 commit-graph: respect 'commitGraph.readChangedPaths'Taylor Blau, Sep 9, 2020
  19. 08/12 bloom: use provided 'struct bloom_filter_settings'Taylor Blau, Sep 9, 2020
  20. 06/12 commit-graph.c: store maximum changed pathsTaylor Blau, Sep 9, 2020
  21. 09/12 bloom/diff: properly short-circuit on max_changesTaylor Blau, Sep 9, 2020
  22. 10/12 bloom: encode out-of-bounds filters as non-emptyTaylor Blau, Sep 9, 2020
  23. Taylor BlauSep 10, 2020
  24. Taylor BlauSep 10, 2020
  25. Derrick StoleeSep 11, 2020
  26. 07/12 bloom: split 'get_bloom_filter()' in twoTaylor Blau, Sep 9, 2020
  27. 02/12 t4216: use an '&&'-chainTaylor Blau, Sep 9, 2020
  28. 01/12 commit-graph: introduce 'get_bloom_filter_settings()'Taylor Blau, Sep 9, 2020
  29. 00/13 more miscellaneous Bloom filter improvements, reduxTaylor Blau, Sep 16, 2020
  30. 04/13 t/helper/test-read-graph.c: prepare repo settingsTaylor Blau, Sep 16, 2020
  31. 03/13 commit-graph: pass a 'struct repository *' in more placesTaylor Blau, Sep 16, 2020
  32. 05/13 commit-graph: respect 'commitGraph.readChangedPaths'Taylor Blau, Sep 16, 2020
  33. 06/13 commit-graph.c: store maximum changed pathsTaylor Blau, Sep 16, 2020
  34. 02/13 t4216: use an '&&'-chainTaylor Blau, Sep 16, 2020
  35. 01/13 commit-graph: introduce 'get_bloom_filter_settings()'Taylor Blau, Sep 16, 2020
  36. 07/13 bloom: split 'get_bloom_filter()' in twoTaylor Blau, Sep 16, 2020
  37. 08/13 bloom: use provided 'struct bloom_filter_settings'Taylor Blau, Sep 16, 2020
  38. 10/13 bloom: encode out-of-bounds filters as non-emptyTaylor Blau, Sep 16, 2020
  39. SZEDER GáborSep 17, 2020
  40. Taylor BlauSep 17, 2020
  41. Junio C HamanoSep 18, 2020
  42. Taylor BlauSep 18, 2020
  43. 11/13 commit-graph: rename 'split_commit_graph_opts'Taylor Blau, Sep 16, 2020
  44. 12/13 builtin/commit-graph.c: introduce '--max-new-filters=<n>'Taylor Blau, Sep 16, 2020
  45. SZEDER GáborSep 18, 2020
  46. Taylor BlauSep 18, 2020
  47. 09/13 bloom/diff: properly short-circuit on max_changesTaylor Blau, Sep 16, 2020
  48. 13/13 commit-graph: introduce 'commitGraph.maxNewFilters'Taylor Blau, Sep 16, 2020
  49. Derrick StoleeSep 16, 2020
  50. Junio C HamanoSep 16, 2020
  51. Taylor BlauSep 17, 2020
  52. Junio C HamanoSep 17, 2020
  53. Taylor BlauSep 17, 2020
  54. Taylor BlauSep 17, 2020
  55. Derrick StoleeSep 17, 2020
  56. 00/13 more miscellaneous Bloom filter improvements, reduxTaylor Blau, Sep 18, 2020
  57. 01/13 commit-graph: introduce 'get_bloom_filter_settings()'Taylor Blau, Sep 18, 2020
  58. 03/13 commit-graph: pass a 'struct repository *' in more placesTaylor Blau, Sep 18, 2020
  59. 02/13 t4216: use an '&&'-chainTaylor Blau, Sep 18, 2020
  60. 05/13 commit-graph: respect 'commitGraph.readChangedPaths'Taylor Blau, Sep 18, 2020
  61. 04/13 t/helper/test-read-graph.c: prepare repo settingsTaylor Blau, Sep 18, 2020
  62. 08/13 bloom: use provided 'struct bloom_filter_settings'Taylor Blau, Sep 18, 2020
  63. SZEDER GáborSep 18, 2020
  64. Taylor BlauSep 18, 2020
  65. 07/13 bloom: split 'get_bloom_filter()' in twoTaylor Blau, Sep 18, 2020
  66. 06/13 commit-graph.c: store maximum changed pathsTaylor Blau, Sep 18, 2020
  67. 10/13 bloom: encode out-of-bounds filters as non-emptyTaylor Blau, Sep 18, 2020
  68. 11/13 commit-graph: rename 'split_commit_graph_opts'Taylor Blau, Sep 18, 2020
  69. 13/13 commit-graph: introduce 'commitGraph.maxNewFilters'Taylor Blau, Sep 18, 2020
  70. Taylor BlauSep 18, 2020
  71. Junio C HamanoSep 18, 2020
  72. 12/13 builtin/commit-graph.c: introduce '--max-new-filters=<n>'Taylor Blau, Sep 18, 2020
  73. 09/13 bloom/diff: properly short-circuit on max_changesTaylor Blau, Sep 18, 2020
  74. Taylor BlauSep 18, 2020
  75. Taylor BlauSep 18, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.