Re: [PATCH v2 4/7] commit-reach: add trace2 instrumentation to paint_down_to_common()

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 24, 2026, 13:41 UTC
Message-ID: <560c91df-3c07-4c8f-9924-ef0cc7646e08@gmail.com>
In-Reply-To: <6ade4df2ed2a836a3b4c5400ab13e8247e36c029.1782303254.git.gitgitgadget@gmail.com>

On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote:

Show 6 quoted lines

> From: Kristofer Karlsson <krka@spotify.com>
> 
> Add a step counter and trace2_data_intmax() call so that the number
> of commits visited during the paint walk is observable via
> GIT_TRACE2_PERF. This provides a way to measure the impact of
> future optimizations without relying on wall-clock benchmarks alone.

> +	trace2_data_intmax("paint_down_to_common", r,
> +			   "steps", steps);

This is great data. Very clearly marked for what we should be doing here.

> +test_expect_success 'merge-base --all commit-walk steps' '
> +	test_when_finished rm -rf .git/objects/info/commit-graph \
> +		.git/objects/info/commit-graphs &&

(highlighting this chunk)

Show 6 quoted lines

> +	rm -rf .git/objects/info/commit-graph \
> +		.git/objects/info/commit-graphs &&
> +
> +	GIT_TRACE2_EVENT="$(pwd)/trace-none.txt" \
> +		git merge-base --all commit-9-9 commit-9-1 >actual &&
> +	test_trace2_data paint_down_to_common steps 81 <trace-none.txt &&

I'd rather see the whitespace line before the `rm` to make it more obvious that it's setting up the "none" case.

Show 12 quoted lines

> +
> +	cp commit-graph-full .git/objects/info/commit-graph &&
> +	GIT_TRACE2_EVENT="$(pwd)/trace-full.txt" \
> +		git merge-base --all commit-9-9 commit-9-1 >actual &&
> +	test_trace2_data paint_down_to_common steps 80 <trace-full.txt &&
> +
> +	cp commit-graph-half .git/objects/info/commit-graph &&
> +	GIT_TRACE2_EVENT="$(pwd)/trace-half.txt" \
> +		git merge-base --all commit-9-9 commit-9-1 >actual &&
> +	test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> +'
> +

This test is a great example. I look forward to seeing that it updates in the future.

One thing I was hoping to see was that your side-exhaustion tests (from patch v2 2/7) would also include these checks so they are more obviously updating when the implementation updates later.

One way to accomplish that is to reorder this patch before adding those tests so their first version includes these checks and then the values update when changing the implementation.

Thanks, -Stolee