- Maniphest Tasks
- T13216: 2018 Week 45-47 Bonus Content
- 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:
These renames are because there's a new renderPolicy() in a parent (ModularTransaction) but this can't safely just call that, yet.
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.
(nitpick) For consistency, these should be setDetail() and getDetail(), or shouldAllowEnormousChanges() and setShouldAllowEnormousChanges().
(nitpick) Inconsistent capitalization with the other titles below.
Is there a primitive like renderNewValueAsURL() that would sanely truncate absurdly long URLs, make them <a> elements, etc?
Same consistency nitpick as above.
"changed the tracked branches"
For consistency with the below message: "recognized by Phabricator. Valid systems are: %s."
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.