From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 24 Jun 2026 13:41:12 GMT
Subject: Re: [PATCH v2 4/7] commit-reach: add trace2 instrumentation to paint_down_to_common()
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:
> 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)

> +	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.

> +
> +	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