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
