a8n: Display diff for changesets that exist on the codehost

[mrnugget]

This is part of https://github.com/sourcegraph/sourcegraph/issues/6085

The proposed GraphQL schema for displaying diffs for changesets that already exist on codehosts will most likely involve returning a RepositoryComparison for each changeset. See https://github.com/sourcegraph/sourcegraph/pull/5850 and https://github.com/sourcegraph/sourcegraph/pull/6135#issue-330763174

---

Backend Implementation Details

In order to implement that diff: RepositoryComparison! on the a8n.changesetResolver all that's required is — theoretically — this:

func (r *changesetResolver) Diff(ctx context.Context) (*graphqlbackend.RepositoryComparisonResolver, error) {
    repo, err := r.repoResolver(ctx)
    if err != nil {
        return nil, err
    }

    base := "master"
    head := "master"

    return graphqlbackend.NewRepositoryComparison(ctx, repo, &graphqlbackend.RepositoryComparisonInput{
        Base: &base,
        Head: &head,
    })
}

But: how do we get to base and head? How do we get the git refs?

First answer: we can ask the codehosts.

GitHub

GitHub allows us to query baseRef and headRef for each pull request. But headRef might be null. That's the case when the pull request has been merged.

There's the headRefOid field, which allows us to get to the Git Object ID even if the PR has been merged and/or the commit has been deleted.

Bitbucket SErver

Bitbucket Server pull requests have FromRef and ToRef fields. The API doesn't mention whether the FromRef is nullable or not.

Questions

Do we still want to display a diff when a changeset has been merged?

Because the next question right after "where do we get the headRef if it's been merged?" is: if the PR has been merged, are we sure that still have the commit in our gitserver copy of the repository?

I personally think that the value doesn't lie in displaying merged diff, in which case we still link to the PR on the codehost where the user can presumably still access the (cached) diff.

[mrnugget]

@sqs Can you take a look at the questions in the ticket? Namely: how important is it to display a diff once a PR has been merged?

[sqs]

Do we still want to display a diff when a changeset has been merged?

No. When viewing the campaign's combined diff, I think users want to see "what are all the outstanding [not-yet-merged] changes", not "what are all the changes [merged and not-yet-merged]". So it is actually desirable to NOT show the diffs of merged changesets.

In the future, it would not completely surprise me if users wanted to be able to see both, but it's not necessary now.

Because the next question right after "where do we get the headRef if it's been merged?" is: if the PR has been merged, are we sure that still have the commit in our gitserver copy of the repository?

It is possible (just mentioning these solutions to satisfy curiosity, since we won't actually need them):

• In GitHub, you can fetch the refs under refs/pull/, which remain even after the PR is closed or merged.
• In Bitbucket Server I believe they are under refs/pull-request/.

Fetching these refspecs would ensure the Git objects remain.

I personally think that the value doesn't lie in displaying merged diff, in which case we still link to the PR on the codehost where the user can presumably still access the (cached) diff.

Agreed.

[mrnugget]

I just picked @keegancsmith's brain about this. Here's the result.

Assuming that

1. we don't need to display diffs for merged PRs
2. we don't need to display diffs for PRs across repos (forks)

then we can just pass the headRefOid and baseRefOid to RepositoryComparison.

But if we do want to support merged PRs and forks we need to ensure that we fetch the correct refs (refs/pull/<PR_NUMBER> on GitHub and refs/pull-request/<PR_NUMBER> on Bitbucket Server).

That would require making a little change to gitserver so that we can do something like gitserver.FetchRef("refs/pull/1234") before every call to RepositoryComparison.

---

@felixfbecker Considering that it requires changes to gitserver that we don't have to make (since @sqs says we probably shouldn't display diffs for merged PRs) do you agree that we should make that diff field for changesets in #6135 nullable?

[mrnugget]

Update: from Bitbucket Server we do net get git ObjectIDs but (I assume) refs:

"fromRef": {
   "id": "refs/heads/release-testing-pr",
   "displayId": "release-testing-pr",
   "repository": {
    "slug": "vegeta",
    "project": {
     "key": "SOUR"
    }
   }
  },
  "toRef": {
   "id": "refs/heads/master",
   "displayId": "master",
   "repository": {
    "slug": "vegeta",
    "project": {
     "key": "SOUR"
    }
   }

[mrnugget]

The id we get from Bitbucket Server is enough to display a diff. I've tested it locally.

Implementation — with nullable diffs — is here: #6206