Details
- Reviewers
amckinley - Maniphest Tasks
- T13216: 2018 Week 45-47 Bonus Content
- Commits
- rPfd12b37d16f6: Modularize Repository transactions
- Created repository.
- Edited callsign (invalid, valid, duplicate, add, remove).
- Edited short name (invaild, valid, duplicate, add, remove).
- Edited description (add, remove).
- Edited encoding (invalid, valid, remove).
- Allowed/denied dangerous changes.
- Allowed/denied enormous chagnes.
- Activated, deactivated, reactivated.
- Changed tags.
- Changed push policy.
- Changed default branch (add, remove).
- Changed track only: add, remove, invalid function, invalid regex.
- Changed autoclose only: add, remove, invalid function, invalid regex.
- Changed publish/notify.
- Changed autoclose.
- Changed staging area (add, remove, invalid).
- Changed blueprints (add, remove).
- Changed symbols (add, remove).
- Grepped for PhabricatorRepositoryTransaction::TYPE_.
- Reviewed transaction history:
Diff Detail
- Repository
- rP Phabricator
- Branch
- rconfig4
- Lint
Lint Passed Severity Location Code Message Advice src/applications/repository/xaction/PhabricatorRepositoryActivateTransaction.php:25 XHP16 TODO Comment Advice src/applications/repository/xaction/PhabricatorRepositoryServiceTransaction.php:34 XHP16 TODO Comment Advice src/applications/repository/xaction/PhabricatorRepositoryServiceTransaction.php:52 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 21203 Build 28833: Run Core Tests Build 28832: arc lint + arc unit
Event Timeline
| src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php | ||
|---|---|---|
| 55–56 | These renames are because there's a new renderPolicy() in a parent (ModularTransaction) but this can't safely just call that, yet. | |
| src/applications/repository/xaction/PhabricatorRepositoryStagingURITransaction.php | ||
| 45–62 | I think this is the only actual behavioral change: I added validation for "Staging URI" so you can't trick clients into ssh://-oExecSomeRandomCode=xyz/ stuff if various stars align. | |
This was kinda big to review, so I read everything and then spot-checked some of the cut/paste changes from PhabricatorRepositoryEditor.php to make sure they ended up in the right places instead of exhaustively going back and forth.
One actual typo, everything else is nitpicks.
| src/applications/repository/xaction/PhabricatorRepositoryEnormousTransaction.php | ||
|---|---|---|
| 9–13 | (nitpick) For consistency, these should be setDetail() and getDetail(), or shouldAllowEnormousChanges() and setShouldAllowEnormousChanges(). | |
| src/applications/repository/xaction/PhabricatorRepositorySVNSubpathTransaction.php | ||
| 22 | (nitpick) Inconsistent capitalization with the other titles below. | |
| src/applications/repository/xaction/PhabricatorRepositorySlugTransaction.php | ||
| 76 | "Duplicate" | |
| src/applications/repository/xaction/PhabricatorRepositoryStagingURITransaction.php | ||
| 24 | Is there a primitive like renderNewValueAsURL() that would sanely truncate absurdly long URLs, make them <a> elements, etc? | |
| src/applications/repository/xaction/PhabricatorRepositorySymbolLanguagesTransaction.php | ||
| 9–14 | Same consistency nitpick as above. | |
| src/applications/repository/xaction/PhabricatorRepositorySymbolSourcesTransaction.php | ||
| 9–13 | Same nitpick. | |
| src/applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php | ||
| 31 | "changed the tracked branches" | |
| src/applications/repository/xaction/PhabricatorRepositoryVCSTransaction.php | ||
| 51 | For consistency with the below message: "recognized by Phabricator. Valid systems are: %s." | |
| src/applications/repository/xaction/PhabricatorRepositoryStagingURITransaction.php | ||
|---|---|---|
| 24 | I don't think there is, currently. Since the URI is often git:// or ssh:// you don't necessarily want to click it in your browser, although maybe <a /> makes it a little easier to copy/paste? Not really sure which way that one goes for most people. Advantage is you can "right-click + copy URL" , disadvantage is that click-select sometimes follow the link by accident and you can't double/triple-click-to-select-adjacent-characters? I think unlinked URLs are slightly easier for me to copy personally but I might be unusual. Seems like enough of a toss-up to not be clearly valuable here. I think the only other place we do this sort of thing (have a transaction which edits a URI) is maybe Phame blog "domain" fields and Phurl URIs? I suppose Phurl URIs might be a good enough use case to motivate actually building this. | |
- Better spelling/consistency.
- I agree on the getDetail() inlines, but think getX/setX is probably the better way to make them consistent and that should probably be a separate change and get proper testing. I'll note and revisit this.