This is cool! Should we support git rev-parse syntax like git show 'HEAD@{3 months ago}' here? rev:"{3 months ago}"? It looks weird but maybe that could be more standard for people familiar with that syntax.

One thing that is weird about the above ref syntax is that git uses reflog for determining these dates, not the commit/author date, which camden is using in this PR (and intuitively makes more sense IMO).

So the above syntax that you provided means "HEAD as it was on Sourcegraph 3 months ago".
Here's that for a freshly cloned repo:

git rev-parse 'HEAD@{3 days ago}'
warning: log for 'HEAD' only goes back to Wed, 3 Apr 2024 00:16:15 +0200
9761f2597af73fa2db530f0afc3c7258538c9dd2

Yep what Erik said. I agree the syntax is more intuitive, but rev@{3 days ago} has meaning to git that is different than what we're doing here (and doesn't actually make much sense in the context of a headless repo), so I don't know that we want to reuse the syntax exactly. Maybe we should use that syntax as inspiration though and pick something closer to that, e.g. repo:myrepo@HEAD%{5 days} or something. Will noodle on that.

This looks really cool! I would probably abuse it for bisect searches (unless there's something better for this).

My only concern is around performance. You mentioned a fast 50ms query. Do you know if there's an upper bound to how long this could take, or could it become increasingly slower as repos get larger and commit histories longer? Mainly concerned about noisy neighbor problems, esp. related to automated queries (e.g. from code insights).

timezones for non-relative time queries

Yeah, that's tricky. We run into this with before and after too. The server uses UTC, but users expect relative days to mean local time. There's some more discussion here, along with an idea to provide a hover with the resolved date (which I think is a good idea). I personally don't think we should interpret queries differently for different users just because of the sharability aspect.

It feels like we should stick to a cut vertex in the commit graph.

I would generally agree, but there's not really an easy way to do this without abandoning the git CLI entirely AFAIK. The exception to that being if we use --first-parent, the graph is linear, so every vertex is a cut vertex.

The downside of --first-parent is it skips the history of all merged branches. So for places that heavily use merge commits, the granularity of the search will be at the "PR" level rather than at the commit level, which can be quite meaningful for long-lived branches. I'm starting to be convinced that this is a worthwhile tradeoff though, and that the simplicity of --first-parent outweights the loss of granularity.

how could this conflict with actual rev names?

The predicate parser only looks for things of the shape rev:ancestor.at([^)]*). I expect it to be vanishingly unlikely that it would conflict, and in the case that it does, we support quoting values like rev:"ancestor.at(abc)" (at least on paper...I haven't actually tested this in a while).

concern around performance

My thought here is that perf cost will be dominated by actually doing the unindexed searches. That's part is no different than a search context that contains a bunch of unindexed revs. Comparatively, the only added cost is the commit resolution, which is non-zero, but not massive either.

As a data point, the chromium repo (largest I have locally with 1.2M commits) takes ~12 seconds to traverse its entire history. It takes 100ms with --before="1 year ago". That seems like an acceptable perf tradeoff to me for a somewhat niche feature.

And in the case perf does become a significant concern, the work the Source team has done to virtualize git calls means we could pretty easily add a cache or index to this specific lookup.

The proposed behavior makes sense to me. +1 to using --first-parent, this feels easiest to understand for users.

My thought here is that perf cost will be dominated by actually doing the unindexed searches.

Now that we use hybrid search by default (yay!) many unindexed searches are very fast. But as you say, there's a lot we can do here -- for example a simple cache of time expression -> hash could really help. Also I assume the command scales logarithmically with history length (not linear) so there's not a huge concern with large histories?

Personally I find the naming a bit surprising because I don't think about "git ancestors" in my day to day dev work :) Throwing out some other ideas:

• rev:commit.at(...)
• rev:at.time(...) -> this feels consistent with the format we use for others like repo:has.topic

Also I assume the command scales logarithmically with history length (not linear) so there's not a huge concern with large histories?

It does not. Git cannot assume a binary search is safe because commit dates are not guaranteed to be monotonic. So it has to iterate commit-by-commit. (I'm also not even sure Git has the indexes that would be needed to do a binary search, but that's another matter entirely)

Personally I find the naming a bit surprising

Thank you! I also dislike the naming, so I really appreciate the suggestions.

For the examples you gave, do they still make sense to you for non-HEAD commits? I read rev:at.time(yesterday) to mean "HEAD at yesterday", which I think is a good default. But what about if I wanted to search the history of other branches? Does rev:at.time(my-branch, yesterday) make sense? Or maybe rev:at.time(yesterday, my-branch) to put the optional param last? Obviously, we can document this all, but an intuitive syntax is preferable 🙂

Or maybe we could even get funky and allow multiple predicates like rev:branch(my-branch):at.time(yesterday) or just rev:my-branch:at.time(yesterday)

the command scales logarithmically

To back up my claims, I measured a few data points on chromium/chromium locally. Looks pretty linear to me. Looks like we can go through ~1M commits/second

@camdencheek I like rev:at.time(yesterday, my-branch) with branch being optional.

It does not. Git cannot assume a binary search is safe because commit dates are not guaranteed to be monotonic.

Oof, I see! Then a cache feels really valuable, even in the initial implementation, as a way of reducing load and making sure follow-up queries are fast.

Updates from feedback:

• I added --first-parent to linearize the history and make it easier to explain
• I updated the syntax to rev:at.time(yesterday) or rev:at.time(yesterday, my-branch). Along with that, I've renamed all the backend pieces to match the "rev at time" naming.
• I added a simple LRU cache to the commit resolution to mitigate the cost of repeated, expensive git commands

Since there seems to be general agreement on direction, I'll work towards productionizing this PR (add tests mostly). Feel free to add more feedback in the mean time, but I'll explicitly request a review once it's ready for full code review

Okay! I think this is ready for a full code review now. I've added tests and query completions/hovers since the last update.

📖 Storybook live preview