Permalink
Switch branches/tags
Nothing to show
Commits on Nov 24, 2017
  1. Fix node for nullable return value

    joshuaspence committed Nov 24, 2017
    Summary: Ref T4334. Fixes a bug introduced by D18621 which created a node without a valid type.
    
    Test Plan: N/A
    
    Reviewers: epriestley, #blessed_reviewers
    
    Reviewed By: epriestley, #blessed_reviewers
    
    Subscribers: Korvin, Firehed, wjiang
    
    Maniphest Tasks: T4334
    
    Differential Revision: https://secure.phabricator.com/D18639
Commits on Oct 31, 2017
  1. Fix a typo in JSON parser

    epriestley committed Oct 31, 2017
    Summary:
    Caught this in error logs. This is an external and it looks like this is already fixed in the upstream, but updating just for a typo fix feels like a bit much.
    
    (<https://github.com/Seldaek/jsonlint/commit/bc020395df50bb658bf468bad8bfc962275c4a2a>)
    
    Test Plan: Read it.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Differential Revision: https://secure.phabricator.com/D18752
  2. Fix an error with Remarkup when a header-indicator row has more cells…

    epriestley committed Oct 31, 2017
    … than the one above it
    
    Summary:
    From fishing through error logs in PHI184. When row 1 has less cells than row 2, and row 2 has a `|--|` section (indicating that the cell above should be a table header), we try to convert an invalid cell into a table header. PHP obliges and creates this cell, but it doesn't have 'content', which produces this error:
    
    > ERROR 8: Undefined index: content at [/core/lib/libphutil/src/markup/engine/remarkup/blockrule/PhutilRemarkupBlockRule.php:160]
    
    Test Plan: Added a failing (well, complaining) test; made it pass.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Differential Revision: https://secure.phabricator.com/D18751
Commits on Oct 23, 2017
  1. Convert all "include_once" warnings into exceptions in all PHP versions

    epriestley committed Oct 23, 2017
    Summary:
    Fixes T12190. That task has a good description of the issue.
    
    The old code kind of snuck by PHP5's rules to kind of get the right result. To make this work for PHP7, just wipe the error reporting entirely and explicitly convert any error messages into exceptions.
    
    (This does not use pht() because it may run before pht() loads.)
    
    Test Plan: Loaded a revision in PHP7 before D18723, saw an exception pointing at the implementation error.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12190
    
    Differential Revision: https://secure.phabricator.com/D18724
Commits on Oct 12, 2017
  1. Fix a bug which prevented Conduit futures from having parallelism lim…

    epriestley committed Oct 12, 2017
    …ited effectively
    
    Summary:
    Ref T13008. Currently, `ConduitClient` calls `isReady()` on futures before returning them.
    
    This is very old, and wrong: it makes the future activate and start working. In the case of ConduitFutures, which wrap HTTPFutures, it causes them to make an HTTP request.
    
    Instead, return dormant futures without calling `isReady()`. Then the construct used by `ArcanistFileUploader` and many other pieces of code will limit parallelism properly:
    
    ```
    $iterator = id(new FutureIterator($futures))
      ->limit(4);
    foreach ($iterator as $future) {
     // ...
    }
    ```
    
    Also, fix up the `--trace` profiler integration so we get a log when the future actually starts, not when we create it.
    
    Test Plan: Ran `arc upload --trace ...` with a bunch of files, fiddling with the `limit(X)`. Saw `4` (default) and `1` properly throttle requests. Saw `200` unthrottle requests.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T13008
    
    Differential Revision: https://secure.phabricator.com/D18704
  2. Stream "multipart/form-data" file uploads to disk

    epriestley committed Oct 10, 2017
    Summary: Depends on D18700. Ref T13008. Adds support for sending file uploads to disk, similar to the buitin PHP mechanism, and a compatibility layer for building $_FILES.
    
    Test Plan: See next diff; uploaded files using a vanilla file upload with `enable_post_data_reading` off.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T13008
    
    Differential Revision: https://secure.phabricator.com/D18701
  3. Add a rough "multipart/form-data" stream parser

    epriestley committed Oct 10, 2017
    Summary:
    Depends on D18699. Ref T13008. This only handles the basics (and doesn't stream files to disk yet) but provides a stream-oriented parser for multipart/form-data HTTP requests.
    
    I'm going to integrate this into web workflows next and add coverage / fix bugs as I hit them.
    
    Test Plan: Added basic unit tests, which pass.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T13008
    
    Differential Revision: https://secure.phabricator.com/D18700
  4. Add a rough HTTP header value parser

    epriestley committed Oct 10, 2017
    Summary:
    Ref T13008. Before we can disable `enable_post_data_reading`, we must be able to rebuild `$_FILES` ourselves. Before we can do this, we must be able to parse `multipart/form-data` requests. And, before we can do this, we must be able to parse complex HTTP headers, including these:
    
    > Content-Type: multipart/form-data; boundary="ABCDEFG"
    > Content-Disposition: form-data; name="something"; filename="something else"
    
    Add a parser which can do this. The key parts are:
    
      - Picking the "boundary" out of the "Content-Type" header.
      - Picking all the stuff out of the "Content-Disposition" header for the actual multipart body.
    
    This parser probably isn't perfect, but it will only be invoked when users upload vanilla files (e.g., "Change Profile Picture") so it's okay if it takes a while to sort out all the details.
    
    Test Plan: Added unit tests, ran unit tests.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T13008
    
    Differential Revision: https://secure.phabricator.com/D18699
Commits on Oct 2, 2017
  1. Properly cache terminal width and dirty on SIGWINCH

    epriestley committed Oct 2, 2017
    Summary:
    See PHI110. Currently, the cache here is bad: if `tput` fails, we'll keep running `tput` over and over again, since `null` is not cached.
    
    Instead:
    
      - Cache `null`.
      - Dirty the cache when we receivew SIGWINCH, which indicates the window/terminal metadata has changed.
    
    Test Plan:
      - Ran `scripts/test/progress_bar.php` in wide and narrow windows, saw it fit to the window.
      - Resized the window while it was running, saw it (mostly) figure it out (it can leave some artifacts behind, but we can't do much about that).
      - Added some debugging "print" statements to make sure the cache was working, saw it only recalculate once and then after a resize.
    
    Reviewers: amckinley, chad
    
    Reviewed By: amckinley
    
    Differential Revision: https://secure.phabricator.com/D18669
  2. Fix an exception in the hyperlink remarkup rule for unparseable URIs

    epriestley committed Oct 2, 2017
    Summary:
    Ref T12526. URIs in the form `http://x.y/#http://x.y/#` can fail to parse. Catch these cases and ignore them, rather than throwing.
    
    See also similar changes earlier, in D18149 and D18076.
    
    Test Plan:
      - Added failing test cases, made them pass.
    
    {F5207360}
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12526
    
    Differential Revision: https://secure.phabricator.com/D18666
Commits on Sep 27, 2017
  1. Improve search stemmer performance for large inputs

    epriestley committed Sep 26, 2017
    Summary:
    Ref T12974. See PHI87. As in D18647, we can improve the performance of some UTF8 operations here.
    
    Instead of calling `phutil_utf8_strtolower()` on each token separately, call it once on the entire input up front. This has the same effect.
    
    Test Plan:
      - See D18647; indexed a 4MB task description.
      - Before, a bit over 9s. After, a little under 6s, with about 3s spent in 8 calls to `mb_convert_case()` that we probably can't improve easily.
      - Before: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-efxv56q2hulr6kjrxbx6/>
      - After: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-pj4uv2pkdfoujxq44piq/>
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12974
    
    Differential Revision: https://secure.phabricator.com/D18648
Commits on Sep 26, 2017
  1. Provide ConsoleView classes for "[ OKAY ] Good things happened." cons…

    epriestley committed Sep 26, 2017
    …ole lines
    
    Summary:
    Ref T12996. CLI tools are increasingly standardizing on the output format with a colored "OKAY", "FAIL", "SKIP", "STORAGE UPGRADE", "DEPLOY", etc., in the leftmost column for easy scanning.
    
    However, we have like 20 different things that each render it on their own. Add a 21st in libphutil, in hopes that we can have fewer than 21 some day.
    
    Test Plan: Used this to render a lint success message in D18645.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12996
    
    Differential Revision: https://secure.phabricator.com/D18646
Commits on Sep 25, 2017
  1. Update `parser_nodes.php`

    joshuaspence committed Sep 25, 2017
    The `Makefile` doesn't seem to update this file...
    
    Auditors: epriestley
  2. Add support for nullable return types

    joshuaspence committed Sep 18, 2017
    Summary: Ref T4334. Adds support for nullable return type, which are a thing as of PHP 7.1.
    
    Test Plan: Updated test cases.
    
    Reviewers: epriestley, richardvanvelzen, #blessed_reviewers
    
    Reviewed By: epriestley, #blessed_reviewers
    
    Subscribers: thaiphv, wjiang, Korvin, Firehed
    
    Maniphest Tasks: T4334
    
    Differential Revision: https://secure.phabricator.com/D18621
Commits on Sep 22, 2017
  1. Default CJK query terms to "substring" mode, not "term" mode

    epriestley committed Sep 22, 2017
    Summary:
    Ref T12995. This might be a little rough but should get us started on CJK support.
    
    If a query term has CJK characters and doesn't have a query operator, default it to "substring" mode instead of "term" mode.
    
    You can force a query term into "term" mode with the "+" operator, which is normally redundant. So this should just make the default behavior better without removing any capabilities.
    
    Test Plan:
      - Added some unit tests and made them pass.
      - Searched for "主羽岡" with no operators, found "距暮類供派主羽岡際問月勉央".
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12995
    
    Differential Revision: https://secure.phabricator.com/D18634
Commits on Sep 15, 2017
  1. Pass HGPLAIN in the environment when testing "hg" version

    epriestley committed Sep 15, 2017
    Summary:
    See T12972. In Phabricator, we always execute with `HGPLAIN` but this didn't make it into the standalone binary analyzer:
    
    https://secure.phabricator.com/source/phabricator/browse/master/src/applications/diffusion/protocol/DiffusionMercurialCommandEngine.php;c71cb944a42851de00b2129f15899f7681b0f7ed$31
    
    `HGPLAIN` disables translations and extensions which may make `hg version` do something unexpected.
    
    Test Plan: Verified my local version of Mercurial is still identified correctly in Config. I have yet to figure out how to install non-English Mercurial so I didn't truly test this, but our use of HGPLAIN in other contexts is well-established.
    
    Reviewers: amckinley
    
    Reviewed By: amckinley
    
    Differential Revision: https://secure.phabricator.com/D18610
Commits on Sep 7, 2017
  1. Adding support for CloudFormation API calls

    Austin McKinley committed Sep 7, 2017
    Summary: Also dumps the result body if we got a list of "errors" that was actually empty.
    
    Test Plan: made some CF API calls
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: Korvin
    
    Differential Revision: https://secure.phabricator.com/D18575
Commits on Aug 30, 2017
  1. Add support for parsing more syntax in search queries

    epriestley committed Aug 29, 2017
    Summary:
    Ref T12819. Supports `field:term` (like `title:fulltext`) and `~` (substring) and `=` (exact) operators. Use cases are:
    
      - `title:fulltext` - Search only in title fields.
      - `~substring` - Search for term as a substring, not a word.
      - `title:="exact full title"` - Search for results with exactly a given title.
    
    Test Plan: Ran unit tests.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12819
    
    Differential Revision: https://secure.phabricator.com/D18492
Commits on Aug 27, 2017
  1. Fix binary translation text

    chadlittle committed Aug 27, 2017
    Summary: This error message triggers an Invalid Translation if binary version fails.
    
    Test Plan: Set binary for `hg` to `Mercurial - 分散構成管理ツール(バージョン 4.3.1)`
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Spies: Korvin
    
    Differential Revision: https://secure.phabricator.com/D18480
Commits on Aug 10, 2017
  1. Increase strictness of URI parsing, rejecting URIs in the form "ssh:/…

    epriestley committed Aug 10, 2017
    …/-flag"
    
    Summary:
    Ref T12961. See that task for discussion of the major attack we're responding to here.
    
      - Reject hosts beginning with "-". These are not legitimate.
      - Reject hosts beginning with ".". These are also not legitimate.
      - Tighten `$` to `\z`. `$` can match either "newline, end of string" or "end of string". `\z` matches ONLY "end of string". We don't want to match a newline, only "end of string" strictly.
      - We already that hosts otherwise contain only "reasonable" characters (letters, numbers, hyphens, and periods).
    
    Test Plan:
      - Added unit tests, ran unit tests.
      - Tried to set a repository URI to `ssh://-oxyz/path` with these changes, which worked previously; it no longer works.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12961
    
    Differential Revision: https://secure.phabricator.com/D18388
Commits on Aug 1, 2017
  1. Consolidate binary (hg, git, svn) analysis code into one place

    epriestley committed Jul 31, 2017
    Summary:
    Ref T12942. Currently, we have a bunch of code spread out all over the place for doing version tests on hg, svn, and git.
    
    Consolidate this code a bit so we can use it to drive a new section on the "Version Information" page showing the installed versions (and paths) for binaries.
    
    The primary goal is to reduce the support burden a little bit. Two particular issues are:
    
      - Various stuff with old (or new) VCS binaries, mostly Mercurial.
      - Cases where the webserver sees a different binary than `sudoers` specifies.
    
    These should be easier to spot with binaries in the version info.
    
    Test Plan:
    Added unit tests, see also next change. Eventual result is this:
    
    {F5073525}
    
    Reviewers: chad, avivey
    
    Reviewed By: chad, avivey
    
    Subscribers: avivey
    
    Maniphest Tasks: T12942
    
    Differential Revision: https://secure.phabricator.com/D18305
Commits on Jun 23, 2017
  1. Don't warn about use of "Throwable" under PHP5

    epriestley committed Jun 23, 2017
    Summary:
    Ref T12855. `Throwable` does not exist in PHP5, but can be used safely in this construct, at a minimum:
    
    ```
    } catch (Exception $ex) {
    } catch (Throwable $ex) {
    ```
    
    We could heavily overachive here by trying to make sure that uses of `Throwable` were all safe, but I suspect we'll end up with about 15 uses and never really have problems with this. Until this is a real problem, just stop the analyzer from complaining about `Throwable` and `Error`.
    
    Test Plan:
      - Changed `Throwable` to `Throwablez`.
      - Ran `arc lint`.
      - Got a lint error.
      - Added `Throwablez` to whitelist.
      - (Prodded some caches.)
      - No more lint error.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12855
    
    Differential Revision: https://secure.phabricator.com/D18151
Commits on Jun 20, 2017
  1. If the overseer can't update the PID file, just move on

    epriestley committed Jun 19, 2017
    Summary:
    Ref T12857. This is a little murky, but this behavior seems clearly better.
    
    When the disk is full, daemons may attempt to update their PID files and fail. When this happens, keep running rather than exiting.
    
    Test Plan:
      - Made `Filesystem::writeFile()` throw an exception unconditionally to simulate a full disk.
      - Ran `bin/phd start`.
      - Saw daemons exit immediately.
      - Applied patch, saw daemons stick around and do useful work instead.
    
    Reviewers: chad, amckinley
    
    Reviewed By: chad
    
    Maniphest Tasks: T12857
    
    Differential Revision: https://secure.phabricator.com/D18139
  2. Make empty Remarkup table rows have more intuitive behavior

    epriestley committed Jun 19, 2017
    Summary:
    Fixes T12849. Currently, when a row contains only empty cells and cells with "----" in them, we treat it as indicating which cells above should be rendered with `<th />` instead of `<td />`.
    
    This produces an unintuitive result when //every// cell is empty.
    
    Instead, treat a row as a "headings" row only if it has at least one "---" cell in it.
    
    Test Plan:
    Added a unit test.
    
    Note that the resulting row is empty, so it doesn't get normal line height.
    
    {F5008070}
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12849
    
    Differential Revision: https://secure.phabricator.com/D18140
  3. Modernize editor selector

    avivey committed with avivey Jun 20, 2017
    Summary:
    FreeBSD doesn't have `nano` by default, so allow falling back to `vi`. Also added `sensible-editor`.
    This mostly retains the order we select things, but reduces some lines.
    
    Test Plan: `arc diff`
    
    Reviewers: epriestley, amckinley, kdhp, #blessed_reviewers
    
    Reviewed By: epriestley, kdhp, #blessed_reviewers
    
    Subscribers: Korvin
    
    Differential Revision: https://secure.phabricator.com/D18141
Commits on Jun 5, 2017
  1. Don't fatal when encountering [[ <bad URI > | ... ]]

    epriestley committed Jun 5, 2017
    Summary:
    Fixes T12796. In D17647, the parser became more strict, but this remarkup rule doesn't deal with it gracefully.
    
    Instead, detect when the parse failed and bail out.
    
    Test Plan:
      - Put `[[ http://good.com#u:p@evil.com/ | broken ]]` into a Remarkup document without backticks.
      - Before patch: fatal ("rejecting ambiguous URI").
      - After patch: link doesn't work (which is correct), but page does.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12796
    
    Differential Revision: https://secure.phabricator.com/D18076
Commits on Apr 23, 2017
  1. In Remarkup, prevent "%%%" on a line from self-terminating the litera…

    epriestley committed Apr 22, 2017
    …l block
    
    Summary:
    Fixes T12621. Currently, `%%%` on a line by itself can both start and terminate a literal block. This is silly and presumably not what anyone ever intends.
    
    Instead, if `%%%` started a block, don't let it terminate the block.
    
    Test Plan:
      - Added a failing test.
      - Made it pass.
    
    {F4920751}
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12621
    
    Differential Revision: https://secure.phabricator.com/D17772
  2. Stabilize sort order for Remarkup block rules

    epriestley committed Apr 23, 2017
    Summary:
    Fixes T10929. Currently, the ordering of Remarkup block rules is ambiguous (multiple rules may have the same priority number) and depends on sort stability (if elements with the same value have their ordered retained by the sort).
    
    Sort stability changed between PHP5 and PHP7, so the result of functions like `asort()` changed too if some of the values are the same.
    
    Currently, some tests fail and some edge-case beahviors differ under PHP7 because of this. Particularly, the Remarkup literal test case fails under PHP 7.1 locally, because `<space><space>%%%` gets interpreted as a code block instead of a literal block.
    
    To fix this:
    
      - Use `msortv()` with `PhutilSortVector`, which is a stable sort.
      - Sort on `<priority, ClassName>`. The class name is guaranteed unique so this ordering is unambiguous.
    
    Test Plan:
    Previously, a literal block test case failed on PHP7.1 locally:
    
    {F4920739}
    
    This test now passes.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T10929
    
    Differential Revision: https://secure.phabricator.com/D17771
  3. Make "fail quietly" flag in DeferredLog a little more quiet

    epriestley committed Apr 23, 2017
    Summary:
    Ref T12611. Currently, access logs have a "setFailQuietly(true)" flag set on them (rationale: the webserver should still work if the log volume fills up or logs are otherwise not correctly configured) but it's only somewhat quiet.
    
    Make it quieter when the log directory itself (not just the logfile) doesn't exist or can't be created.
    
    (If installs run into confusion with this, we could more reasonably raise a setup warning instead.)
    
    Test Plan: With a missing logfile directory, saw a log about being unable to log instead of an exception.
    
    Reviewers: chad, amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12611
    
    Differential Revision: https://secure.phabricator.com/D17777
Commits on Apr 13, 2017
  1. Make the query compiler emit intermediate tokens

    epriestley committed Apr 12, 2017
    Summary:
    Ref T12137. Ref T12003. Ref T2632. Ref T7860. In all these tasks, we want to examine or interact with the query //after// it is parsed, but before it is compiled.
    
    Make the compiler emit an intermediate parsed representation so we can do that.
    
    (The next change fixes Phabricator for this new API.)
    
    Test Plan: Ran unit tests, see also next change.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12137, T12003, T7860, T2632
    
    Differential Revision: https://secure.phabricator.com/D17669
Commits on Apr 10, 2017
  1. Reject ambiguous URIs with unescaped "#" or "?" in username/password …

    epriestley committed Apr 10, 2017
    …parts
    
    Summary:
    Fixes T12526. These URIs are ambiguous and nonstandard, and different versions of different clients parse them differently.
    
    Instead of trying to get this right across PHP versions, just reject these outright. No normal user will ever expect these to work.
    
    Test Plan: Ran unit tests in PHP 7.1, got clean results. See T12526 for more discussion.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12526
    
    Differential Revision: https://secure.phabricator.com/D17647
  2. Make Calendar recurrence sets return the indexes of recurrence dates …

    epriestley committed Apr 10, 2017
    …properly
    
    Summary:
    Ref T11816. Currently, if you specify a start time, you get back indexes starting from that start time (0, 1, ...).
    
    This can cause some bugs elsewhere when querying for recurring event ranges.
    
    Instead, return the indexes consistently, even if there is a start time.
    
    Test Plan: Added unit tests, made them pass. Fixed an existing unit test.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T11816
    
    Differential Revision: https://secure.phabricator.com/D17644
Commits on Mar 25, 2017
  1. Allow daemons to read their own idle state

    epriestley committed Mar 25, 2017
    This sets up the next change, which will make the Taskmaster hibernate
    only if it has been idle for more than a minimum amount of time.
    
    Auditors: chad
  2. Don't hibernate daemons if we'd sleep for less than 30 seconds

    epriestley committed Mar 25, 2017
    In some production situations, PullLocal daemons are going up and down
    fairly frequently. Instead, only hibernate if we'll be down for a while.
    
    Auditors: chad
  3. When daemons hibernate, mark them as not busy

    epriestley committed Mar 25, 2017
    The idle timer was not being reset on a hibernation exit vs a scaledown exit,
    so the pool could get sort of "stuck" relaunching the second daemon. Instead,
    reset the idle timer on hibernation exit.
    
    `bin/phd debug task --pool 4` now scales down cleanly instead of trying to
    relaunch the second daemon after its scales down if the first one is
    hibernating.
    
    Auditors: chad