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

[Jeff King <peff@peff.net>]

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.

E.g., imagine resolving "main" to 1234abcd in step one, then somebody
updates it to 5678cdef, then you run "for-each-ref" to compute
ahead/behind, and now you show an inconsistent result: you say that
"main" points to 1234abcd, but show the wrong ahead/behind information.

Showing 1234abcd at all is out-of-date, of course, but the real problem
is the lack of atomicity. Most porcelain scripts deal with this by
resolving the refs immediately, assuming object ids are immutable (which
they are modulo games like refs/replace), and then working with them.

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

-Peff