Summary:
Ref T12997. Although we can't query by transaction type (since we can't easily enumerate all possible types which may have comments -- inline types may also have comments), we //can// just check if there's a comment row or not.
This reduces the amount of garbage we need to load to rebuild indexes for unusual objects with hundreds and hundreds of mentions.
Test Plan:
- Used batch editor to mention a task 700 times.
- Indexed it before and after this change, saw index time drop from {nav 1600ms > 160ms}.
- Made some new comments on it, verified that they still indexed/queried properly.
- Browsed around, made normal transactions, made inline comments.
- Added a unique word to an inline comment, indexed revision, searched for word, found revision.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12997
Differential Revision: https://secure.phabricator.com/D18660
Summary:
Ref T12997. See that task for more details. Briefly, an unusual dataset (where commits are mentioned hundreds of times by other commits) is causing some weird memory behavior in the daemons.
Forcing PHP to GC cycles explicitly after each task completes seems to help with this, by cleaning up some of the memory between tasks. A more thorough fix might be to untangle the `$xactions` structure, but that's significantly more involved.
Test Plan:
- Did this locally in a controlled environment, saw an immediate collection of a 500MB `$xactions` cycle.
- Put a similar change in production, memory usage seemed less to improve. It's hard to tell for sure that this does anything, but it shouldn't hurt.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12997
Differential Revision: https://secure.phabricator.com/D18657
Summary:
See PHI87. Ref T12974. The `array_slice()` method of splitting the string apart can perform poorly for large input strings. I think this is mostly just the large number of calls plus building and returning an array being not entirely trivial.
We can just use `substr()` instead, as long as we're a little bit careful about keeping track of where we're slicing the string if it has UTF8 characters.
Test Plan:
- Created a task with a single, unbroken blob of base64 encoded data as the description, roughly 100KB long.
- Saw indexing performance improve from ~6s to ~1.5s after patch.
- Before: https://secure.phabricator.com/xhprof/profile/PHID-FILE-nrxs4lwdvupbve5lhl6u/
- After: https://secure.phabricator.com/xhprof/profile/PHID-FILE-6vs2akgjj5nbqt7yo7ul/
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18649
Summary:
See PHI87. Ref T12974. Currently, we do a lot more work here than we need to: we call `phutil_utf8_strtolower()` on each token, but can do it once at the beginning on the whole block.
Additionally, since ngrams don't care about order, we only need to convert unique tokens into ngrams. This saves us some `phutil_utf8v()`. These calls can be slow for large inputs.
Test Plan:
- Created a ~4MB task description.
- Ran `bin/search index Txxx --profile ...` to profile indexing performance before and after the change.
- Saw total runtime drop form 38s to 9s.
- Before: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-wiht5d7lkyazaywwxovw/>
- After: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-efxv56q2hulr6kjrxbx6/>
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18647
Summary:
Fixes T12995. Currently, the result highlighter (which shows //where// terms matched) only works in "term" mode, not in "substring" mode.
Provide better feedback and behvaior:
- When a term is a substring term, color it a little differently and add a tooltip. (This is partly to make it easier to debug/diagnose things, probably not enormously valuable to users.)
- When a term is a substring term, highlight it anywhere in the results.
Test Plan:
Queried for latin and CJK terms.
Here is CJK being highlighted:
{F5192195}
Here is substring vs non-substring implicit behavior:
{F5192196}
Here's ONLY terms being highlighted:
{F5192198}
Here's terms and substrings, since the query now has a substring:
{F5192201}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12995
Differential Revision: https://secure.phabricator.com/D18635
Summary:
Ref T2543. This doesn't stand alone since mail still goes out normally, but gets this piece working: new revisions start as "Draft", then after updates if there are no builds they go into "Needs Review".
This should work in general because builds update revisions when they complete, to publish a "Harbormaster finished build yada yada" transaction. So either we'll un-draft immediately, or un-draft after the last build finishes.
I'll hold this until the mail and some other stuff (like UI hints) are in slightly better shape since I think it's probably too rough on its own.
Test Plan: Created revisions locally, saw them un-draft after builds.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18628
Summary:
Ref T2543. Currently, we always do some special things when a revision is created, mostly adding more stuff to the mail.
With drafts, we want to suppress initial mail and send this big, rich mail only when the revision actually moves out of "draft".
Prepare the code for this, with the actual methods hard-coded to the current behavior. This will probably take some tweaking but I think I got most of it.
Test Plan: Banged around in Differential so it sent some mail, saw normal mail without anything new.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18627
Summary:
Ref T2543. Most actions are not available for drafts.
Authors can "Request Review" (move out of draft to become a normal revision) or "Abandon".
Non-authors can't do anything (maybe we'll let them do something later -- like "Commandeer"? -- if there's a good reason).
Test Plan: Viewed a draft revision as an author and non-author, saw fewer actions available.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18626
Summary: See PHI79. When you edit another user's SSH keys (normally, for a bot account) we currently redirect you to an older URI.
Test Plan:
- Viewed a bot's profile page.
- Clicked "Edit Settings" on the Manage page.
- Went to "SSH Keys".
- Uploaded an SSH key.
- Before: redirected to a 404 after finishing the workflow.
- After: redirected to the same page after the workflow.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18633
Summary:
See PHI78. The user was getting this message and (reasonably) interpreted it to mean "reset mail can never be sent to unverified addresses".
Reword it to be more clear, albeit an entire paragraph long. I don't really have a good solution in these cases where we'd need a whole page to explain what's happening (this, plus "we can't tell you which address you should use because an attacker could get information if we did" and "this rule defuses the risk that an opportunistic attacker may try to compromise your account after you add an email you don't own by mistake"). We could write it up separately and link to it, but I feel like that stuff tends to get out of date.
Just land somewhere in the middle.
Test Plan: {F5189105}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18630
Summary:
Ref T2543. There's no way to put revisions into this state yet, but start adding support for when there is.
Adds the status constant, plus support for bucketing them.
Test Plan:
- Manually put a revision in "Draft" state by updating the database directly.
- Verified my drafts showed up in a "Drafts" section on the bucket view.
- Verified others' drafts did not appear on the action bucket view.
- Viewed revisions, queried for "Draft" revisions, etc (stuff we get for free).
{F5186781}
{F5186782}
{F5186783}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18625
Summary:
Fixes T12986. I caught this bug in the changes from D18584: when we moved a large hunk to file storage, we would decompress it but keep the "deflated" flag. This could cause confusion when loading it later. I missed this in testing since I wasn't exhaustive enough in checking hunks and didn't run into a compressed one.
Instead of compressing on `save()`, compress during the normal workflow.
We currently never advise users to run this workflow so I didn't bother trying to clean up possible existing migrations.
Test Plan:
- Ran `bin/differential migrate-hunk` on compressed hunks, moving them to and from file storage. Saw them work correctly and remain compressed.
- Created new small (uncompressed) and large (compressed) hunks, verified they work properly and get compressed (if applicable).
- Used `bin/cache purge --caches changeset` to clear changeset caches and make sure the actual table was being hit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12986
Differential Revision: https://secure.phabricator.com/D18624
Summary: Despite how I (and everyone else?) pronounce it, it is spelled with an "a". See PHI38.
Test Plan: Googled both spellings.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18622
Summary:
See PHI71. These didn't get properly updated when we wrote Subprojects and Milestones, and should use materialized members, not raw members. Swap the query so projects you are an indirect member of (e.g., milestones you are a member of the parent for, and parent projects you are a member of a subproject of) are included in the result list.
Also fix a bad typeahead datasource.
Test Plan:
- Ran a dry run with the test console, saw project PHIDs for milestones and parent projects in the raw field value.
- Tried to set "Author's projects" to a user, no longer could.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18619
Summary:
Ref T11823. I think this is the last callsite which relies on the old data format: `bin/repository parents` rebuilds a cache which we don't currently use very heavily.
Update it to work with the new data.
Test Plan: Ran `bin/repository parents <repository> --trace`, saw successful script execution and reasonable-looking output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18615
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.
I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.
Test Plan:
- This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
- Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
- Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
- Browed around Diffusion in various repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18614
Summary:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.
This data was moved to the RefPosition table in D18612.
Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18613
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.
Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.
Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.
Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.
(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)
Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18602
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391>.
The Mercurial commit is helpful in particular: <https://www.mercurial-scm.org/repo/hg/rev/77eaf9539499>
We weren't vulnerable to the security issue (users can not control any part of the command) but pass the working directory explicitly to get past the new safety check.
I left `setCWD()` in place (a few lines below) just because it can't hurt, and in some other contexts it sometimes matter (for example, if commit hooks execute, they might inherit the parent CWD here or in other VCSes).
Test Plan:
- Cloned from a Mercurial repo locally over HTTP.
- Verified that SSH cloning already uses `-R` (it does, see `DiffusionMercurialServeSSHWorkflow`).
- Did not actually upgrade to Mercurial 4.0/4.1.3 to completely verify this, but a user in the Discourse thread asserted that a substantially similar fix worked correctly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18611
Summary: See T12414. This just gets started; we still need edit endpoints for network interfaces and bindings.
Test Plan: Created some devices/services from the conduit UI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18605
Summary:
See PHI66. See that issue for context. This UI is bad broken legacy junk, but was especially broken when reporting merges.
These do not currently generate a "status" transaction, so they were never counted as task closures. Pretend they're normal closures.
This is still wrong, but should be much closer to the real numbers. Specifically, if you merge a closed task into another task, it will incorrectly be counted as an extra close. This could result in negative tasks, but the numbers should be much closer to reality than they are today even so.
The "Facts" application (T1562) is the real pathway forward here in the longer term.
Test Plan:
- Moved my `maniphest_transactions` table aside with `RENAME TABLE ...`.
- Created a new empty table with `CREATE TABLE ... LIKE ...`.
- Reloaded reports UI, saw empty chart.
- Created, closed, and reopened tasks while reloading the chart, saw accurate reporting.
- Merged an open task into another task, saw bad reporting.
- Applied patch, saw the right chart again.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18601
Summary:
Ref T11823. See PHI68. T11823 has a full description of this issue and a plan to fix it, but the full plan is relatively complicated.
Until that can happen, provide a workaround for the biggest immediate issue, where multiple copies of a ref cursor can cause `executeOne()` to throw, since it expects a single result. In practice, these copies are always identical so we can just pick the first one.
This will get cleaned up once T11823 is fixed properly.
Test Plan:
Forced the table into a duplicate/ambiguous state, reproduced a similar-looking error:
{F5180999}
Applied the patch, got the "Land" to work as expected:
{F5181000}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18599
Summary:
Ref T12033. This is a very narrow fix for this issue, but it should fix the major error: don't attach patches if they're bigger than the mail body limit (by default, 512KB).
Specifically, the logs from an install in T12033 show a 112MB patch being attached, and that's the biggest practical problem here.
I'll follow up on the tasks with more nuanced future work.
Test Plan: Enabled `differential.attach-patches`, saw a patch attached to email. Set the byte limit very low, saw patches get thrown away.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12033
Differential Revision: https://secure.phabricator.com/D18598
Summary:
Fixes T12979. In D18457, we added a "No Notifications" setting to let users disable the blue and yellow pop-up notifications that alert you when an object has been updated, since some users found them distracting.
However, the change made "do nothing" the default, so all other `JX.Notification` callsites -- which never pass a preference -- were effectively turned off no matter what your setting was set to. This includes the "Read-Only" mode warning (grey), the "High Security" mode warning (purple), the "timezone" warning, and a few others.
Tweak things a little bit so the setting applies to ONLY blue and yellow ("object you're following was updated" / "this object was updated") notifications, not other types of popup notifications.
Test Plan:
- With notifications on in settings, got blue notifications and "Read-only".
- With notifications off in settings, got "Read-only" but no blue notifications.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12979
Differential Revision: https://secure.phabricator.com/D18600
Summary:
Ref T12819. In D18581, I corrected one bug (ngram selection for terms) but introduced a minor new bug. We now pass `' query '` (term corpus with boundary spaces) to the stemmer, but it bails out on this since English words don't start with spaces.
Trim these extra boundary spaces off before invoking the stemmer.
The practical effect of this is that searching for non-stem variations of a word ("detection") now finds stemmed variations again ("detect"). Prior to fixing this bug, the stem could find longer variations but not the other way around.
Test Plan: Searched for "detection", found results matching "detect" after patch (and saw same results for "detect" and "detection").
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18593
Summary: Ref T12819. Adds some documentation for `-term`, `~term`, `title:term`, etc.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18592
Summary: Ref T12819. Swaps constants so existing configurations that use a "mysql" engine now use the Ferret engine, not an InnoDB/MyISAM FULLTEXT engine.
Test Plan: Swapped my local config back to "mysql" (the default), saw Ferret engine results in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18590
Summary:
Ref T12819. These render the little "Searched For: X, Y, U V" hint about how something was parsed.
(This might get a "substring" color or "title only" color or something in the future.)
Test Plan: {F5178807}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18589
Summary:
Ref T12819. Obsoleted by the Ferret engine "Query" field.
This is a compatibility break, I'll note it in the changelog.
Test Plan: Searched for repositories by name with "Query" instead of "Name Contains".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18588
Summary:
Ref T12819. Show the new Ferret engine fields (and enable the indexer) unconditionally.
Also pull them to the top since they're fairly general-purpose and appear more broadly now, and also they actually work correctly (WOW).
Some redundant fields (like "Name Contains" in Repositories and Owners) could probably be removed now, I may clean those up in a followup.
Test Plan: Browsed around, saw Ferret fields in UI without "(Prototype)" suffix.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18587
Summary:
Ref T12819. Obsoleted by the Ferret engine, which is unprototyping shortly.
This breaks compatibility in two ways:
- `maniphest.query` no longer supports "fullText" (now throws an explicit exception).
- Existing saved searches with a "Contains Words" constraint will no longer have that constraint.
It seems unlikely (?) that either of these are seeing too much use, and they should be easy to fix. I'll note them in the changelog.
Test Plan: Viewed Maniphest, no more "Contains Words" field. Called `maniphest.query` with "fullText", got explicit exception.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18586
Summary:
Ref T12932. For long-lived installs, one of the largest tables tends to be the hunk data table. Although it doesn't grow tremendously fast, it's also well suited to storage in Files instead of the database (infrequent access, relatively large blobs of data, mostly one-at-a-time access), and earlier work anticipated eventually adding support for Files storage.
Make Files storage work, and provide `bin/differential migrate-hunk` to manually test/migrate hunks. This is currently the only way hunks get moved to file storage, but I expect to add a GC step which moves them to File storage after 30 days shortly.
The immediate motivation for this is to relieve storage pressure on db001/db002 so we have more headroom for deploying the Ferret engine and its larger indexes (see also T12819).
Test Plan:
- Used `bin/differential migrate-hunk` to move a hunk to and from file storage, verified it survived intact.
- Downloaded the actual stored file, sanity-checked it. Verified permissions.
- Destroyed a diff with `bin/remove destroy`, saw the hunk and file storage destroyed.
- Verified that going from file -> text destroys the old file properly with `migrate-hunk --trace ...`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12932
Differential Revision: https://secure.phabricator.com/D18584
Summary:
Ref T12819. For queries like `v0.2`, we would incorrectly search for ngrams including `0.2`, but this is only a substring ngram: the term corpus splits this into `v0` and `2`, so `0.2` is not in the ngrams table.
When executing term queries, search for term ngrams instead. This makes "v0.2" work properly again.
Test Plan: Searched for "v0.2", found a task with "v0.2" in the title.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18581
Summary:
Ref T12819. The "full" field has all other fields, and the "core" field has "title" and "body". Due to the way the "full" and "core" fields were being built, the "core" field also got included in the "full" field, so the "full" field has two copies of the title, two copies of the body, and then one copy of everything else.
Put only one copy of each distinct thing in each "full" and "core". Also, simplify the logic a little bit so we build these virtual fields in a more consistent way.
Test Plan: Ran `bin/search index` and looked at the fields in the database, saw less redundant information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18580
Summary:
Ref T12819. Currently, strings are split only on spaces, but newlines (and, if they exist, tabs) should also split strings.
Without this, we can fail to get the proper term boundary tokens for words which begin at the start of a line or end at the end of a line.
Test Plan: Reindexed a document with "xyz\nabc", saw `"yz "` and `" ab"` term boundary tokens generate properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18579
Summary:
Ref T12819. This worked right in a non-cluster environment, but `bin/storage upgrade` iterates over each master in a partitioned cluster environment.
Tweak the API so `bin/storage analyze` targets a single host but `bin/storage upgrade` can hit all the masters.
Test Plan: Will run `bin/storage upgrade` in production again. Ran `upgrade` and `analyze` locally, still work fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18576
Summary: Miss this with earlier pass, updates the VCS password page.
Test Plan: Try to set a vcs password
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18574
Summary:
Ref T12819. Normallly "ANALYZE TABLE" is like sprinkling magic pixie dust on the database and hoping it will make "good vibes" that cause it to go faster, but in at least some concrete cases with the ngrams tables there really was a key cardinality issue which ANALYZE TABLE corrected, fixing bogus query plans.
Add `bin/storage analyze` to analyze all tables, and make `bin/storage upgrade` run it after adjustment if `--no-adjust` is not specified, and make `bin/storage adjust` run it always.
This runs in a couple seconds and should never hurt anything, so it should be fine to sprinkle lots of pixie dust into the `bin/storage` workflow.
Test Plan: Ran `bin/storage analyze`. Ran `bin/storage upgrade`, saw analyze run. Totally felt great vibes and really aligned chakras on the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18573
Summary: Ref T12819. More ferret engine support.
Test Plan: Indexed and searched commits and repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18572
Summary: Ref T12819. Support for Pholio.
Test Plan: Indexed and searched mocks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18569
Summary: Ref T12819. Adds ferret engine support for Calendar events.
Test Plan: Indexed and queried calendar events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18568
Summary: Ref T12819. Adds Ferret engine support.
Test Plan: Indexed and searched for documents.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18567
Summary: Ref T12819. Adds support for projects.
Test Plan: Indexed and searched for projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18566
Summary: Ref T12819. Mostly straightforward, with a couple of minor query modernization things.
Test Plan: Indexed and searched for posts and blogs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18565
Summary: Ref T12819. Same deal as before, but smaller diffs after D18559.
Test Plan: Indexed and searched for packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18564
Summary:
See brief discussion in D18554. All the index tables are the same for every application (and, at this point, seem unlikely to change) and we never actually pass these objects around (they're only used internally).
In some other cases (like Transactions) not every application has the same tables (for example, Differential has extra field for inline comments), and/or we pass the objects around (lots of stuff uses `$xactions` directly).
However, in this case, and in Edges, we don't interact with any representation of the database state directly in much of the code, and it doesn't change from application to application.
Just automatically define document, field, and ngram tables for anything which implements `FerretInterface`. This makes the query and index logic a tiny bit messier but lets us delete a ton of boilerplate classes.
Test Plan: Indexed objects, searched for objects. Same results as before with much less code. Ran `bin/storage upgrade`, got a clean bill of health.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18559
Summary: Ref T12819. Adds Ferret support to Passphrase.
Test Plan: Indexed credentials, searched for credentials.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18556
Summary: Ref T12819. Adds Ferret engine support to initiatives.
Test Plan: Indexed and searched for initiatives.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18555
Summary: Ref T12819. Prepares for Ferret engine support.
Test Plan: Queried for various initiatives.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18554
Summary: Ref T12819. Prepares Fund to move to Ferret.
Test Plan: Searched for initiatives in Fund.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18553
Summary:
Ref T12819. Adds support for indexing user accounts so they appear in global fulltext results.
Also, always rank users ahead of other results.
Test Plan: Indexed users. Searched for a user, got that user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18552
Summary:
Ref T12819. Currently, under the Ferret engine, we query each application's index separately and then aggregate the results.
At the moment, results are aggregated by type first, then by actual rank. For example, all the revisions appear first, then all the tasks.
Instead, surface the internal ranking data from the underlying query and sort by it.
Test Plan: Searched for "A B" with a task named "A B" and a revision named "A". Saw task first. Broadly, saw mixed task and revision order in result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18551
Summary:
Ref T12819. I started trying to get individual engines to drive these constraints (e.g., `ManiphestTaskQuery` can do most of the work) but this is a big pain, especially since most engines don't support "any owner" or "no owner", and not everything has an owner, and so on and so on. Going down this path would have meant a huge pile of stub functions everywhere, I think.
Instead, drive these through the main engine using the fulltext document table, which already has everything we need to apply these constraints in a uniform way.
Also tweak some parts of query construction and result ordering.
Test Plan: Searched for documents by author, owner, unowned, any owner, tags, subscribers, fulltext in global search. Got sensible results without any application-specific code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18550
Summary: Updates and clarifies UI
Test Plan: New peoples, new bots, new mailing list
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18562
Summary: New edit ui
Test Plan: create a space
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18558
Summary: This should have a border
Test Plan: Reload page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18549
Summary:
Ref T12819. Provides a Ferret-engine-based fulltext engine to ultimately replace the InnoDB fulltext engine.
This is still pretty basic (hard-coded and buggy) but technically sort of works.
To activate this, you must explicitly configure it, so it isn't visible to users yet.
Test Plan: Searched for objects with global fulltext search, got a mixture of matching revisions and tasks back.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18548
Summary: Ref T12819. Uses an extension rather than hard-coding support into Maniphest.
Test Plan: Saw "Query" field appear in Differential, which also implements the interface and has support. Used field in both applications.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18547
Summary:
See PHI57. For example, a query for "ios, only()" finds tags tasked with iOS, exactly, and no other tags.
I called this "only()" instead of "exact()" because we use the term/function "Exact" elsewhere with a different meaning, e.g. in Differential.
Test Plan:
Basic query for a tag:
{F5168857}
Same query with "only", finds tasks tagged with only that tag:
{F5168858}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18543
Summary: Ref T12819. This was originally intended for debugging, but never actually used and not clearly useful. There are no callers and it probably does not work. Just get rid of it.
Test Plan: Grepped for callers; none exist.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18544
Summary:
Fixes T12118. See PHI54. This adds a special case for the initial "reviewers" transactions, similar to the existing special case for "projects" transactions.
Although these transactions are redudnant in the web view since you can see the information clearly on the page, they're more reasonably useful in mail.
Test Plan: {F5168838}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12118
Differential Revision: https://secure.phabricator.com/D18542
Summary: Create a diff page, new UI
Test Plan: Create a diff from page
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18529
Summary: This simplifies EditEngine pages in general by removing the dual header, and extending to allow setting of a custom PHUIHeaderView if needed (like settings).
Test Plan:
Review all settings pages, review task, project pages. This should all be fine, but is a big change maybe some layouts I'm not considering. Tested these all mobile, destkop as well.
{F5166181}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18527
Summary: Updates settings panel UI for new white box, cleans up other various UI nitpicks.
Test Plan: Click through each setting that had a local setting page. Edit Engine pages will follow up on another diff.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18526
Summary: Adds a `MenuName` method to applications that `ProfileMenuItem` uses instead of the application name if set. This improves the home/menu/new user experience at little cost. Also renamed the label from Applications to Favorites, since this menu gets altered to provide more than just applications. This also allows instances to set back to Maniphest if they so choose. Overall I think this direction resolves 95% of my concerns, with maybe a small potential downside which I don't really anticipate. We already name Dashboard panels by their object, and that hasn't really caused confusion. I think these links are similar. I click 'Tasks' and get presented a list of my tasks from Maniphest.
Test Plan: Review each of the name changes as a default new install and a modified install.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18524
Summary: Simplifies the Repository Management pages to the new fixed column layout. I've also moved "Status" into the Basics page, which feels better, and moved "Documentation" as a nav item to a button in the header. This removed "action list" and "curtain view" from the management panels and uses the new bits from Config/Phacility. Undecided if the icons should stay or go for the nav. Left them in for Diffusion. I want to update the EditEngine pages to display in this UI and not leave the portal, but I haven't dug into that this page. I'm a bit worried it will not easily be possible.
Test Plan:
Generate a svn, git, hg repository, test each of the new pages and each of the new buttons. Activate, deactivate, etc.
{F5164674}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18523
Summary: Ref T12819. Adds storage and indexing for the Ferret engine to Differential.
Test Plan: Ran `bin/search index D123 --force`, saw indexes appear in database. No UI/user impact yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18540
Summary: Ref T12819. "Relevance" here just means "how many of your search terms are present in the title?" but that's about the best we can do anyway.
Test Plan: Indexed tasks "A B", "A Z", "Z B", and "Z Z" (all with "A B" in comments). Searched for "A B". Got results ranked in the listed order, with "A B" as the most relevant hit for query "A B".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18539
Summary:
This is a full UI pass at a cleaner "Config" application. The main idea is to simplify the UI, center it, and have a different feel than other UI, a sort of "manage" UI theme for objects with loads of settings. Also adds a new minimalistic "WHITE_CONFIG" box type which may get re-used in Diffusion settings. This is a 90% pass, I'll have a few follow up diffs. Specifically:
- Build breadcrumbs as a flexible UI to go into headers.
- One click ObjectItemView option, for hover states.
- Sidenav doesn't always select (AphrontFilter issue)
- Mobile touchups, though it's pretty reasonable.
Test Plan:
Click through every page here, edit options, see new navigation UI. Test a few various setup issue layouts including fatals.
{F5163228}
{F5163229}
{F5163230}
{F5163231}
{F5163232}
{F5163233}
{F5163234}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18519
Summary: Ref T12819. Move these out of the core engine into the Ferret engine. In the future different applications can define different functions, like "summary:..." or whatever. This may get more formalization when I possibly do "author:" and such some time down the road.
Test Plan: Searched for "title:...". Searched for "dog:...", got a useful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18536
Summary:
Ref T12819. Push more of the magic `' '` stuff into the engine and simplify calls to ngram construction.
Also fixes a bug where a task with title "apple banana" and description "cherry doughnut" could match query "banana cherry" by separating separate term segments with newlines instead of spaces.
Test Plan:
- Indexed some objects.
- Searched (term, substring, quoted terms).
- Viewed index in database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18534
Summary: Ref T12819. Earlier I separated some ngram code into an "ngram engine" hoping to share it across the simple Ngrams stuff and the full Ferret stuff, but they actually use slightly different rules. Just pull more of this stuff into FerretEngine to reduce the number of moving pieces and the amount of code duplication.
Test Plan: Searched for terms, rebuilt indexes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18533
Summary: Fixes T12975. This logic didn't deal with PolicyException correctly.
Test Plan: {F5167549}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12975
Differential Revision: https://secure.phabricator.com/D18537
Summary: Custom icons here aren't being set. Also use more standard `tt` UI.
Test Plan: Set an icon, see set Icon.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18522
Summary: See PHI46. The `core:` function means "find results in either the title or body, but not other auxiliary fields like comments".
Test Plan: Searched for text present in the title (yes), body (yes), and comments (no) with the `core:...` prefix.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18514
Summary:
Ref T12819. Some of the extensions "enrich" the document (adding more fields or relationships), while others "index" it (insert it into some kind of index for later searching).
Currently, these are all muddled under a single "index" phase. However, the Ferret extension cares about fields and relationships which other extensions may add.
Split this into two phases: "enrich" adds fields and relationships so other extensions can read them later if they want. "Index" happens after the document is built and has all the fields and relationships.
The specific problem this solves is that comments may not have been added to the document when the Ferret extension runs. By moving them to the "enrich" phase, the Ferret engine will be able to see and index comments.
Test Plan: Ran `bin/search index ...`, grepped for `indexFulltextDocument`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18513
Summary: Implements a new mobile view thats more fullscreen, not boxed, so more space. Fixes issues with mobile tables when scrolling overflowed content.
Test Plan: Test home, branch, tags, code, file browse, graph, compare, history, readme, open revisions, owners.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18505
Summary: Visually selects the button if blame is on.
Test Plan: Turn blame on and off in Diffusion on a file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18504
Summary:
Ref T12819. Adds (hacky, hard-coded) field support (for now, only for "title").
I've written this so `title:quick ferret` is the same as `title:quick title:ferret`. I think this is what users probably mean.
You can do the other thing as `ferret title:quick`, or `title:quick all:ferret`.
Test Plan: Searched for `title:x`, `title:"x"`, `title:~"x"`, etc. Searched for "garbage:y", got an exception since that's not a recognized function. Searched for `title:x y`, saw both do title search.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18503
Summary:
Ref T12819. Supports negating search terms, e.g. "apple -honeycrisp".
When negating a term, we're a little more strict about what can match (that is, what can //prevent// a document from being returned) since it's easy for a user to type "apple -honeycrisp -honey -crisp -crispies -olcrispers -honeyyums" to keep refining their search, but hard/impossible to split apart an overboard term.
Test Plan:
- Ran `apple -smith`, `apple -"granny smith"`, etc.
- Verified `phone -tact` does not exclude `phone contact`.
- (In theory, `phone -~tact` would, but the parser currently doesn't support this, and I'm not champing at the bit to add support.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18502
Summary:
Ref T12819. Distinguishes between "term" queries and "substring" queries, and tries to match them correctly most of the time. For example:
- `example` matches "example", obviously.
- `~amp` matches "example", but `amp` does not.
- `examples` matches "example" through stemming.
- `"examples"` does not match "example" (quoted text does not stem).
- `"an examp"` does not match "an example" (quoted text is still term text).
- `~"an examp"` matches "an example" (quoted, substring-operator text uses substring search).
Test Plan: Ran searches similar to the above, they seemed to do what they should.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18500
Summary:
Ref T12819. Depends on D18492. Instead of passing a raw query into the Query layer, parse it first.
This allows the query layer to figure out which parts should be substring vs term match, and would allow the SearchEngine layer to do `author:...` eventually by picking it out before sending it to the Ferret engine.
Test Plan: Ran some Ferret queries. They work like before, except that nonsense like `-+"quack"` raises an exception now.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18499
Summary:
Ref T12819. Ferret currently does substring search, but this is not the default mode users expect: when you search for the "RICO" act, you do not expect to find documents containing "apRICOt" even though "RICO" is a substring.
To support term search, index the corpus as a list of terms with puncutation removed and whitespace normalized so the engine can match against it.
Test Plan:
Ran `storage upgrade`, ran `search index`, saw sensible database results:
```
rawCorpus: This is the task description.
Hark! Whom'st'dve eaten this "food" shall surely ~perish~?? #blessed
normalCorpus: thi the task descript hark whom dve eaten food shall sure perish bless
termCorpus: This is the task description Hark Whom'st'dve eaten this food shall surely perish blessed
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18498
Summary:
Ref T12819. This addresses two issues:
- One practical issue is that right now, if you search for "dog cat", and they appear in different fields (for example, "dog" appears ONLY in the title, while "cat" appears ONLY in a comment) we won't find the document. This is somewhat rare -- usually, if "dog" appears in the title, it's also repeated in the description -- but I think clearly a bug. To attack this, start automatically creating a virtual "ALL" field with the full document text which we'll use as the primary thing we match against.
- For fields which may occur more than once -- today, only comments -- aggregate them all into one big "all of the text" row instead of writing one row per comment. This partly addresses the first point ("dog" in one comment and "cat" in a different comment won't be found) and partly makes some of the query gymnastics easier.
Test Plan:
Ran `bin/storage upgrade`, ran `bin/search index <Txxx>`, saw sensible corpus values in the database:
```
mysql> select * from maniphest_task_ffield\G
*************************** 1. row ***************************
id: 3
documentID: 1981
fieldKey: full
rawCorpus: This is the task title
This is the task description.
normalCorpus: thi the task titl
thi the task descript
*************************** 2. row ***************************
id: 4
documentID: 1981
fieldKey: titl
rawCorpus: This is the task title
normalCorpus: thi the task titl
*************************** 3. row ***************************
id: 5
documentID: 1981
fieldKey: body
rawCorpus: This is the task description.
normalCorpus: thi the task descript
3 rows in set (0.00 sec)
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18497
Summary:
See PHI47. When users copy/paste a wall of text into a project tokenizer, we can end up performing a very large number of JOINs.
These JOINs seem okay locally and on `secure`, but the install in PHI47 reports hitting issues.
Since these queries are almost certainly illegitimate (I think no one uses 5+ words to find a project), just limit the search to the 5 longest tokens.
Note that typing 6 tokens will still almost always work, since the UI does additional filtering. However, if you have 100+ projects named "a b c d e ..." and search for "a b c d e z", you may not hit it. This is so degenerate that it's hard to imagine any users encountering it.
This is a stopgap fix, I'll file something longer-term as a followup.
Test Plan: Used `/typeahead/class/PhabricatorProjectDatasource/` to run queries. Saw the same results with shorter query plans for all reasonable queries.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18506
Summary: Only for grey buttons, but can expand. Sets a selected class.
Test Plan: Review new changes in UIExamples.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18501
Summary:
Ref T2543. The type on these got changed by accident, it should be "string" (crazy nonsense, compatible) not "int" (sensible, not compatible).
(New API uses sensible strings like "accepted" only.)
Test Plan: Called `differential.query` from web UI, saw `"2"` and similar statuses.
Reviewers: chad, jmeador, lvital
Reviewed By: jmeador, lvital
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18493
