Skip to content

Refactor Phabricator backend and file_info code

Warren Gifford requested to merge phabricator-base-commit into master

Created by: lguychard

Fixes https://github.com/sourcegraph/sourcegraph/issues/3823, https://github.com/sourcegraph/sourcegraph/issues/5051

This took me a good minute to get to a PRable state, and I'm sorry it's such a mammoth PR 😞

Notable changes:

  • Phabricator FileInfo resolvers no longer return rev names. This was the source of many bugs, as returning the name of a rev that only exists on a staging repo, and was auto-generated by arc, doesn't make sense when the staging repo is not synced to the Sourcegraph instance. The main drawbacks are that the OpenDiffOnSourcegraph links will always contain commit IDs and not rev names, and that the workspace roots exposed to extensions will always have undefined inputRevisions. I think this is an acceptable tradeoff considering it a) gets rid of bugs and b) gets rid of a lot of hacky code like this (this is just one example): https://sourcegraph.com/github.com/sourcegraph/sourcegraph@a3add8a6499095ad4b62536e6b37a7ccf661c449/-/blob/browser/src/libs/phabricator/util.tsx#L354-359
  • Functions that queried the Phabricator Conduit API have been DRYed, now use rxjs.ajax() instead of fetch(), and QueryConduitHelper is now injected so that it can be mocked in tests. This allowed me to add tests for all fileInfo resolvers by running them on page snapshots, and providing mock Sourcegraph and Phabricator API responses.
  • Terminology:
    • differentialID -> revisionID: in Differential, a set of changes up for review is a revision, which is composed of one or more diffs (patches).
    • leftDiffID becomes baseDiffID, to clarify the use case of comparing two diffs in Phabricator. This is the best I could come up with, I explained it in comments in the code.
  • getPhabricatorState() now returns a properly discriminated union. This allowed me to find errors where we were building incorrect states.
  • FileInfo resolvers now throw an error if they receive an unexpected state (instead of silently never emitting).
  • Many scraping functions now throw precise errors as early as possible instead of doing a console.error() or nothing at all and then returning null.

Test plan:

  • New unit tests for all FileInfo resolvers.
  • I have done some manual testing of all permutations I added as unit tests.
  • I'm considering adding some e2e tests as well, but I will do this in a follow-up PR.

Merge request reports