Page MenuHomePhabricator

Modularize Repository transactions

Authored by epriestley on Nov 21 2018, 9:59 PM.



Depends on D19828. Ref T13216. Before adding new transactions to repositories (filesize limit, copy time limit, etc) modularize the existing transactions.

Test Plan
  • 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

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 21 2018, 9:59 PM
epriestley requested review of this revision.Nov 21 2018, 10:01 PM
epriestley marked 2 inline comments as done.Nov 21 2018, 10:02 PM
epriestley added inline comments.

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.

amckinley accepted this revision.Nov 28 2018, 6:09 PM

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.


Same nitpick.


"changed the tracked branches"


For consistency with the below message: "recognized by Phabricator. Valid systems are: %s."

This revision is now accepted and ready to land.Nov 28 2018, 6:09 PM
epriestley marked an inline comment as done.Nov 28 2018, 8:06 PM
epriestley added inline comments.

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.
This revision was automatically updated to reflect the committed changes.