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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Ref T13577. This lint rule correctly detects the error in `pht('x %s y')` but the narrow test for `n_STRING_SCALAR` prevents it from detecting the error in `pht('x %s y'.'z')`.
Make the test broader.
Test Plan:
- Ran `arc lint` on `HTTPSFuture.php`, got a detection of the issue in T13577.
- Added a failing test and made it pass.
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21453
Summary:
Ref T13555. Currently:
- If an exception is raised in "start()", the exception state is not set on the future.
- Futures do not always call "startFuture()" before starting, and do not always call "endFuture()" once they become resolvable.
- If you start an ExecFuture which immediately fails and then call "getPID()" on it, you get an unclear exception.
Simplify these behaviors:
- In FutureIterator, only start futures which have not already started.
- When starting a future on any pathway, run start code.
- When a future becomes resolvable on any pathway, run end code.
- Raise a more clear exception when calling "getPID()" on a future with no subprocess.
Test Plan: Faked a failing subprocess with "$proc = null", ran "bin/phd debug taskmaster" etc. Got clearer errors and more consistent future lifecycle workflows.
Maniphest Tasks: T13555
Differential Revision: https://secure.phabricator.com/D21423
Summary:
See PHI1808. Currently, "arc land <some bookmark>" does not destroy the bookmark in Mercurial. There are three issues here:
- "hg rebase --keep --collapse" moves bookmarks to the rewritten commits;
- "hg strip" moves bookmarks backwards;
- "hg prune" moves bookmarks backwards.
To get around "hg rebase", save and restore bookmark state.
To get around "hg strip" and "hg prune", explicitly destroy bookmarks pointing at commits before we strip/prune those commits.
Test Plan:
- Ran "arc land <some bookmark> --trace". Saw arc reset the bookmark position after rebasing, and destroy the bookmark explicitly before stripping.
- When the workflow exited, saw no more bookmark (previously: bookmark existed and pointed at a possibly-intermediate state).
Differential Revision: https://secure.phabricator.com/D21397
Summary:
Ref PHI1808. In Mercurial, we must save and restore bookmark state explicitly.
- Save and restore bookmarks.
- Clean up concepts in "arc-ls-markers" slightly, so we don't need separate "isCurrent" and "isActive" flags, hopefully.
I believe the totality of Mercurial state is:
- A (non-bare) working copy points at exactly one commit (which might be the empty/null commit, in an empty repository).
- A working copy has exactly one active branch.
- Each branch has zero or more heads.
- Each head may be closed.
- Each (non-null) commit belongs to exactly one branch.
- Note that the active branch may have zero heads and zero commits which belong to it!
- A working copy has zero or one active bookmark.
To capture this, we now emit:
- A list of branch heads. If a branch head is a working copy commit, that head is flagged as active.
- A list of bookmarks. If a bookmark is the current bookmark, that bookmark is flagged as active.
- A single "branch-state" virtual marker. This covers the case where you have run "hg branch X" to create X, but no objects in the working copy actually correspond to X yet. It also covers the case where you are on a concrete branch, but not any head of that branch.
- A single "commit-state" virtual marker. This always shows the current commit in the working copy.
Test Plan:
- Useful states to test are:
- Empty repository (not all commands currently work here).
- Normal repository, on a bookmark.
- Normal repository, no bookmark.
- "hg up 123" to update to somewhere in history.
- "hg branch X", to start a new branch with no commits.
- Ran "arc branches" and "arc bookmarks" in various states. Saw generally sensible output.
- Ran "arc land --hold ..." in various states against a failing remote. Saw generally sensible output, and saw working properly restored to the original state.
Differential Revision: https://secure.phabricator.com/D21396
Summary: Ref PHI1808. Currently, push failures are messaged awkwardly. Make this exception handling more selective and the user-facing behavior more readable.
Test Plan: Ran "arc land" against a failing remote, saw a human-readable message instead of a stack trace.
Differential Revision: https://secure.phabricator.com/D21395
Summary: See PHI1808. Some refactoring of the "passthru" API resulted in error conditinos here being dropped. Instead, raise them as exceptions.
Test Plan: Forced "hg push" to fail, used "arc land" against a failed push, saw error behavior instead of "success" feedback.
Differential Revision: https://secure.phabricator.com/D21394
Summary:
Ref PHI1805. Under some combination of versions (Python 3.8?), "arc-ls-markers" is running into additional Python runtime issues.
Sprinkle more "b" around to resolve them? Also clean up a couple of plain "arc" issues.
Test Plan:
Landed a change in Mercurial.
Some of this works fine without changes in Python 3.7/2.7 against Mercurial 4.7/5.4, so this may not be exhaustive.
Differential Revision: https://secure.phabricator.com/D21393
Summary:
Ref T13546. See PHI1805. Currently, the "arc-ls-markers" extension doesn't run under Python 3:
- some stuff needs "b'...'" to mark it as a byte string;
- "dict.iteritems()" is gone in Python 3, and "mercurial.pycompat" isn't always available;
- in Python 3, "json" refuses to print byte strings; and
- the compiler caching behavior in Python 3 has changed.
Try to get these things working in the same way under Python 2 and Python 3.
Test Plan:
Ran this command (with `python` as Python 2, locally):
```
$ python /usr/local/bin/hg --config 'extensions.arc-hg=/Users/epriestley/dev/core/lib/arcanist/support/hg/arc-hg.py' arc-ls-markers --
```
...and this command:
```
$ python3 /usr/local/bin/hg --config 'extensions.arc-hg=/Users/epriestley/dev/core/lib/arcanist/support/hg/arc-hg.py' arc-ls-markers --
```
..and saw the same output in both cases (previously, `python3 ...` fataled in various ways).
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21392
Summary: See PHI1805. This call is constructed improperly and can lead to a fatal in `arc patch` under Mercurial.
Test Plan: In Mercurial, ran a valid `arc patch` operation.
Differential Revision: https://secure.phabricator.com/D21391
Summary:
Ref T13546. Try using unicode box drawing characters to render a more obvious tree struture in "arc branches".
Unclear if this has enough support to use, but seems okay so far.
Test Plan: Ran "arc branches", saw a nicer tree display.
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21390
Summary:
Correct two minor Conduit future issues:
- FutureAgent shows up in the trace log as "<mystery>". Since it isn't doing anything useful, solve the mystery and drop it from the log.
- Simply the ConduitFuture code for interacting with the service profiler now that a more structured integration is available.
Test Plan: Ran "arc branches --trace", saw conduit calls and no more "<mystery>" clutter.
Differential Revision: https://secure.phabricator.com/D21388
Summary:
See PHI1802. After D21384, "arc land" and similar with no credentials now properly raise a useful exception, but it isn't formatted readably.
Update the display code to make it look prettier.
Test Plan: Ran "arc land" with no and invalid credentials, got properly formatted output.
Differential Revision: https://secure.phabricator.com/D21387
Summary:
Currently, modern "arc" workflows accept and parse "--anonymous" but don't do anything with it.
I intend to move away from anonymous workflows, which only really supported a tiny subset of unusual workflows on open source installs but added significant complexity to login/auth behavior.
Drop support for this flag.
Test Plan: Grepped for "anonymous", didn't turn up any relevant hits.
Differential Revision: https://secure.phabricator.com/D21386
Summary:
Currently, if you type "arc upload <tab>", we do not autocomplete the working directory. We should, but the "current argument" is the empty string and that's technically a prefix of every flag, so we suggest that you might want a flag instead.
You probably don't. Suggest paths in this case.
Test Plan:
- Ran "arc upload <tab>", saw path completions.
- Ran "arc land <tab>" (this workflow does NOT take paths as arguments), saw flag completions.
Differential Revision: https://secure.phabricator.com/D21385
Summary:
See PHI1802. Currently, we can't raise a "you must login" error in a generic way at the beginning of a workflow because we don't know if a workflow needs credentials or not.
For example, "arc help" does not need credentials but "arc diff" does.
Additionally, some actual Conduit calls do not need credentials ("conduit.ping", "conduit.getcapabilities") and others do.
Although I'd like to simplify this eventually and move away from anonymous/unauthenticated "arc", this isn't trivial today. It's also possible for third-party code to add authenticated calls to "arc help", etc., so even if we could execute these tests upfront it's not obvious we'd want to.
So, for now, we raise "you must login" at runtime, when we receive an authentication error from Conduit.
This got implemented for Toolsets in a well-intentioned but not-so-great way somewhere in wilds/experimental, with an "ArcanistConduitCall" that behaves a bit like a future but is not really a future. This implementation made more sense when ConduitEngine was serving as a future engine, and FutureProxy could not rewrite exceptions.
After the Toolsets code was first written, ConduitEngine has stopped serving as a future engine (this is now in "HardpointEngine"). Since HardpointEngine needs a real future, this "show the user a login message" code gets bypassed. This results in user-visible raw authentication exceptions on some workflows:
```
[2020-06-30 21:39:53] EXCEPTION: (ConduitClientException) ERR-INVALID-SESSION: Session key is not present. at [<arcanist>/src/conduit/ConduitFuture.php:76]
```
To fix this:
- Allow FutureProxy to rewrite exceptions (see D21383).
- Implement "ArcanistConduitCall" as a FutureProxy, not a future-like object.
- Collapse the mixed-mode future/not-quite-a-future APIs into a single "real future" API.
Test Plan:
- Created a paste with "echo hi | arc paste --".
- Uploaded a file with "arc upload".
- Called a raw method with "echo {} | arc call-conduit conduit.ping --".
- Invoked hardpoint behavior with "arc branches".
- Grepped for calls to either "resolveCall()" method, found none.
- Grepped for calls to "newCall()", found none.
- Grepped for "ArcanistConduitCall", found no references.
Then:
- Removed my "~/.arcrc", ran "arc land", got a sensible and human-readable (but currently ugly) exception instead of a raw authentication stack trace.
Differential Revision: https://secure.phabricator.com/D21384
Summary:
See PHI1764. See PHI1802. Address two resolution behaviors for FutureProxy:
- FutureProxy may throw an exception directly from iteration via "FutureIterator" (see PHI1764). This is wrong: futures should throw only when resolved.
- FutureProxy can not change an exception into a result, or a result into an exception, or an exception into a different exception. Being able to proxy the full range of result and exception behavior is useful, particularly for Conduit (see PHI1802).
Make "FutureProxy" more robust in how it handles exceptions from proxied futures.
Test Plan:
Used this script to raise an exception during result processing:
```
<?php
require_once 'support/init/init-script.php';
final class ThrowingFutureProxy
extends FutureProxy {
protected function didReceiveResult($result) {
throw new Exception('!');
}
}
$future = new ImmediateFuture('quack');
$proxy = new ThrowingFutureProxy($future);
$iterator = new FutureIterator(array($proxy));
foreach ($iterator as $resolved) {
try {
$resolved->resolve();
} catch (Exception $ex) {
echo "Caught exception properly on resolution.\n";
}
}
```
Before this change, the exception is raised in the `foreach()` loop. After this change, the exception is raised at resolution time.
Differential Revision: https://secure.phabricator.com/D21383
Summary: This method is private and has no callers. The code has moved to "FileUploader" in a prior change.
Test Plan: Grepped for callers, found none.
Differential Revision: https://secure.phabricator.com/D21382
Summary: Ref T13546. Fixes some issues where marker selection in Mercurial didn't work, and selects "draft()" as the set of commits to show, which is at least somewhat reasonable.
Test Plan: Ran "arc branches" and "arc bookmarks" in Mercurial, got more reasonable output.
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21380
Summary: Ref T13546. Allow the commit graph to be queried by date range, and Git to be queried for multiple disjoint commits.
Test Plan: Ran "arc branches" and future code which searches for alternate commit ranges for revisions.
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21379
