Re: [PATCH v2 0/8] ref-filter: ahead/behind counting, faster --merged option

[Derrick Stolee <derrickstolee@github.com>]

On 3/15/2023 1:31 PM, Jeff King wrote:
> On Fri, Mar 10, 2023 at 02:25:52PM -0500, Derrick Stolee wrote:
> 
>>> Having read all the patches, I am very impressed and pleased, but
>>> are we losing anything by having the feature inside for-each-ref
>>> compared to a new command ahead-behind?  As far as I can tell, the
>>> new "for-each-ref --stdin" would still want to match refs and work
>>> only on refs, but there shouldn't be any reason for ahead-behind
>>> computation to limit to tips that are at the tip of a ref, so that
>>> may be one downside in this updated design.  For the intended use
>>> case of "let's find which branches are stale", that downside does
>>> not matter in practice, but for other use cases people will think
>>> of in the future, the limitation might matter (at which time we can
>>> easily resurrect the other subcommand, using the internal machinery
>>> we have here, so it is not a huge deal, I presume).
>>
>> I think the for-each-ref implementation solves the use case we
>> had in mind, I think. I'll double-check to see if we ever use
>> exact commit IDs instead of reference names, but I think these
>> callers are rarely interested in an exact commit ID but instead
>> want the latest version of refs.
> 
> One thing I'd worry about here are race conditions.
> 
> If you have a porcelain-ish view (and I'd count "showing a web page" as
> a porcelain view) that requires several commands to compute, it's
> possible for there to be simultaneous ref updates between your commands.
> If each command is given a refname, then the results may not be
> consistent.

> I don't know if this is how your current application-level code calling
> ahead-behind works, or if it just accepts the possibility of a race (or
> maybe the call is not presented along with other information so it's
> sort-of atomic on its own). Presumably your double-checking will find
> out. :)

I completely agree on both of these points.

The major lift in this series is that the two commit walk algorithms
are being contributed to the core project in a way that are easy to
modify our 'git ahead-behind' builtin to use the "new" internals
without any UX change. Actually porting the application layer to use
'git for-each-ref' instead would be a second step, where I'd plan to
do this deep dive. From what I understand, though, these race conditions
do exist already, but they are minor relative to the cost of doing a
lookup of all the ref values and then calling this backend.
 
> I do otherwise like exposing this as an option of for-each-ref, as that
> is the way I'd expect most normal client users to want to get at the
> information. And if this is step 1 and that's good enough for now, and
> we have a path forward to later expose it for general commits, that's OK
> with me.

And if we truly need more general committish inputs for tips (HEAD~10,
too), then a new builtin could be built on top. Modeling it after
for-each-ref (for-each-commit?) would be a good start to make the
behavior as similar as possible. Doing that in full generality might
require strange updates to ref-filter.[c|h], but we can cross that
bridge when we come to it.

Thanks,
-Stolee