# [PATCH] commit-reach: stop sorting in paint_down_to_common() 4 messages from 2026-05-27 to 2026-05-29. Participants: René Scharfe, Jeff King. Thread: https://gitlist.dev/t/65701 ## René Scharfe, 2026-05-27 15:52 Subject: [PATCH] commit-reach: stop sorting in paint_down_to_common() Message-ID: <450163b0-82c8-4b57-baab-a269efe430aa@web.de> ``` None of the three callers of paint_down_to_common() care about the order of its result list: merge_bases_many() sorts it again after removing stale items, remove_redundant_no_gen() and repo_in_merge_bases_many() throw the list away without even looking at it. So drop the unnecessary commit_list_sort_by_date() call. Signed-off-by: René Scharfe --- commit-reach.c | 1 - 1 file changed, 1 deletion(-) diff --git a/commit-reach.c b/commit-reach.c index 5a52be90a6..056a7ed8d8 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -137,7 +137,6 @@ static int paint_down_to_common(struct repository *r, } clear_prio_queue(&queue); - commit_list_sort_by_date(result); return 0; } -- 2.54.0 ``` ## Jeff King, 2026-05-29 08:43 Subject: Re: [PATCH] commit-reach: stop sorting in paint_down_to_common() Message-ID: <20260529084325.GF1106035@coredump.intra.peff.net> In-Reply-To: <450163b0-82c8-4b57-baab-a269efe430aa@web.de> ``` On Wed, May 27, 2026 at 05:52:17PM +0200, René Scharfe wrote: > None of the three callers of paint_down_to_common() care about the order > of its result list: merge_bases_many() sorts it again after removing > stale items, remove_redundant_no_gen() and repo_in_merge_bases_many() > throw the list away without even looking at it. So drop the unnecessary > commit_list_sort_by_date() call. Seems like an easy win. If some of the callers do not even look at the result, could we avoid building it at all in those cases (e.g., by passing in a NULL result pointer)? I guess there is not much to be gained, though. The result is a list of merge bases, so it should usually be rather small. The benefit in your patch is probably not performance, but just reducing the size of the code. -Peff ``` ## René Scharfe, 2026-05-29 15:32 Subject: Re: [PATCH] commit-reach: stop sorting in paint_down_to_common() Message-ID: <107314f5-0057-4ed3-9bee-9dca4f424bd1@web.de> In-Reply-To: <20260529084325.GF1106035@coredump.intra.peff.net> ``` On 5/29/26 10:43 AM, Jeff King wrote: > On Wed, May 27, 2026 at 05:52:17PM +0200, René Scharfe wrote: > >> None of the three callers of paint_down_to_common() care about the order >> of its result list: merge_bases_many() sorts it again after removing >> stale items, remove_redundant_no_gen() and repo_in_merge_bases_many() >> throw the list away without even looking at it. So drop the unnecessary >> commit_list_sort_by_date() call. > > Seems like an easy win. If some of the callers do not even look at the > result, could we avoid building it at all in those cases (e.g., by > passing in a NULL result pointer)? Yes, at the cost of adding NULL checks to paint_down_to_common(). Which is probably worth it. > I guess there is not much to be gained, though. The result is a list of > merge bases, so it should usually be rather small. The benefit in your > patch is probably not performance, but just reducing the size of the > code. True. The list can be arbitrarily long, but should only contain a handful commits in normal repos. René ``` ## Jeff King, 2026-05-29 19:04 Subject: Re: [PATCH] commit-reach: stop sorting in paint_down_to_common() Message-ID: <20260529190453.GA1711766@coredump.intra.peff.net> In-Reply-To: <107314f5-0057-4ed3-9bee-9dca4f424bd1@web.de> ``` On Fri, May 29, 2026 at 05:32:58PM +0200, René Scharfe wrote: > > Seems like an easy win. If some of the callers do not even look at the > > result, could we avoid building it at all in those cases (e.g., by > > passing in a NULL result pointer)? > > Yes, at the cost of adding NULL checks to paint_down_to_common(). Which > is probably worth it. Yeah. I thought it was only one line (where we append to "tail"), but there are a couple spots where "result" is referenced directly. Another fun fact: it looks like paint_down_to_common() makes sure to clean up the output list before returning an error. But the callers do so, too, which is redundant. That would go away if they just passed in NULL. :) -Peff ```