Page MenuHomePhabricator

Make blame UI recover gracefully if Identities haven't been built yet for a commit
ClosedPublic

Authored by epriestley on Jan 4 2019, 10:14 PM.
Tags
None
Referenced Files
F14096583: D19958.diff
Tue, Nov 26, 4:13 AM
Unknown Object (File)
Thu, Oct 31, 6:04 AM
Unknown Object (File)
Oct 22 2024, 9:32 PM
Unknown Object (File)
Oct 22 2024, 1:45 PM
Unknown Object (File)
Oct 14 2024, 5:17 AM
Unknown Object (File)
Oct 9 2024, 8:25 AM
Unknown Object (File)
Sep 4 2024, 12:12 PM
Unknown Object (File)
Aug 31 2024, 7:54 AM
Subscribers
None

Details

Summary

See PHI1014. We may not have Identities if you race the import pipeline, or in some other cases which are more "bug" / "missing migration"-flavored.

Load the commit data so we can fall back to it if we don't have identities.

Test Plan
  • Wiped out all my identities with UPDATE ... SET authorIdentityPHID = NULL WHERE ....
  • Before change: blame fataled with Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached..
  • After change: blame falls back gracefully.
  • Restored identities with bin/repository rebuild-identities, checked blame again.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/applications/diffusion/controller/DiffusionBlameController.php
30

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

This revision is now accepted and ready to land.Jan 4 2019, 10:21 PM
src/applications/diffusion/controller/DiffusionBlameController.php
30

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.

This revision was automatically updated to reflect the committed changes.