1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-28 01:32:41 +01:00
Commit graph

2416 commits

Author SHA1 Message Date
Christopher Speck
41f6c6ecb2 Update "arc diff" to amend non-head commits with Mercurial
Summary:
After `arc diff` creates a revision in Phabricator it amends the commit to include a link to the revision in the commit message. For Mercurial this is done with `hg commit --amend --logfile` however this will fail when trying to create a diff for a non-head commit.

This updates `ArcanistMercurialAPI::amendCommit()` to allow amending a non-head commit in two ways, depending on whether `evolve` is in use:

No evolve:
1. Rebasing the current commit onto the current commit's parent, using the new commit message
2. Rebasing all children + descendants of the current commit onto the new resulting commit
3. Stripping the original commit

With evolve:
1. Amend the commit with `hg amend --logfile`
2. Run `hg evolve` to tidy up all commits

Test Plan:
I created 6 commits in a row placing a bookmark at commits 2 `bookmark1`, 4 `bookmark2`, and 6 `bookmark3`, and ensured I had `arc:bookmark` in my base ruleset.

No evolve, non-head changeset:
1. I verified I did not have `evolve` enabled by running `hg debugextensions` and did not see `evolve` in the listed active extensions.
2. I updated to `bookmark1` and modified a file to leave a dirty working state.
3. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
4. I checked the status of my repository and verified it was still linear and the bookmarks pointed to the proper commits.
5. I ran `hg log -r bookmark1 --template {desc}` to view the full commit message and verified it contained both `Summary: ...` and `Differential Revision: https://...`.
6. I ran `hg diff -c bookmark1` and verified the changes for that commit included the changes I made in step 2.

No evolve, head changeset:
1. I updated to `bookmark3` which is the head commit and modified a file to leave a dirty working state.
2. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
3. I checked the status of my repository and verified it was still linear and all the bookmarks pointed to the proper commits.
4. I ran `hg log -r bookmark3 --template {desc}` to view the full commit message and verified it contained both `Summary: ...` and `Differential Revision: https://...`.
5. I ran `hg diff -c bookmar3` and verified the changes for that commit included the changes I made in step 1.

With evolve:
1. I enabled `evolve` and verified it was enabled by running `hg debugextensions` and saw `evolve` in the listed active extensions.
2. I updated to `bookmark2` and modified a file to leave a dirty working state.
3. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
4. I checked the status of my repository and verified it was still linear and all the bookmarks pointed to the proper commits.
5. I ran `hg log -r bookmark2 --template {desc}` to view the full commit message and verfieid it contained both `Summary: ...` and `Differential Revision: https://...`.
6. I ran `hg diff -c bookmark2` and verified the changes for that commit included the changes I made in step 2.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D21686
2021-07-20 16:55:27 -04:00
Christopher Speck
a43a3a9aab An assortment of fixes and updates to using arc-land with mercurial
Summary:
Refs T13546

**Behavior Changes**
1. Currently, after landing the `--onto` bookmark will not be advanced to the newly pushed commit(s)
  - This updates so that after pushing commits upstream, onto-marker bookmarks are pulled back from the server to retrieve their updated state
2. Currently, after landing the working directory will typically be the old `--onto` commit
  - This updates the behavior in the following ways
    1. If the starting working state is a commit that is part of a revision being landed, the post-land state will be the newly published commit for that revision
    2. If the starting working state is a commit for a tip revision being landed and there is only a single `--onto` target, the post-land state will be the newly published commit and the `--onto` bookmark will be activated, if applicable. If there are multiple `--onto` targets defined (uncommon) then the resulting working state will be the same behavior as before, as the desired behavior for this case is ambiguous.
    3. If the starting working state is a commit that is not part of any revision being landed, then the post-land state will be that same commit, activating the same previous bookmark if applicable.

**Bugs Fixed**
1. When landing a diff that includes multiple commits, where the non-tip commit adds a file and later commit modifies that file, the land process would fail during rebasing.
2. When landing, the display of what commits are going to be landed only includes the commit hash but should include part of the commit message for easy identification.
3. If errors occur during a rebase while landing, the resulting state is an in-progress rebase. Users will typically not be able to resolve this in-progress rebase and possibly confuse them further as they have to first abort the rebase before trying to clean up. These rebases will now be aborted by arcanist before exiting.
4. If using evolve, landing a non-tip revision would leave behind orphaned commits.

Test Plan:
Bookmark test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to activate `test`
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit

Non-bookmark test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to be the new commit, but not activate `test` even though it points to that same commit
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit

Not-working state test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to an older commit not related to the one being landed
- I landed that diff
- I verified the ending resulting working state was on the older commit I was on prior to landing

Multiple commits test
- Using `test` bookmark I created a commit that added a new file, made a diff, made another new commit on top of it which modified that file, then updated the diff
- I updated my working state to the first commit for the diff (non-head)
- I ran `arc land test` to land the diff
- I verified that the resulting working state was on the newly published `master` commit

Landing while on `master`
- I created a diff with a bookmark `test`, as a branch from before the `master` commit
- I updated working state to the `master` bookmark
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit

Landing with no local `master` bookmark
- I created a diff consisting of two commits with bookmark `test`, as a branch from before the `master` commit
- I left working state on `test` and deleted my local `master` bookmark
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit and the `master` bookmark was pulled and active

Cascading diffs while on an earlier diff changeset
- I setup two diffs, each consisting of two commits, one depending on the other
- I put the working state onto a commit from the earlier diff
- I landed the later diff
- I verified that the resulting working state was on the newly published commit associated with the earlier diff, which is not the new `master` commit so there was no active bookmark

Cascading diffs while on the later diff changeset
- I setup two diffs, each consisting of two commits, one depending on the other
- I put the working state into a commit from the later diff
- I landed the later diff
- I verified that the resulting working state was on the newly published commit for the later diff which was `master` and `master` was active

Landing a non-tip revision, with evolve
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I verified I had the evolve extension enabled via `hg debugextensions`
- I put the working state on a commit in the first revision
- I landed the second revision
- I verified that the resulting working state was clean, that revisions 1 and 2 were properly landed and published, and the resulting working state was on the published commit for the first revision.

Landing a non-tip revision, without evolve
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I verified I did not have the evolve extension enabled via `hg debugextensions`
- I put the working state on a commit in the first revision
- I landed the second revision
- I verified that the resulting working state was clean, that revisions 1 and 2 were properly landed and published, and the resulting working state was on the published commit for the first revision.

Landing a non-tip revision, while the non-landing revision is active
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I put the working state on the latest tip revision commit and its bookmark
- I landed the second revision
- I verified that revisions 1 and 2 were properly landed and published, and the resulting working state was still on the latest tip revision commit and its bookmark was active.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21680
2021-07-11 23:40:53 -04:00
Christopher Speck
514c12366b Update templates used with mercurial to remove '--debug'
Summary:
Refs D21679 (phabricator changes)

This updates Arcanist to be able to check whether the version of Mercurial supports using `{p1.node}` template format vs. `{p1node}`.

Test Plan: Tested under coverage from D21679

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D21681
2021-07-09 12:34:54 -04:00
Christopher Speck
c94c5bbf35 Force all mercurial commands to use UTF-8 encoding
Summary:
When non-ascii characters appear in revision titles/summaries the `patch` and `diff` (to update) commands will fail on Windows systems. This often occurs due to “smart quotes” or "em—dash" characters being inserted into commit messages by editors on "user-friendly" operating systems like macOS.

This can be worked around by forcing all mercurial commands to use the global option `--encoding utf-8` which applies for any mercurial command. This option was [[ https://www.mercurial-scm.org/repo/hg/rev/a88e02081a88 | added in ~2006 ]] so this should work across all supported versions of mercurial.

Refs T13649

Test Plan:
I created a diff on a mercurial repository using smart quotes in the "Title" and "Summary" fields as well as in the content of a file being changed. Then on macOS, Windows (PowerShell), and Windows (cmd.exe) I was able to `patch` down the revision, make a modification, and `diff` the change back up to Phabricator, as well as `land` the change. I verified the commit and content looked correct on macOS as well as on Windows by using `nvim` which seems to properly detect and render the encoding, whereas mercurial displays the smart quotes and em-dashes with odd characters instead.

I did a grep through Arcanist codebase to find other places where `--encoding` might be specified for mercurial commands and could not find any. In the event that somehow this argument is added elsewhere I verified that multiple specifications of `--encoding utf-8` does not cause any issues and the later specification of `--encoding` appears to "win".

```lang=console
$ hg --encoding utf-8 --encoding utf-8 log -r tip
# prints out results in UTF-8 without issue

$ hg --encoding utf-8 log --encoding latin-1 -r tip
# prints out results in latin-1 without issue
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13649

Differential Revision: https://secure.phabricator.com/D21676
2021-06-27 21:39:27 -04:00
Matthew Bowker
7498c10215 Update arcanist readme to reference Phorge
Test Plan: Looked at the rendered markdown.

Reviewers: O1 Blessed Committers, 20after4

Reviewed By: O1 Blessed Committers, 20after4

Subscribers: 20after4, eax, speck, tobiaswiese

Maniphest Tasks: T15006

Differential Revision: https://we.phorge.it/D25009
2021-06-20 15:20:47 -06:00
Matthew Bowker
737bd0d424 Update Diviner documentation to reference Phorge instead of Phabricator for Arcanist.
Test Plan: Generated Diviner documentation on a local install and verified that the changes look good.

Reviewers: O1 Blessed Committers, deadalnix

Reviewed By: O1 Blessed Committers, deadalnix

Subscribers: speck, tobiaswiese

Maniphest Tasks: T15012

Differential Revision: https://we.phorge.it/D25008
2021-06-19 18:46:39 -06:00
ekubischta
cf4b14fcbf Arcanist Update .arcconfig to point to we.phorge.it
Summary:
Updated .arcconfig to we.phorge.it

Refs T15006

I would also note, I created this revision using the container setup developed by @willson556 mentioned in T15011#370 and it worked great!

Test Plan:
Ran `arc install-certificate` and was able to get token from the right place and install it

Ran `arc which` saw that it identified `we.phorge.it`

Reviewers: speck, avivey, tobiaswiese, O1 Blessed Committers, eax, #blessed_committers

Reviewed By: speck, avivey, tobiaswiese, O1 Blessed Committers, eax, #blessed_committers

Subscribers: deadalnix, chris, willson556, speck

Maniphest Tasks: T15006

Differential Revision: https://we.phorge.it/D25003
2021-06-19 18:10:39 +00:00
epriestley
246e604a07 Correct an issue when winning "arc anoid" with certain terminal dimensions
Summary: See PHI2085. Python 3 is stricter about integers and floats than Python 2 was, and we can end up passing a float where an integer was expected if the player wins "arc anoid" using a terminal with certain (most?) character dimensions.

Test Plan:
  - Modified "arcanoid.py" to win instantly.
  - Adjusted terminal window to 80x24, ran "arc anoid", reproduced crash.
  - Ran "python2 arcanoid.py" and observed old victory animation behavior.
  - Applied patch.
  - Ran "arc anoid" and observed identical victory animation behavior.

Differential Revision: https://secure.phabricator.com/D21667
2021-05-31 23:10:36 -07:00
epriestley
be1a4a9142 Avoid leaving stdin in nonblocking mode after a modern prompt
Summary:
Ref T13649. Currently, "arc" may leave stdin nonblocking after showing a prompt. This can cause various odd behaviors down the line.

I can't immediately reproduce this behavior on macOS in "zsh" or "bash" (I'm unable to get stdin to remain nonblocking beyond the process lifespan), and also don't have pcntl locally so there's a fair amount of handwaving here.

Test Plan: This is somewhat speculative since I can't immediately reproduce the behavior. I tested the locally-reachable paths (no pcntl) but they're not interesting.

Maniphest Tasks: T13649

Differential Revision: https://secure.phabricator.com/D21666
2021-05-30 01:33:15 -07:00
epriestley
f0f95e5b26 On Windows, implement "Filesystem::copyFile()" with "copy()"
Summary:
Ref T13562. Currently, "Filesystem::copyFile()" uses "copy", which doesn't work now that we no longer invoke "cmd.exe" by default.

Use "copy()" instead.

Note that this whole function is probably nonsense, but I'll follow up on T13562.

Test Plan:
  - Created a standalone script which runs "Filesystem::copyFile()".
    - Before: failed to copy any file.
    - After: succesfully copied normal files.
    - After: failed to copy a file over an existing directory with a reasonable error.
    - After: failed to copy a file over itself with a reasonable error.

Maniphest Tasks: T13562

Differential Revision: https://secure.phabricator.com/D21643
2021-03-22 12:00:23 -07:00
epriestley
cc23551a7d Update Paste help to include missing "--"
Summary: See PHI2027. This example command is missing "--", but it's required.

Test Plan: Ran the new command, no longer got an error about "--".

Differential Revision: https://secure.phabricator.com/D21623
2021-03-16 09:29:50 -07:00
epriestley
7ad4afb919 Correct a mistaken Phurl link in the "missing symbol" exception
Summary: See PHI2022. This link is missing the `/u/` part and currently 404's.

Test Plan: Followed the new link, got documentation.

Differential Revision: https://secure.phabricator.com/D21611
2021-03-12 09:51:18 -08:00
epriestley
7570dd0da1 Improve "PhutilJSON" handling of PHP-object JSON values
Summary:
Ref T13635. PHP native JSON functions sometimes represent JSON objects as PHP "stdClass" objects.

Accept this representation and emit it correctly in "PhutilJSON".

Test Plan: Added a test, made it pass.

Maniphest Tasks: T13635

Differential Revision: https://secure.phabricator.com/D21604
2021-03-11 12:43:08 -08:00
epriestley
2d6452acb5 In Arcanist, when trying to write to a file configuration source, create missing directories
Summary: The ".git/arc" directory may need to be created when writing to working copy configuration.

Test Plan:
  - Tried to store an answer to a prompt in a new working copy.
  - Before: error that ".git/arc" does not exist.
  - After: prompt saved to working copy configuration.

Differential Revision: https://secure.phabricator.com/D21588
2021-03-03 13:40:56 -08:00
epriestley
953d742a1a In "arc land", if rebasing a range fails, attempt to "reduce" it
Summary:
Ref T13576. See that task for discussion.

When a user runs `arc land --pick A`, they may be selecting a range of commits ("X..Y") which have ancestors ("V..W") that should NOT land.

We must slice "X..Y" out of history before we can merge it, to avoid landing changes from "V..W".

When "X..Y" is simple and linear, we can rebase the range to pick our desired slice out of history.

When "X..Y" includes merge commits, we frequently can not, and I could not identify any simple alternative. The best alternative I came up with is this "reduce" operation:

  - squash "into" onto Y, producing S, to guarantee there are no natural conflicts;
  - squash S onto X^, producing T, to get rid of the merge commits;
  - rebase T onto "into", producing R, to slice "X..Y" out of history;
  - squash R onto "into", producing Q. (R and Q will be the same, but this simplies the code.)

This feels flimsy and fragile, but I can't immediately find a way to break it. See T13576 for more discussion.

Test Plan:
  - Applied conflicting changes to `example.txt` in `master` and `feature1`.
  - Ran `arc land`, got a merge conflict.
  - Resolved the conflict with `git merge master`.
  - Ran `arc land`.
    - Before: merge conflict.
    - After: `arc land` resolves the merge correctly.
  - Stacked `feature2` on `feature1`, and made various mutations to `feature1` and `feature2`, then ran `arc land --pick feature2`. Changes made in `feature1` should not land, and they mostly do not. See T13576.

Maniphest Tasks: T13576

Differential Revision: https://secure.phabricator.com/D21590
2021-03-03 13:40:56 -08:00
epriestley
d72fad6461 Add a character marker to the "IMPLICIT COMMITS" warning in "arc land"
Summary:
Ref T13576. The "implicit commits" prompt in "arc land" shows a list of implicit and non-implicit commits.

The implicit commits are marked with a background color, but this doesn't survive if you copy/paste the output into a support ticket.

Make my life easier by also marking commits so the marker survives copy/paste.

Test Plan: Ran "arc land" with implicit commits, saw a copy-pastable indicator.

Maniphest Tasks: T13576

Differential Revision: https://secure.phabricator.com/D21589
2021-03-03 13:40:56 -08:00
epriestley
4399ee6b7f Temporarily disable all logfile writability checks
These cause too much trouble in too many cases.
2021-03-01 15:55:42 -08:00
epriestley
6d60422dbb Add a simple primitive for managing PHP runtime error logs
Summary:
Ref T13624. If we want to send PHP errors to a log, using the "error_log" configuration option catches the broadest set of errors across versions of PHP.

Configuring this disables errors on `stderr`, since they're sent to the log instead. We'd like them to go to both places; provide a simple wrapper for this. Also do a bit of writability testing.

Test Plan: Wrote errors to a new log, see followup changes.

Maniphest Tasks: T13624

Differential Revision: https://secure.phabricator.com/D21578
2021-02-26 14:54:41 -08:00
epriestley
e95afd1d00 Apply "pcre.*_limit" config options at startup in CLI environments
Summary:
Ref T13100. Ref T13586. These options have been in production for a while and seem to consistently improve behavior, downgrading segfaults to runtime errors.

Make them global options for all CLI environments. See next change for web enviornments.

Test Plan: Ran `arc`.

Maniphest Tasks: T13586, T13100

Differential Revision: https://secure.phabricator.com/D21566
2021-02-19 11:16:09 -08:00
epriestley
9d5802cb9f Provide some "preg_*" wrappers which raise exceptions on failure
Summary: Ref T13608. Ref T13100. Ref T13586. Properly checking "preg_match()" and similar calls for failure and raising useful exceptions is complicated and error-prone. Provide wrapper functions with an API that's more consistent with the rest of the codebase: matches are returned; and errors raise detailed exceptions.

Test Plan: See next change.

Maniphest Tasks: T13608, T13586, T13100

Differential Revision: https://secure.phabricator.com/D21561
2021-02-19 11:16:09 -08:00
Vihang Mehta
f501f85eb8 Update golint install instructions
Summary:
These are the instructions from https://github.com/golang/lint.

The fetch location changed to `golang.org/x/lint/golint` from `github.com/golang/lint/golint`.
`-u` tells go to update the package and its deps if they exist.
`-u` Shouldn't strictly be necessary, but figured we might as well follow the instructions from `golint`.

Test Plan:
Enable golint without having it installed.
Ensure that the install instructions now show the new location.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21552
2021-02-10 10:03:39 -08:00
epriestley
239ad5c55d In "array_mergev()", guarantee the "call_user_func_array()" parameter list is a natrual list
Summary:
Ref T13588. The behavior of "call_user_func_array()" has changed in PHP8, and the function now attempts to use array keys as argument names.

This always fails when calling "array_merge()" (which does not accept named parameters), and may cause misbehavior in the general case.

Guarantee the argument is a natural list (with keys "0", "1", "2", ...).

Test Plan:
  - Behavior unchanged under PHP7.
  - User reports fixed behavior under PHP8, see <https://discourse.phabricator-community.org/t/daemon-fails-on-php-8-0-2-in-utils-php-array-merge-call-w-fix/4568>.
  - See T13588.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21551
2021-02-08 10:19:52 -08:00
epriestley
32fe933f3a Add a lint check for catching "Exception" without catching "Throwable"
Summary:
Ref T13588. For consistency of behavior between versions on either side of PHP7, any "catch (Exception)" block should generally have a "catch (Throwable)" block in the same catch list.

Raise a lint warning when "Exception" is caught without also catching "Throwable", since this is almost certainly not desired.

Test Plan: Added tests.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21542
2021-02-03 14:49:13 -08:00
epriestley
c1afa91f9f Annotate the unusual use of "$callback()" in "xsprintf()"
Summary: Ref T13588. See D21500. This syntax is unusual and there are some hidden complexities involved; annotate them. See D21500 for more discussion.

Test Plan: Read text, reviewed D21500.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21541
2021-02-03 14:21:08 -08:00
epriestley
c51a996fb0 Detect and correct "final private" methods in lint
Summary:
Ref T13588. Marking a method "final private" has never been meaningful, and is an error in PHP8.

Add static analysis to detect (and correct) this issue.

Test Plan: Added unit tests, will lint "phabricator/".

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21539
2021-02-03 14:14:50 -08:00
epriestley
e2b6439a73 Allow lint to correct the spelling of builtin symbols
Summary: Ref T13598. This builds on D21537 and adds support for correcting the capitalization of builtin systems.

Test Plan:
  - Linted a file that uses "ExCePtIoN", got a lint correction.

Maniphest Tasks: T13598

Differential Revision: https://secure.phabricator.com/D21538
2021-02-03 13:26:33 -08:00
epriestley
08dbbbba5a When lint identifies an unknown symbol, attempt to correct it if it is miscapitalized
Summary:
Ref T13598. If you spell a symbol like "Polygon" as "PoLyGoN", you currently get an "unknown symbol" lint message. However, provided "Polygon" is a valid symbol, we can unambiguously correct the spelling of the symbol.

Note that this patch can only correct the spelling of application symbols, not builtin symbols (since none of the library maps contain builtin symbols).

Test Plan: {F8374599}

Maniphest Tasks: T13598

Differential Revision: https://secure.phabricator.com/D21537
2021-02-03 13:26:33 -08:00
epriestley
b2e715fc5a Provide "gitsprintf(...)" and disambiguate Git ref selectors
Summary: Ref T13589. See that task for discussion.

Test Plan:
  - Created this diff, ran most commands in isolation.
  - This change is difficult to test extensively.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21509
2021-01-13 12:31:15 -08:00
Jessica Clarke
172381260e Fix pyflakes tests for recent pyflakes versions
Summary:
Since 2.1.0 (commit 75bc0c03c145), pyflakes has included the Python
version and platform in its version output, so ignore it if present.

Since 2.2.0 (commit 6ba3f8e0b59b), pyflakes has included the column
number in its messages, so update the parser to include it and drop the
column number from the (only) test in order to work with both old and
new versions. Whilst here, assign names to the capture groups to make
the code clearer.

Test Plan: Ran arc unit

Reviewers: epriestley, joshuaspence, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21504
2021-01-11 04:52:29 +00:00
Jessica Clarke
09cff8611b Fix ArcanistJSHintLinterTestCase::testLinter for recent JSHint
Summary:
Recent JSHint improves the warning and attributes it to the equals sign
rather than the end of the expression (changed in 897e0359ce19, first
released in 2.11.0-rc1).

Test Plan: Ran arc unit with JSHint 2.12.0

Reviewers: epriestley, joshuaspence, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21503
2021-01-11 04:51:20 +00:00
Jessica Clarke
f64eb04300 Fix PhutilOAuth1FutureTestCase::testOAuth1SigningWithJIRAExamples for PHP 8
Summary:
PHP 8 deprecates openssl_free_key as the key is automatically freed, so
silence the warning in PhutilOAuth1Future::signString.

Test Plan: Ran arc lint --everything

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21502
2021-01-11 04:50:37 +00:00
Jessica Clarke
9589fd1866 Fix PhutilUTF8TestCase::testUTF8Convert for PHP 8
Summary:
In PHP 8 passing an invalid encoding to mb_convert_encoding raises a
ValueError (which extends Error not Exception), so fix the test to also
catch Throwable (but leave the explicit Exception case for PHP 5, which
lacks Throwable).

Test Plan: Ran arc unit --everything

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21501
2021-01-11 04:49:54 +00:00
Jessica Clarke
687cb41ace Fix ArcanistFormattedStringXHPASTLinterRule on older PHP after D21500
Summary:
Calling 'Foo::bar' is only supported since PHP 7, whereas the array form
is supported since PHP 5.4, which is below our PHP 5.5 baseline.

Test Plan: No regressions under PHP 8 and snippet tested on 3v4l

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21505
2021-01-11 04:40:35 +00:00
Jessica Clarke
90ac9a2ff2 Fix ArcanistFormattedStringXHPASTLinterRule for PHP 8
Summary:
PHP 8's sprintf raises a ValueError when encountering unknown format
specifiers (previously it would eat the argument and print nothing), so
linting format strings like %Ls dies with an uncaught ValueError.

Fix this by using a custom callback during linting to turn all format
specifiers into %s and replace the dummy null argument with the original
format specifier, ensuring we always end up providing valid input to the
sprintf at the end. This has the nice property that the output of the
call to xsprintf is the original format string, though any
transformation into valid input would do.

Test Plan: Ran arc lint

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21500
2021-01-11 04:04:59 +00:00
Jessica Clarke
0adef03fdf Fix PhutilTypeSpec's regex handling for PHP 8
Summary:
In previous versions, passing the wrong type to preg_match would give a
warning that could be suppressed by @ and caught by set_error_handler,
but as of PHP 8 this raises a TypeError and so remains uncaught. Thus
check up-front whether the provided value is a string.

This fixes linting arc itself when run with PHP 8, as includes and
excludes use "optional regex | list<regex>", so would previously try to
pass an array to preg_match for the first alternative and die.

Test Plan: Ran arc lint

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21499
2021-01-11 04:04:23 +00:00
Jessica Clarke
446dcf1ccd Fix error handler on PHP 8
Summary:
PHP 7.2.0 deprecated the 5th parameter and PHP 8 removed it, so stop
using it and provide a default value to avoid erroring with:

```
Too few arguments to function PhutilErrorHandler::handleError(), 4 passed and exactly 5 expected
```

Test Plan: Used to create this revision with PHP 8 on macOS

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21498
2021-01-11 02:02:16 +00:00
Jessica Clarke
930f7e117d Suppress PHP 8 deprecation warning in __arcanist_init_script__
Summary:
As of PHP 8, the XML entity loader is disabled by default and the
libxml_disable_entity_loader function is deprecated. Thus suppress the
deprecation warning for now; we could skip the function call, but this
is safer.

Test Plan: Used to create this revision with PHP 8 on macOS

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21497
2021-01-10 22:21:29 +00:00
Jessica Clarke
3ab2b407db Remove final from private functions for PHP 8 compatibility
Summary:
This combination does not make sense and PHP 8 errors with:

```
Private methods cannot be final as they are never overridden by other classes
```

Thus remove the redundant final from all such functions.

Test Plan: Used to create this revision with PHP 8 on macOS

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21496
2021-01-10 22:05:20 +00:00
epriestley
4b3baca999 Fix a typo of "previously" in FutureIterator
Summary: Ref T13572. D21466 had a typo in a comment.

Test Plan: Read carefully.

Maniphest Tasks: T13572

Differential Revision: https://secure.phabricator.com/D21478
2020-10-16 14:23:18 -07:00
epriestley
ccf74a40dd Fix an issue where "phutil_utf8v()" could fatal when passed an integer
Summary:
See <https://discourse.phabricator-community.org/t/search-by-name-in-files-doesnt-support-number/4300>.

I can't exactly reproduce the original issue, but when a query like "quack 1234" is tokenized, we end up calling "phutil_utf8v(1234)", where the argument is an integer.

At least in recent versions of PHP, this fatals ("trying to access an offset of an integer"). Cast the argument first.

Test Plan: Searched for "quack 1234" in Files. Before: fatal accessing offset of integer; after: correct results.

Differential Revision: https://secure.phabricator.com/D21477
2020-10-16 09:22:22 -07:00
bootstraponline
04e340ab0f Fix rubocop lint tests
Summary: Fix tests to work with rubocop 0.92.0 released on September 25, 2020

Test Plan: Unit tests pass

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D21474
2020-09-30 15:19:04 +00:00
epriestley
524aa2aed2 Fix "PhutilOpaqueEnvelopeTestCase" under PHP 7.4 with "zend.exception_ignore_args"
Summary:
See PHI1894. PHP 7.4 introduced a new runtime configuration option, "zend.exception_ignore_args", which removes the "args" from exception backtraces.

The "PhutilOpaqueEnvelopeTestCase" relies on this behavior (since it explicitly inspects stack frames). Although the test isn't critical and could be restructured, it seems like there is little value to ever enabling this option in the context of Phabricator.

Disable it at startup so environments are more consistent across different PHP versions and configurations.

Test Plan:
  - Enabled "zend.exception_ignore_args" under PHP 7.4.
  - Ran "PhutilOpaqueEnvelopeTestCase".
  - Before: failure, expected signpost value not present in stack trace (because no "args" are present on the exception).
  - After: test passes.

Differential Revision: https://secure.phabricator.com/D21473
2020-09-30 07:49:25 -07:00
Paul Tarjan
7597f31b6a fail arc diff if second lfs push errors
Summary:
We are having issues where people run out of file descriptors and the first `git push` will succeed, but the
second one will not. We'd like the diff to not be created in this case as it leads to weird behavior like our tests
running against 0 changed files.

Test Plan: none

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21471
2020-09-28 16:20:56 -07:00
epriestley
a716c4e55f In "phutil_passthru()", "resolve()" the future rather than calling "execute()" directly
Summary:
See PHI1862. This code calls "execute()" on the future directly, but that skips some steps -- notably, ServiceProfiler hooks.

Call "resolve()", which has the same effect but includes desirable/expected side effects.

Test Plan: Changed a workflow to run "phutil_passthru('ls')", ran it with "--trace". Before: no execution in trace; after: execution in trace.

Differential Revision: https://secure.phabricator.com/D21470
2020-09-18 11:22:52 -07:00
epriestley
563dc2a993 In ConduitCallFuture, only call Conduit exception messages on Conduit exceptions
Summary: Ref T13582. When this code is reached with a raw HTTP exception, it currently fatals.

Test Plan:
  - Ran `arc branches --conduit-uri=http://example.org` (a bad Conduit URI).
  - Before: hard fatal with a bad method call.
  - After: non-Conduit exception raised to user. Not ideal, but a step forward.

Maniphest Tasks: T13582

Differential Revision: https://secure.phabricator.com/D21467
2020-09-17 13:20:24 -07:00
epriestley
8e5e49984d Fix a slow memory leak in long-lived FutureIterator objects, as used by FuturePool
Summary:
See T13572. FutureIterator does not release futures, so long-lived iterators (like the one that FuturePool may build) can end up leaking memory.

This affects the FuturePool used by the daemon overseer.

See T13572 for more discussion.

Test Plan:
  - Ran the simple FutureIterator script from T13572. Before: memory held during iteration, script grows without bound. After: memory released, script uses stable memory.
  - Ran the overseer with memory logging and an immediate wakeup from hibernation. Before: saw memory usage grow without bound at a rate of ~300MB/day. After: saw memory usage stable.

Differential Revision: https://secure.phabricator.com/D21466
2020-09-17 12:56:48 -07:00
epriestley
de209ec064 When raising a Conduit client exception, show the called method in the error message
Summary: Ref T13581. This message can be slightly more helpful in some cases by showing which method call failed.

Test Plan: Ran `arc branches` under the error condition in D21462, got a more useful error.

Maniphest Tasks: T13581

Differential Revision: https://secure.phabricator.com/D21463
2020-09-15 17:34:22 -07:00
epriestley
7112ee3d59 Fix additional "xsprintf()"-family static parameter errors
Summary:
Ref T13577. After the lint rule fix in D21453, it can identify more errors. Fix the errors it identifies in "arcanist/".

These all seem fairly obscure/benign.

Test Plan: Ran `arc lint` on the files before and after these changes. Did not specifically re-test these particular messages, but they mostly very obscure.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21456
2020-09-08 11:45:54 -07:00
epriestley
83e63aeb07 Allow AAST to extract string literal values from HEREDOCs
Summary:
Ref T13577. I'd like to `arc lint --everything` to find other bad calls to `pht()` and similar functions, but `n_HEREDOC` nodes currently can not generate a response to "getStringLiteralValue()".

Support literal extraction from heredocs.

Test Plan: Added a test, made it pass. Will lint everything.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21455
2020-09-08 11:45:54 -07:00
epriestley
1c327208d7 Fix a missing "pht()" parameter in HTTPSFuture
Summary: Ref T13577. This call is missing a parameter. After D21453, this is detected properly by lint. Provide the parameter.

Test Plan: Ran `arc lint` on HTTPSFuture before and after the change.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21454
2020-09-08 11:45:54 -07:00