Details
Reparsed some revisions, observed objects getting created in the database. Altered some Identity objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.
Diff Detail
- Repository
- rP Phabricator
- Branch
- repo-id-2
- Lint
Lint Passed Severity Location Code Message Advice src/applications/repository/storage/PhabricatorRepositoryIdentity.php:58 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 20312 Build 27577: Run Core Tests Build 27576: arc lint + arc unit
Event Timeline
This all looks great to me, with one minor inline about possible race behavior.
src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php | ||
---|---|---|
80 | There is some possibility that this save() could race, if two workers are parsing different commits by the same author:
In this case, the slower INSERT will hit AphrontDuplicateKeyQueryException when the second row collides with the first row on the UNIQUE KEY on nameHash. I don't have a great solution for this. Where it comes up elsewhere, I think we usually use this pattern: thing = load(id); if (!thing) { thing = new(id); try { thing->save(); } catch (duplicate) { thing = load(id); if (!thing) { throw new Exception("Raced and lost, then failed to load the object we lost the race to? Very confusing."); } } } This doesn't feel very elegant, but should survive anything reasonable. |
No attempts made to validate behavior under "challenging" author/committer strings.
This doesn't need to be vetted too heavily, no install has actually managed to produce any of these so far that I'm aware of (although I'm just sure one will some day) and I'm pretty confident this pattern works (if it doesn't, we'll have to fix a few other things too).
Also, if workers do race the one that lost the race should just retry, so the race is harmless, just not ideal (and will stick a potentially confusing error in the error log).