Page MenuHomePhabricator
Differential D19958 Diff 47643 src/applications/diffusion/controller/DiffusionBlameController.php

Changeset View

Standalone View

View Options

src/applications/diffusion/controller/DiffusionBlameController.php

Show All 19 Lines public function handleRequest(AphrontRequest $request) {
$identifiers = array_fuse($blame); $identifiers = array_fuse($blame);
if ($identifiers) { if ($identifiers) {
$commits = id(new DiffusionCommitQuery()) $commits = id(new DiffusionCommitQuery())
->setViewer($viewer) ->setViewer($viewer)
->withRepository($repository) ->withRepository($repository)
->withIdentifiers($identifiers) ->withIdentifiers($identifiers)
->needIdentities(true) ->needIdentities(true)
// See PHI1014. If identities haven't been built yet, we may need to
// fall back to raw commit data.
->needCommitData(true)
amckinleyUnsubmitted
Not Done
Inline Actions

Couldn't this still result in a race? Or do we write to repository_commit and repository_commitdata inside a transaction?

amckinley: Couldn't this still result in a race? Or do we write to `repository_commit` and…
epriestleyAuthorUnsubmitted
Done
Inline Actions

We can still race and read the Commit before it has CommitData. For most other objects, we'd fail the load and end up in trouble again.

However, in the special case of Commit, if we ask for CommitData and it doesn't exist yet, we get a dummy/empty one instead of a "you didn't ask for commit data but are trying to use it now" exception, specifically because stuff can race the creation of CommitData.

I didn't actually verify that an empty/default CommitData doesn't hit some other error, but from inspection it looks like it will be okay (we'll just end up with a null authorPHID, which the code should probably handle anyway).

Specifically, in CommitQuery near line 400, we do this:

if (!$commit_data) {
  $commit_data = new PhabricatorRepositoryCommitData();
}

In most other applications, that would be:

if (!$commit_data) {
  $this->didRejectResult($commit);
  unset($commits[$key]);
  continue;
}

...to drop the result.

epriestley: We can still race and read the `Commit` before it has `CommitData`. For most other objects…
->execute(); ->execute();
$commits = mpull($commits, null, 'getCommitIdentifier'); $commits = mpull($commits, null, 'getCommitIdentifier');
} else { } else {
$commits = array(); $commits = array();
} }
$commit_map = mpull($commits, 'getCommitIdentifier', 'getPHID'); $commit_map = mpull($commits, 'getCommitIdentifier', 'getPHID');
▲ Show 20 Lines • Show All 246 Lines • Show Last 20 Lines
Phabricator · Built by Phacility