Page MenuHomePhabricator

Convert Owners paths to application transactions
ClosedPublic

Authored by epriestley on May 27 2015, 1:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 4, 10:36 PM
Unknown Object (File)
Fri, Aug 30, 1:47 PM
Unknown Object (File)
Mon, Aug 26, 11:16 PM
Unknown Object (File)
Mon, Aug 26, 4:05 PM
Unknown Object (File)
Mon, Aug 26, 3:34 PM
Unknown Object (File)
Sun, Aug 25, 9:02 PM
Unknown Object (File)
Sat, Aug 24, 8:59 AM
Unknown Object (File)
Sat, Aug 24, 8:59 AM
Subscribers

Details

Summary

Ref T8320. Fixes T8317. Fixes T2831. Fixes T8073. Fixes T7127.

There was a bug with this line:

for ($ii = 0; $ii < count($paths); $ii++) {

...because the array may be sparse if there have been deletes, so count($paths) might be 3, but the real keys could be 1, 5 and 6. I think this was the primary issue behind T7127.

The old Editor did a lot of work to try to validate paths. When a path failed to validate, it silently discarded it. This was silly and pointless: it's incredibly bad UX; and it's totally fine if users saves "invalid" paths. This was likely the cause of T8317, and probably the cause of T8073.

T2831 I'm less sure about, but I can't reproduce it and I rewrote all the logic so I suspect it's gone.

This also records and shows edits, so if stuff does keep happening it should be more clear what's going on.

I removed some adjacent stuff:

  • I removed the ability to delete packages. I'll add "disable" in a future diff, plus bin/remove destroy, like other objects. Getting rid of this now let me get rid of all the mail stuff.
  • I removed "path validation" where packages would try to automatically update in response to commits. This doesn't necessarily make sense in Git/Mercurial, is sketchy, could easily have been the source of T2831, and seems generally complicated and not very valuable. We could maybe restore it some day, but I'd like to get Owners stable before trying to do crazy stuff like that.
Test Plan

Screen Shot 2015-05-26 at 6.16.38 PM.png (1×1 px, 210 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/applications/owners/controller/PhabricatorOwnersPathsController.php
29

Specifically, this was the "updates with deletes" bug.

src/applications/owners/editor/PhabricatorOwnersPackageEditor.php
134

There are a bunch of pathways through this code where we would silently discard edits, which is an awful UX and worse than just letting them through.

src/applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php
59

This stuff silently rewrote packages (!!!) and none of it checks exclude, so I'm highly suspicious it caused T2831.

Specifically, I think committing to any "exclude" path would convert all packages which "exclude" that path to instead "include" it.

(╯°□°)╯︵ ┻━┻

webroot/rsrc/css/aphront/table-view.css
225
epriestley edited edge metadata.
  • Remove extra copy-pasted CSS rule.
This revision is now accepted and ready to land.May 27 2015, 5:16 PM
This revision was automatically updated to reflect the committed changes.