Skip to content

gitCommitResolver should not do lazy OID resolution

Created by: slimsag

Because Zoekt-produced file matches do not contain a commit ID, this is an empty string for Zoekt search results:

https://github.com/sourcegraph/sourcegraph/blob/a464200809bd8d79ef843205084f5d95b1087bfe/cmd/frontend/graphqlbackend/textsearch.go#L81

Due to this you could not fetch information about the Git commit for Zoekt-produced file match search results, as documented:

https://github.com/sourcegraph/sourcegraph/blob/a464200809bd8d79ef843205084f5d95b1087bfe/cmd/frontend/graphqlbackend/textsearch.go#L75-L77

Because customers wanted to do this via GraphQL however, we began supporting it by lazily resolving the Git commit OID when it is an empty string: https://github.com/sourcegraph/sourcegraph/pull/2318

This is not a good change for several reasons:

  1. It was the source of a bad perf regression in v3.2.0: https://github.com/sourcegraph/sourcegraph/pull/3685
  2. Due to the frequency at which it is called, and variety of circumstances under which it is called, it is difficult to make changes to that resolution logic without introducing perf regressions. e.g. see misconception about removing sync.Once logic here: https://github.com/sourcegraph/sourcegraph/issues/3746
  3. The "lookup zoekt-indexed revision if there is none" behavior is not even always desirable, as noted by @keegancsmith and @ijsnow in https://github.com/sourcegraph/sourcegraph/pull/2318#issuecomment-468403967
  4. There is a race condition where the commit is inaccurate: Zoekt produces the results from master revisionA, then indexes master revisionB and we tell callers of OID the results were from master revisionB incorrectly.

What we should do:

  1. Effectively revert back to the state we were in before https://github.com/sourcegraph/sourcegraph/pull/2318 and https://github.com/sourcegraph/sourcegraph/pull/3685 were merged, without doing on-the-fly OID resolution.
  2. Refactor Zoekt codepaths so that when it produces fileMatchResolvers, they contain the commit ID that the result came from. Zoekt has this information already and can just pass it down to us, sparing us from major complexity and the need for many caches.