Use PhabricatorAuditEditor instead of PhabricatorEdgeEditor when writing reverts edges. This ensures that a transaction is created in addition to the edge.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- Restricted Diffusion Commit
rPdd42020ef3bc: Use `PhabricatorAuditEditor` to write revert edges
Reverted a commit and pushed to remote. Saw a row created in phabricator_audit.audit_transaction_comment. Interestingly, I can't actually see the transaction at http://phabricator.local/r${CALLSIGN}${REVERTED_COMMIT_HASH}.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Warnings Severity Location Code Message Warning src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:124 TXT3 Line Too Long - Unit
Tests Passed - Build Status
Buildable 3574 Build 3582: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I'm not sure why, but I am hitting this:
[05-Jan-2015 09:50:12 Australia/Sydney] [2015-01-05 09:50:12] EXCEPTION: (PhutilProxyException) Error while executing task ID 105944 from queue. {>} (PhabricatorDataNotAt
tachedException) Attempting to access attached data on PhabricatorRepositoryCommit (via getRepository()), but the data is not actually attached. Before accessing attachab
le data on an object, you must load and attach it.
Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method e
xplicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
[05-Jan-2015 09:50:12 Australia/Sydney] #0 PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository
Commit.php:46]
[05-Jan-2015 09:50:12 Australia/Sydney] #1 PhabricatorRepositoryCommit::getRepository() called at [<phabricator>/src/applications/audit/editor/PhabricatorAuditEditor.php:528]
[05-Jan-2015 09:50:12 Australia/Sydney] #2 PhabricatorAuditEditor::shouldSendMail(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:796]
[05-Jan-2015 09:50:12 Australia/Sydney] #3 PhabricatorApplicationTransactionEditor::applyTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:141]
[05-Jan-2015 09:50:12 Australia/Sydney] #4 PhabricatorRepositoryCommitMessageParserWorker::updateCommitData(DiffusionCommitRef) called at [<phabricator>/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php:15]
[05-Jan-2015 09:50:12 Australia/Sydney] #5 PhabricatorRepositoryGitCommitMessageParserWorker::parseCommit(PhabricatorRepository, PhabricatorRepositoryCommit) called at [<phabricator>/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php:44]
[05-Jan-2015 09:50:12 Australia/Sydney] #6 PhabricatorRepositoryCommitParserWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
[05-Jan-2015 09:50:12 Australia/Sydney] #7 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:157]
[05-Jan-2015 09:50:12 Australia/Sydney] #8 PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:19]
[05-Jan-2015 09:50:12 Australia/Sydney] #9 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:91]
[05-Jan-2015 09:50:12 Australia/Sydney] #10 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:111]Comment Actions
Looks good so far. One inline.
| src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php | ||
|---|---|---|
| 143 | Use setContinueOnNoEffect(true) to prevent the Editor from throwing this. But PhabricatorEdgeCycleException can still be raised. | |
| src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php | ||
|---|---|---|
| 127–130 | Oh, you don't want to do this. Just implement all the getStringBlahBlahBlah() methods to say: epriestley marked this commit as reverting rXabcdef. epriestley marked rXabcdef as reverting this commit. (Or find some more natural language.) The table exists, but TYPE_EDGE transactions don't show comments, I think. You could add a second transaction with TYPE_COMMENT, but I think it would be overkill to put the entire commit message on the reverted commit. | |
| src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php | ||
|---|---|---|
| 127–130 | I'm not sure how the get*String() methods would work here. '%s added %s blocking task(s): %s.' is an example of such a string, so perhaps '%s reverted %s commit(s): %s'? But then getTransactionRemoveString seems odd as '%s unreverted %s commit(s): %s'? | |
Comment Actions
This is the best I can come up with offhand:
- epriestley added reverted commits: rX, rY.
- epriestley removed reverted commits: rX, rY.
- epriestley edited reverted commits, added: rX, removed: rY.
And one of these on the other side:
- epriestley added reverting commits: rX, rY.
- epriestley removed reverting commits: rX, rY.
- epriestley edited reverting commits, added: rX, removed: rY.
Realistically, since these will never be removed or edited and the singular case is the most common, we could maybe special case the inverse edge singular and just do:
- epriestley reverted this commit in rX.
It would be OK for the relevant getString() methods to do logical tests against the number of affected objects, etc., if that seems significantly better.
| src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php | ||
|---|---|---|
| 92 ↗ | (On Diff #26935) | In a totally ideal world, we should probably catch PhabricatorEdgeCycleException, then remove the revert transaction and re-apply just the "commit" transaction. We can wait until a user manages to hit this before doing the legwork to set it up and test it, though. |