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
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
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).