Skip to content

Ensure revision when calculating changeset state

Warren Gifford requested to merge mrn/ensure-rev-before-diffstat into main

Created by: mrnugget

This fixes #14921 (closed) by making sure that the HeadRev is fetched on gitserver before attempting to get the git diff and calculating the diff stat.

There were other options to do this:

  1. Adding an git.EnsureRevision call to the reconciler when importing changesets
  2. Adding git.EnsureRevision to computeDiffStat

Why not (1)? It adds a special case to one method, when in fact we should always ensure that the head rev is fetched on gitserver before calculating diff stats.

But adding two git.EnsureRevision to (2) would've duplicated the already existing call to git.EnsureRevision that we did in computeRev.

The problem was that we only called out to EnsureRevision in computeRev when we didn't have the OID.

When importing changesets, though, we do have the OID, we just need to ensure it exists on gitserver.

So I added those calls to computeRev, which, I think, makes sense there, since it means we don't work with possibly outdated data, since we always ensure the newest commit is used.

I also had to fix computeGitserverRepo which was simply using the external URL as the clone URL when constructing the gitserver.Repo. backend.GitRepo does the correct thing.


Trivia

This is the test script I used to push a new commit and cause the reconciler to import the changeset, failing on

$ psql --dbname sourcegraph -c 'DELETE FROM changeset_events; DELETE FROM CHANGESETS; DELETE FROM campaigns;' && newCommit 3 && src campaign apply -f ~/tmp/wonky-diffstat.campaign.yaml -n mrnugget

Requirements:

$ newCommit() { echo "foo" >> foo${1}.txt && git add . && gc -m "foo${2}" && up }
$ cd automation-testing && git checkout thorsten/detach-reattach

And the campaign is this one:

name: debugging-1
description: hopefully fixing a bug

importChangesets:
  - repository: github.com/sourcegraph/automation-testing
    # https://github.com/sourcegraph/automation-testing/pull/357
    externalIDs: [357]

Merge request reports

Loading