Summary:
Ref T4327. There are a bunch of other probably-related tasks too, some linked there.
We have some rare/unusual bugs in the change parsers, mostly in Subversion, but it's terrifying to touch them because they're complicated and fragile and have no test coverage.
To fix this stuff, I want to make them more testable. In particular, they basically end with this big INSERT right now. Instead, I'm going to make them return objects representing the data to be inserted, then have the common infrastructure do the insert. This gives us two benefits:
- Reduced code duplication on the insert;
- we can stop before the insert and have unit tests examine the objects.
This swaps the Git parser over, but doesn't swap the hg/svn parsers yet. I'll do those separately, the SVN one looks a bit tricky.
Test Plan:
- Used `scripts/repository/reparse.php` to reparse a Git commit, with `--trace`. Verified it looked the same as before and the SQL that was executed seemed reasonable.
- Did the same for `hg` / `svn` commits, to make sure I didn't derp anything. These aren't expected to do anything differently.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7980
Summary: This handles whitespace differently on tables for phones. Fixes T3637
Test Plan: Test a History Table, able to scroll entire message.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3637
Differential Revision: https://secure.phabricator.com/D8011
This seems to be a specific of how browsers are dealing with
spaces/tabs. Multiple spaces works just fine, but multiple
tabs were treating as a single space which breaks indentation.
Now made it so tabs are replaced with 4 spaces. Not ideal but
still better than fully unreadable code. This also matches to
how differential is handling tabs.
Ref T2495. See: <https://github.com/facebook/phabricator/issues/487>
Reviewed by: epriestley
Summary: Updates setup CSS to use more common colors, spacing.
Test Plan: review a fatal, a common error, and uiexample
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8006
Summary: Ref T4327. The change parser unit tests need database fixtures, which are getting a bit slow to build. Speed them up by updating the quickstart.
Test Plan: Initialized new storage via quickstart, clicked around, everything seemed to work properly. Ran all unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8004
Summary:
Ref T4327. This is general cleanup since I was in this area of the code. Primarily, the Mercurial implementation here was completely broken and wrong:
- It returned only one branch, but a commit can be present on many branches.
- It did not account for multiple branch heads.
- It returned a result implying the branch head pointed at the queried commit, which is no consistent or accurate.
Simplify the amount of API we're dealing with by collapsing this method into the very similar `diffusion.branchquery` method.
Test Plan: Looked at mercurial and git repositories and commits, branch information seemed correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8003
Summary: Ref T4327. At some point these two very similar classes got introduced. Collapse `DiffusionBranchInformation` into the nearly identical `DiffusionRepositoryRef`, which enjoys slightly more generality and support.
Test Plan: Viewed branch overview and detail pages. Ran `repository refs` and `repository discover`. Grepped for removed symbols.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8002
Summary:
Currently, we can sit in a sleep() for up to 15 seconds (or longer, in some cases), even if there's a parse request.
Try polling the DB every second to see if we should wake up instead. This might or might not produce significant improvements, but seems OK to try and see.
Also a small fix for logging branch names with `%` in them, etc.
Test Plan: Ran `phd debug pulllocal` and pushed commits, saw them parse within a second.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7998
Summary:
Ref T4327. This moves the last pieces of discovery responsibility out of the PullLocal daemon and into the DiscoveryEngine.
(This makes it easier to discover repositories in unit tests in the future, since we don't need to build a PullLocal daemon and can just build a DiscoveryEngine.)
Test Plan: Ran `phd debug pulllocal`. Ran `repostory discover`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7997
Summary: be sure to use array() (and not null) so we don't fatal if we have no signatures
Test Plan: no more fatal when editing a signature-less document
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8000
Summary: we need this for legalese. Ref T3116
Test Plan: made a legalpad document with underlines. also re-gened docs and noted underlines worked
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7996
Summary:
Ref T4327. Consolidates and simplifies infrastructure:
- Moves Git discovery into DiscoveryEngine.
- Collapses a bunch of the Git and Mercurial code related to stream discovery.
- Removes all cach code from PullLocal daemon (it's no longer called).
- Adds basic unit tests for Git and Mercurial discovery.
Various cleanup:
- Makes GitStream and MercurialStream extend a common base.
- Improves performance of MercurialStream in some cases, by requiring fewer commits be output and parsed.
- Makes mirroring exceptions easier to debug.
- Fixes discovery of Mercurial repositories with multiple branch heads.
- Adds some missing `pht()`.
Test Plan:
I tested this fairly throughly because I think this phase is complete:
- Made new repositories in multiple VCSes and did full imports.
- Particularly, I reimported Arcanist to make sure that TODO was resolved. I think it was related to the toposort stuff.
- Pushed commits to multiple VCSes.
- Pushed commits to a non-close branch, then pushed a merge commit. Observed commits import initially as non-close, then get flagged for close.
- Started full daemons and resolved various minor issues that showed up in the daemon log until everything ran cleanly.
- Basically spent about 30 minutes banging on this in every way I could think of to try to break it. I found and fixed some minor stuff, but it seems solid.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7987
Summary:
Ref T4327. Simplify the git discovery process so I can move it to the DiscoveryEngine, so I can make change parsing testable.
In particular:
- As an optimization, we process closeable branches ("master") first, then process uncloseable branches ("epriestley-devel"). This means that in the common case we can insert a commit as closeable immediately when it is discovered, the first pass through the pipeline will get it right, and the "ref update" step will never need to do any meaningful work.
- Commits which do not initially appear on a closeable branch, but later move to one (via merges or ref moves) will now be caught in the ref update step, have the closeable flag set, and have a message step re-queued.
- We no longer need to do a separate discovery step on closable branches.
- We no longer need to keep track of `seenOnBranches`.
Test Plan: Ran discovery on repositories after pushing commits, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7985
Summary:
Ref T4327. This provides a more general RefCursor-based way to identify closeable commits, and moves away from the messy `seenOnBranches` stuff. Basically:
- When a closeable ref (like the branch "master") is updated, query the VCS for all commits which are ancestors of the new ref, but not ancestors of the existing closeable heads we previously knew about. This is basically all the commits which have been merged or moved onto the closeable ref.
- Take these commits and set the "closeable" flag on the ones which don't have it yet, then queue new tasks to reprocess them.
I haven't removed the old stuff yet, but will do that shortly.
Test Plan:
- Ran `bin/repository discover` and `bin/repository refs` on a bunch of different VCSes and VCS states. The effects seemed correct (e.g., new commits were marked as closeable).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7984
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary: Ref T4327. Moves us one small step forward toward testable change parsers by separating out this unrelated logic from the PullLocal daemon. We will also probably want to run this logic so we can do remote path lookups to limit the role of Arcanist Projects in the future, which is why I made the URI type (here, only "git") a parameter rather than calling this a `GitURINormalizer` or something.
Test Plan:
- Ran unit tests.
- Ran `repository discover` on a correctly-configured remote repository.
- Ran `repository discover` on an incorrectly-configured remote, got an error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7981
Summary: doh
Test Plan: no more fatal on home page
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7993
Summary: ...needs to add a LegalpadDocumentSignatureQuery class to get this done, which is also re-deployed everywhere we were issuing raw queries. Ref T3116.
Test Plan: viewed some signatures. Verified color and footer icons showed up how I wanted them to.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7986
Summary: Not sure this would have avoided the issue, but I remember a couple of other people asking about migrations, so try to make it more clear/obvious that the backup tools are also useful for migrations. Although this is reasonably obvious when you think about it, it's not very obvious when you're trying to do a migration, and maybe making it more explicit will help.
Test Plan: Read new documentation.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7992
Summary:
From @chad. This setup link should open in a new window so you don't lose your context in resolving setup issues.
@chad, this was the only one I could find immediately, let me know if you remember seeing others that I missed.
Test Plan: Faked an error, clicked the link, got a new tab.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7991
Summary: Cookie-prefix should fix phabricator instances where x.com and x.y.com have conflicting cookie names
Test Plan: Pushed branch to dev.phab.example.com, logged into phab.example.com and into dev.phab.example.com.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7979
Summary:
does a few smallish things... Ref T3116
- adds an action to "sign document", thus improving visiblity of this feature from 0 to some value more than 0
- adds a crumb on the edit page to get back to the view page
- warns the user on the edit page IFF signatures exist for the current version that their edits could invalidate those signatures
- adds a "needSignatures" option to the Document Query class
Test Plan: click the new UI elements and they worked. edited a document with signatures, noted warning UI, edited anyway, noted warning UI correctly disappeared on new edit. also verified a single signature had the correct translation
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7983
Summary:
Ref T4310. Fixes T3720. This change:
- Removes concurrent session limits. Instead, unused sessions are GC'd after a while.
- Collapses all existing "web-1", "web-2", etc., sessions into "web" sessions.
- Dramatically simplifies the code for establishing a session (like omg).
Test Plan: Ran migration, checked Sessions panel and database for sanity. Used existing session. Logged out, logged in. Ran Conduit commands.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7978
Summary:
Ref T3116. This creates a policy rule where you can require a signature on a given legalpad document.
NOTE: signatures must be for the *latest* document version.
Test Plan: made a task have a custom policy requiring a legalpad signature. verified non-signers were locked out.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7977
Summary:
Ref T3720. Ref T4310. Currently, we limit the maximum number of concurrent sessions of each type. This is primarily because sessions predate garbage collection and we had no way to prevent the session table from growing fairly quickly and without bound unless we did this.
Now that we have GC (and it's modular!) we can just expire unused sessions after a while and throw them away:
- Add a `sessionExpires` column to the table, with a key.
- Add a GC for old sessions.
- When we establish a session, set `sessionExpires` to the current time plus the session TTL.
- When a user uses a session and has used up more than 20% of the time on it, extend the session.
In addition to this, we could also rotate sessions, but I think that provides very little value. If we do want to implement it, we should hold it until after T3720 / T4310.
Test Plan:
- Ran schema changes.
- Looked at database.
- Tested GC:
- Started GC.
- Set expires on one row to the past.
- Restarted GC.
- Verified GC nuked the session.
- Logged in.
- Logged out.
- Ran Conduit method.
- Tested refresh:
- Set threshold to 0.0001% instead of 20%.
- Loaded page.
- Saw a session extension ever few page loads.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7976
Summary:
Ref T4310. Ref T3720. Two major things are going on here:
- I'm making this table work more like a standard table, which, e.g., makes `delete()` simpler to implement.
- Currently, the primary key is `(userPHID, type)`. I want to get rid of this, issue unlimited sessions, and GC old sessions. This means we can't have a unique key on `(userPHID, type)` anymore. This removes it as the primary key and adds it as a normal key instead. There's no functional change -- the code to generate sessions guarantees that it will never write duplicate rows or write additional rows -- but allows us to drop the `-1`, `-2` qualifiers in the future.
- Also of note, our task is made far simpler here because MySQL will automatically assign values to new `AUTO_INCREMENT` columns, so we don't have to migrate to get real IDs.
Test Plan: Ran migrations, verified table looked sane. Logged out, logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3720, T4310
Differential Revision: https://secure.phabricator.com/D7975
Summary: This modularizes the rest of the GC submethods. Turned out there was nothing tricky.
Test Plan: Ran `bin/phd debug garbage` and got reasonable looking behavior and output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7971
Summary:
The GC is a big block of hard-coded application GCs right now. Among other things, this means third parties can't tap into the infrastructure.
Modularize it into `GarbageCollector` classes. This implements only one to prove the new stuff works; I'll followup with the rest in the next diff or few depending on how much mess I run into.
Test Plan: Used `bin/phd debug garbage` to run the collector in debug mode, observed reasonable output and behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7970
Summary: Adds "verified" and "secretKey" to Legalpad document signatures. For logged in users using an email address they own, things are verified right away. Otherwise, the email is sent a verification letter. When the user clicks the link the signature is marked verified.
Test Plan: signed the document with a bogus email address not logged in. verified the email that would be sent looked good from command line. followed link and successfully verified bogus email address
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, asherkin
Maniphest Tasks: T4283
Differential Revision: https://secure.phabricator.com/D7930
Summary: Fixes T4317. Update the "inline comment" control to a RemarkupControl. This could maybe use some padding/spacing/design touches eventually but seems OK for the moment.
Test Plan: {F101825}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4317
Differential Revision: https://secure.phabricator.com/D7969
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Summary:
Ref T3857.
- Always send mail via daemons. This lets us get rid of this config, and is generally much more performant.
- After D7964, we warn if daemons aren't running.
Test Plan: Sent some mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7965
Summary:
Currently, we try to mostly-kind-of-work if daemons aren't running (for example, we send mail in-process). I want to stop doing this. A major motivator is that `metamta.send-immediately` is confusing for a lot of users and frequently the cause of performance problems. Increasingly, functionality of applications depends on the daemons (Harbormaster, Drydock, Nuance all require daemons to do anything at all). They're also fairly stable/robust/well-tested and no reasonable install should be running without them.
This will let us simplify or remove some flags (like `metamta.send-immediately`) and simplify some other processes like search indexing.
Test Plan: Stopped daemons, loaded warnings, saw daemon warning. Started daemons, reloade, no warning.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7964
Summary: Ref T4310. Ref T3720. We use bare strings to refer to session types in several places right now; use constants instead.
Test Plan: grep; logged out; logged in; ran Conduit commands.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7963
Summary: Ref T4310. Ref T3720. Session operations are currently part of PhabricatorUser. This is more tightly coupled than needbe, and makes it difficult to establish login sessions for non-users. Move all the session management code to a `SessionEngine`.
Test Plan:
- Viewed sessions.
- Regenerated Conduit certificate.
- Verified Conduit sessions were destroyed.
- Logged out.
- Logged in.
- Ran conduit commands.
- Viewed sessions again.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7962
Summary: Adds the ability to set icons into Tags.
Test Plan: tested on UIExamples page.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7961
Summary: Ref T4310. Ref T3720. Partly, this makes it easier for users to understand login sessions. Partly, it makes it easier for me to make changes to login sessions for T4310 / T3720 without messing anything up.
Test Plan: {F101512}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3720, T4310
Differential Revision: https://secure.phabricator.com/D7954
Summary:
Ref T4310. This is a small step toward separating out the session code so we can establish sessions for `ExternalAccount` and not just `User`.
Also fix an issue with strict MySQL and un-admin / un-disable from web UI.
Test Plan: Logged in, logged out, admined/de-admin'd user, added email address, checked user log for all those events.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310
Differential Revision: https://secure.phabricator.com/D7953
Summary: just a little nip/tuck, facebook will surely rejoice.
Test Plan: reload html, see new css
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7956
Summary: Add the 'Depends On' field to releeph requests. This will help the release engineers to be aware of the dependencies and make sure pick them altogether.
Test Plan: Check sandbox. This field shows up when a revision has some dependencies.
Reviewers: JoelB, lifeihuang, epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7946
Summary: See IRC. This is a likely fix for @DctrWatson's error: we're returning `null` but should return `array()` to indicate no values.
Test Plan: Will make @DctrWatson do it.
Reviewers: btrahan
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7950
Summary: This might be `null`.
Test Plan: Loaded, got exception, applied patch, no exception. Viewed a method with an actual message too, that also worked.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7955
Summary: Ref T1344. We try to do a bad edge query with no sources right now if there are no tasks in a project.
Test Plan:
- Hit exception, applied patch, no exception.
- Other boards still have tasks.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7951
