Page MenuHomePhabricator

Modularize Repository transactions
ClosedPublic

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

Details

Summary

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

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

Event Timeline

epriestley created this revision.Wed, Nov 21, 9:59 PM
epriestley requested review of this revision.Wed, Nov 21, 10:01 PM
epriestley marked 2 inline comments as done.Wed, Nov 21, 10:02 PM
epriestley added inline comments.
src/applications/meta/xactions/PhabricatorApplicationPolicyChangeTransaction.php
55–57

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
46–63

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.Wed, Nov 28, 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.

src/applications/repository/xaction/PhabricatorRepositoryEnormousTransaction.php
10–14

(nitpick) For consistency, these should be setDetail() and getDetail(), or shouldAllowEnormousChanges() and setShouldAllowEnormousChanges().

src/applications/repository/xaction/PhabricatorRepositorySVNSubpathTransaction.php
23

(nitpick) Inconsistent capitalization with the other titles below.

src/applications/repository/xaction/PhabricatorRepositorySlugTransaction.php
77

"Duplicate"

src/applications/repository/xaction/PhabricatorRepositoryStagingURITransaction.php
25

Is there a primitive like renderNewValueAsURL() that would sanely truncate absurdly long URLs, make them <a> elements, etc?

src/applications/repository/xaction/PhabricatorRepositorySymbolLanguagesTransaction.php
10–15

Same consistency nitpick as above.

src/applications/repository/xaction/PhabricatorRepositorySymbolSourcesTransaction.php
10–14

Same nitpick.

src/applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php
32

"changed the tracked branches"

src/applications/repository/xaction/PhabricatorRepositoryVCSTransaction.php
52

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

This revision is now accepted and ready to land.Wed, Nov 28, 6:09 PM
epriestley marked an inline comment as done.Wed, Nov 28, 8:06 PM
epriestley added inline comments.
src/applications/repository/xaction/PhabricatorRepositoryStagingURITransaction.php
25

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.