Re: [PATCH 0/8] ahead-behind: new builtin for counting multiple commit ranges

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 6, 2023, 20:18 UTC
Message-ID: <8be01cb6-8937-faed-0850-4501a2f1ef55@github.com>
In-Reply-To: <xmqqedq1ag8d.fsf@gitster.g>

On 3/6/2023 1:26 PM, Junio C Hamano wrote:

Show 31 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> These numbers can be computed by 'git rev-list --count B..C' and 'git
>> rev-list --count C..B', but there are common needs that benefit from having
>> the checks being done in the same process:
> 
> This makes readers wonder if "git rev-list --count B...C" should be
> the end-user facing UI for this new feature, perhaps?
> 
> Of course if you are checking how C0, C1, C2,... relate to a single
> B, the existing rev-list syntax would not work, and makes a totally
> new subcommand a possibilty.
> 
>>  2. When a branch is updated, a background job checks if any pull requests
>>     that target that branch should be closed because their branches were
>>     merged implicitly by that update. These queries can e batched into 'git
>>     ahead-behind' calls.
>>
>> In that second example, we don't need the full ahead/behind counts (although
>> it is sufficient to look for branches that are "zero commits ahead", meaning
>> they are reachable from the base), so this builtin has an extra '--contains'
>> mode that only checks reachability from the base to each of the tips. 'git
>> ahead-behind --contains' is sort of the reverse of 'git branch --contains'.
> 
> I thought that the reverse of "git branch --contains" was "git
> branch --merged".  "git branch --merged maint ??/\*" is how I cull
> topic branches that have already served their purpose.  
> 
> Isn't closing pull requests because they have been already merged
> the same idea?  "git for-each-ref --merged main refs/pull/\*" or
> something, perhaps?

You are definitely on to something, and I was not aware of --merged as an option to either of these.

'git branch --merged' has some limitations that tags cannot be used.

'git for-each-ref --merged' is probably sufficient. The only difference being that it would be nice to specify the matching refs over stdin with --stdin to avoid long argument lists.

With this in mind, I can update the performance test to look like this (after updating the setup step to add branches for each line in 'refs')

test_perf 'batch reachability: git ahead-behind --contains' '
	git ahead-behind --contains --base=HEAD --stdin <refs
'

test_perf 'batch reachability: git branch --merged' '
	xargs git branch --merged=HEAD <branches
'

test_perf 'batch reachability: git for-each-ref --merged' '
	xargs git for-each-ref --merged=HEAD <refs
'

And get decent results on all cases with the Linux kernel repository:

Test this tree ------------------------------------------------------------------------- 1500.2: ahead-behind counts: git ahead-behind 0.26(0.24+0.01) 1500.3: ahead-behind counts: git rev-list 4.46(3.91+0.54) 1500.4: batch reachability: git ahead-behind --contains 0.02(0.01+0.01) 1500.5: batch reachability: git branch --merged 0.14(0.13+0.00) 1500.6: batch reachability: git for-each-ref --merged 0.14(0.13+0.00)

So, there is benefit in using this tips_reachable_from_base() method in the two existing 'git (branch|for-each-ref) --merged' computations. The API boundary selected in this series might not be the most appropriate for those builtins, so let's kick out patch 8 from this series for now and I'll revisit it separately.

Thanks, -Stolee