Summary:
Depends on D21074. Ref T13490. Ref T11968. Before toolsets, Arcanist has a "WorkingCopyIdentity" object. After toolsets, it has a "WorkingCopy" object.
Most workflows don't access either one, so make a slightly-breaking API change to simplify the transition.
- "getWorkingCopy()" now returns a modern object (a "WorkingCopy").
- "getWorkingCopyIdentity()" now returns an older object (a "WorkingCopyIdentity").
This isn't backward-compatible, but out-of-date code should get an explicit failure with clear resolution steps.
Test Plan: Ran "arc lint", "arc branch", and a few other commands. Grepped for "getWorkingCopy()".
Maniphest Tasks: T13490, T11968
Differential Revision: https://secure.phabricator.com/D21075
Summary:
Ref T11968. Ref T13490. These are the workflows which currently use the intermediate-level hardpoint code (which made hardpoints formal, but didn't do the yield stuff).
Move toward updating them by doing some basic bookkeeping, with a few compatibility adjustments to the parent Workflow class.
Test Plan: Ran "arc branch" and "arc browse" with various arguments.
Maniphest Tasks: T13490, T11968
Differential Revision: https://secure.phabricator.com/D21074
Summary:
This reverts commit 7e25288f49. See T13507 for discussion, shortly.
This needs to be switched to capability detection because some server configurations can not actually accept these requests.
Test Plan: Straight revert.
Differential Revision: https://secure.phabricator.com/D21076
Summary: Ref T13507. Enable compression in the body of Conduit requests if we have client support for it; we expect the server should always support it.
Test Plan: Created this revision, ran random "arc" commands that use Conduit. Added debugging code, saw payload size drop.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21073
Summary:
Depends on D21071. Ref T11968. Currently, "ConduitEngine" tries to lightly parallelize futures. This was a compromise when the initial "hardpoint" change didn't plan to pursue real request paralleization.
Now that the newer hardpoint change does, we don't need onboard resolution in ConduitEngine. Throw it away.
When the engine is supposed to resolve a future, it now just resolves that future on its own. This should be functionally identical to the previous behavior, except that it may be slower.
(In practice, because HTTP futures are backed by an internal cURL request pool, this proably has little effect anywhere. Moving to modern hardpoints will make performance no worse than it was prior to this change, in any case.)
Test Plan: Ran various modern "arc" commands.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21072
Summary:
Depends on D21070. Ref T11968. Adds "yield"-aware query classes for parallelizing service calls.
These will replace the similar (but not yield-aware) "Hardpoint" classes introduced previously. This is an API change but most of the old classes still exist and still do the same thing, just with more "yield" statements.
This just adds a bunch of new code with no callers and no API updates.
Test Plan: See future changes.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21071
Summary:
Ref T11968.
- Allow "WorkingCopy" objects to maintain an API object and update callers ("get...()" instead of "new...()").
- Always generate a WorkingCopy object and a RepositoryAPI object.
Currently, code has to look like this:
```
$working_copy = ...
if ($working_copy) {
$repository_api = ...
if ($repository_api [instanceof ... ]) {
```
This is clunky. There's also no reason some "arc" commands can't run outside a VCS working directory without special-casing how they interact with the filesystem.
Conceptually, model the filesystem as a trivial VCS (which stores exactly one commit, always amends onto it, and discards history). Provide a trivial WorkingCopy and API for it.
(This change isn't terribly interesting on its own, but chips away at landing the new Hardpoint infrastructure.)
Test Plan: Ran `arc version`, `arc upgrade`.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21070
Summary:
Depends on D21067. Ref T13492. Converting unit tests to be readable exposed this error in the grammar.
Normally, "grammar_rule" rules emit a standalone Node. In this case, the bottom-level grammar rule is a collection of trivial rules and the callers configure the Node. Wrap the bottom-level rule in a configuration rule so the node is configured correctly and consistently, in exactly one place.
Test Plan: Ran unit tests.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21068
Summary:
Depends on D21066. Ref T13492. The switch of unit test data to stable/readable output exposed this bug in parsing of variadic calls: some nodes are not given types properly.
Fix the parser and update the test.
Test Plan: Ran the test, which now works.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21067
Summary:
Depends on D21065. Ref T13492. Swap existing "expect" blocks from unstable, unreadable JSON to readable, stable trees.
(There are two "INVALID TYPE" outputs which this update effectively detects and which future changes correct.)
Test Plan: Ran "arc unit --everything", got a clean build.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21066
Summary:
Depends on D21064. Ref T13492. Earlier, see D17819. This is essentially the same change, although I inlined the token stream into the node list.
This intentionally breaks most tests since it just has the new "expect" generator; the next change will fix them by swapping the test bodies.
Test Plan: Ran "arc unit --everything" after the next change (which fixes all the tests), got a clean build. This change on its own fails all existing XHPAST tests since the block formats don't match.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21065
Summary: Ref T13490. The new Arcanist runtime supports workflow signal handling, but Phage isn't quite able to make use of it. Clean up the last few pieces so it can work.
Test Plan: Ran "phage", hit ^C, got status information.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21060
Summary: Ref T11968. Phage has another "sustained pool of Futures" use case, and needs some slight adjustments after Future API changes.
Test Plan: Ran `bin/phage status ...`, got a clean result instead of a JSON decoding failure.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21058
Summary:
Ref T11968. "FutureIterator" recently became non-rewindable, and starting a Future twice is now an error.
This complicates a handful of use cases where a mostly-constant pool of futures is maintained over a long period of time, notably in daemon overseers and repository pull daemons.
They previously relied on being able to do "new FutureIterator($futures)" to continue resolution of a list of futures from any state. This no longer works quite like it used to, since Futures generally may not belong to more than one iterator now (and this property is desirable).
Introduce "FuturePool", which maintains exactly one iterator but manages the small amount of glue around adding and removing Futures from it, destroying it if the pool empties, and rebuilding it if the pool fills.
Test Plan: See next change, which makes use of this.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21053
Summary:
Depends on D21051. Ref T13504. Ref T13209. This very old Mercurial code uses "X=Y hg ..." on Linux and "set X=Y & hg ..." on Windows.
The latter construct no longer works because we bypass the shell. The former construct is obsolete.
Additionaly, delete some ancient "branch merge" code which has no callers.
Test Plan: Created a diff in a Mercurial repository on Linux. I minimally vetted this on Windows since I don't have a "hg + Windows" environment at the moment.
Maniphest Tasks: T13504, T13209
Differential Revision: https://secure.phabricator.com/D21052
Summary:
Ref T13504. Ref T13209. We currently use "escapeshellarg()" under Windows, which destructively replaces "%" symbols with spaces. This is wrong, breaks general behavior with "git log --format=...", and has led to a lot of weird workarounds in `arc` code where we don't escape arguments we should be escaping to tiptoe around issues with "%".
Now that execution occurs via "bypass_shell", we can safely (probably?) escape things, although programs are still apparently free to parse the command string however they want.
Test Plan: Added unit tests, ran them under Mac and Windows, got clean results. Ran "arc version" on Mac and Windows.
Maniphest Tasks: T13504, T13209
Differential Revision: https://secure.phabricator.com/D21051
Summary:
Ref T13504. The API has changed here slightly, and if you run "arc version" without "arcanist/" being a Git working copy, it currently fatals in a misleading way.
Instead, reach the error properly.
Test Plan: Ran "arc version" after moving aside ".git/", got a helpful error message instead of a confusing "call on null" exception.
Maniphest Tasks: T13504
Differential Revision: https://secure.phabricator.com/D21050
Summary:
Fixes T13504. This fixes three issues:
# In ExecFuture, "proc_open()" on an invalid binary could fail with an unconditional exception.
# In ExecPassthru, "proc_open()" on an invalid binary could fail with an unconditional exception.
# In "arc browse", "start <url>" does not work when the shell is bypassed.
In (1) and (2), the desired behavior is to fail with an exit code which is sometimes upgraded to an exception depending on calling convention.
Issue (1) most commonly manifested as "find" failing when run via "cmd.exe".
Issue (2) most commonly manifested as "arc browse" failing.
Issue (3) was entangled with issue (2).
In cases (1) and (2), assume "proc_open()" failures under Windows are because of bad binaries and treat them like bogus commands on Linux/Mac.
In case (3), use "cmd /c start" instead of "start" as a default browser on Windows.
Test Plan:
- On Windows, did mime type detection in cmd.exe. Before patch: proc_open() exception in "find". After patch: clean (albeit not terribly useful) detection.
- On Windows, did "arc browse ...". Before patch: proc_open() exception in "start". After patch: clean browser execution.
Maniphest Tasks: T13504
Differential Revision: https://secure.phabricator.com/D21047
Summary:
Ref T11968.
- Rekeying natural lists of futures creates more problems than it solves. For now, don't try to deal with this.
- The scope of the future autokey was incorrect (per subclass, not per process).
Test Plan: Ran various future-oriented flows on new HardpointEngine code.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21046
Summary:
Depends on D21036. Ref T11968. Ref T13177. Currently, each Future integrates separately with ServiceProfiler, but much of the code is similar.
Move integration to the base class and lift up most of the implementation details.
Test Plan: Ran `arc diff --trace`, saw sensible output.
Maniphest Tasks: T13177, T11968
Differential Revision: https://secure.phabricator.com/D21038
Summary: Depends on D21035. Ref T11968. This allows a "FutureIterator" to hold futures which are blocked because of unresolved dependencies, and makes the resolution process more structured.
Test Plan: Ran unit tests, created this revision.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21036
Summary: Depends on D21033. Ref T11968. This is just an incremental step in modernizing Future and making it more robust. Currently, subclasses are expected to write directly to `$this->result`, but this isn't consistent with how modern classes generally work.
Test Plan: Ran unit tests, created this revision.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21034
Summary:
Depends on D21032. Ref T11968. Currently, "Future" and "FutureIterator" can both resolve futures.
Treat "Future->resolve()" as sugar on resolving an iterator of size 1.
Test Plan: Ran tests, created this revision.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21033
Summary:
Ref T11968. This future-level parameter has no nontrivial callers and makes the "fate of FutureGraph" changes more difficult.
Callers that are genuinely interested in this behavior can wrap the Future in a FutureIterator and use "setUpdateInterval()" to get the same behavior.
Test Plan: Grepped for "resolve()" and "resolvex()", updated callers.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21031
Summary:
One of our users ran into an issue where they were running `arc diff` in
a subdirectory and it so happened that in that directoy there wasn't any `lfs`
files so the repo was marked as `$is_lfs == false`. Lets allow `arc` to be run
from anywhere.
Test Plan: Internally this patch worked
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21045
Summary:
Ref T13499. Currently, we throw most language errors as exceptions, but this list isn't exhaustive and errors we don't specifically make more severe are allowed to slip through.
This is generally undesirable, particularly in the case of "Undefined index:" errors. See T13499 for a specific case where this caused behavior to be more difficult to understand and diagnose than it should have been.
Make this the default behavior instead, except for "E_USER" errors, which we never expect to arise from first-party code.
This may be slightly too aggressive, but future changes can selectively reduce the severity of some types of errors if problems arise.
Test Plan:
- Executed code which intentionally accessed an undefined index, got an exception.
- Poked around Phabricator and Arcanist without any further issues cropping up, but I don't have a good way to develop confidence that the "reduced severity" list should genuinely be empty.
Maniphest Tasks: T13499
Differential Revision: https://secure.phabricator.com/D21044
Summary: Fixes T13498. Currently, `arc diff` may invoke the file upload flow using an older variation of the API. Both the modern and fallback clients build an Engine, so switching to the modern API is trivial.
Test Plan:
- Created a local commit with a 1MB binary file.
- Ran `arc diff --only`.
- Before patch: fatal on old API call.
- After patch: clean upload.
Maniphest Tasks: T13498
Differential Revision: https://secure.phabricator.com/D21043
Summary:
I'm working on a fairly large monorepo using phabricator and we are
running into a `git-lfs` issue. Every single `arc diff` we have to wait
many seconds while all our lfs files are walked before the push goes through.
I chatted with the `git-lfs` folks in
https://github.com/git-lfs/git-lfs/issues/4076 and they suggested this workflow
change. I tested it on our repo and it does indeed seem to fix the issue. Three
things were change:
1. The remote url is named
2. Use branches instead of tags
3. Do a `git fetch`
(Also, hello after all this time! Hope you're all doing well over at phacility.)
Test Plan:
Before
```
$ arc diff
Linting...
LINT OKAY No lint problems.
Running unit tests...
UNIT OKAY No unit test failures.
PUSH STAGING Pushing changes to staging area...
Uploading LFS objects: 100% (8211/8211), 1.0 GB | 0 B/s, done.
Enumerating objects: 31, done.
Counting objects: 100% (31/31), done.
Delta compression using up to 12 threads
Compressing objects: 100% (21/21), done.
Writing objects: 100% (24/24), 1.77 KiB | 8.00 KiB/s, done.
Total 24 (delta 19), reused 0 (delta 0)
remote: Resolving deltas: 100% (19/19), completed with 6 local objects.
To github.com:robinhoodmarkets/web-staging.git
* [new tag] 2581578b59b6341c1023377fd8741814e89fcd0c -> phabricator/base/427095
* [new tag] 7d9be50fbca590f15742d8cc20edd4f082dfbb3b -> phabricator/diff/427095
Updated an existing Differential revision:
Revision URI: https://phabricator.robinhood.com/D135442
Included changes:
M __shared/tools/arcanist-js/src/workflow/RHArcanistDiffWorkflow.php
```
After
```
$ arc diff
Linting...
LINT OKAY No lint problems.
Running unit tests...
UNIT OKAY No unit test failures.
PUSH STAGING Pushing changes to staging area...
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 12 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 638 bytes | 2.00 KiB/s, done.
Total 8 (delta 6), reused 0 (delta 0)
remote: Resolving deltas: 100% (6/6), completed with 6 local objects.
To github.com:robinhoodmarkets/web-staging.git
* [new branch] 2581578b59b6341c1023377fd8741814e89fcd0c -> phabricator/base/427105
* [new branch] 274cc3a25eb7856291c1d4c078b28b76af8a43b2 -> phabricator/diff/427105
Updated an existing Differential revision:
Revision URI: https://phabricator.robinhood.com/D135442
Included changes:
M __shared/tools/arcanist-js/src/workflow/RHArcanistDiffWorkflow.php
```
(notice the lack of `Uploading LFS objects`)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21041
Summary:
Depends on D21031. Ref T11968. This clears some lint with the word "Future", but is just a minor cleanup patch not really related to the main goal in that task.
We currently warn on `preg_quote()` with no second parameter, but this is valid and correct:
```
preg_match('('.preg_quote($x).')', ...)
```
This is the modern recommended style for this sort of expression, so the warning is often a false positive; drop it.
The "__CLASS__" warning looks for hard-coded class names in strings, but currently looks for them as a word anywhere in the string.
This is often a false positive and sometimes makes error messages or exceptions untranslatable. For example, translators can't do anything with this:
> Filesystem path "%s" does not exist.
...if it's written as:
> %s path "%s" does not exist.
...just because it happens to appear in the class "Filesystem". Some other classes, including "Future", suffer from this.
Even when the detection is correct, it's clunky and a mistake here (failing to update the class name in an error message) is unlikely to ever do any real damage.
Test Plan: Ran unit tests and `arc lint`.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21032
Summary:
See PHI1663. If "phabricator.uri" is not configured, we try to fall back to "default", but doesn't have a modern config specification and fails.
Instead, read "default" as a raw config value to preserve compatible behavior. My intent is to eventually remove it.
Test Plan:
In a directory with no "phabricator.uri" config available, ran `echo {} | arc call-conduit conduit.ping`. After the patch, got a reasonable error instead of a fatal.
In a directory with "default" configured but not "phabricator.uri", ran the same command. After the patch, got a ping.
Maniphest Tasks: T13497
Differential Revision: https://secure.phabricator.com/D21039
Summary: Ref T13490. This largely makes "arc upload" work, although the Future stuff is still a bit wonky. See T11968.
Test Plan: Ran "arc upload README.md", got an upload.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21030
Summary: Ref T13490. This fixes one bug in D21025 (loading relative to "arcanist/" rather than the parent directory) and improves behavior when a library fails to load (we prompt the user to continue).
Test Plan:
- Ran `arc list` with bad library specifications, got a sensible error and an option to continue.
- Ran `arc list` with a library specification that existed next to "arcanist/", got a library load instead of an error.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21029
Summary: Ref T13490. This makes the "phage" toolset work properly and updates external library behavior to match the "classic" behavior, except that "--library" is now supported.
Test Plan: Ran "phage remote" workflows.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21025
Summary:
Ref T13490. There's one simple "implode()" order issue here, and one slightly more complex one that uses "DIRECTORY_SEPARATOR" as glue.
Add test coverage for this, update the lint check to detect constants used as glue, and fix the callsites.
Test Plan:
- Added a failing test and made it pass.
- Ran `arc lint --everything` and looked for remaining implode warnings, found none.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21024
Summary: Ref T13490. Allows "arc upgrade" to run through the new Toolsets infrastructure.
Test Plan: Ran "arc upgrade", although you can't upgrade feature branches so this can't be entirely tested until it hits "master".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21006
Summary: Depends on D21004. Ref T13490. The "wilds" branch removed a bunch of dead features but some of them were resurrected by the merge. I don't think any of these are desirable to retain, so purge them all again, even in classic mode.
Test Plan: Grepped for affected symbols.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21005
Summary:
Depends on D21001. Ref T13490.
- Require "--" to terminate arguments when running noninteractively.
- Don't correct spelling when running noninteractively (we still suggest corrections).
- Remove old "phage" wrapper script.
- Fix an issue where "argv" could implicitly generate a parseable "--argv" flag.
- Accept "--" as an argument terminator even when there is no argument list.
- Update some strings to use more modern quoting style.
- Make workflows decline to handle signals by default.
- Allow "arc weld" to weld non-UTF8 files together.
Test Plan: Ran various commands. Welded non-UTF8 files.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21002
Summary: Ref T13490. Update the "weld" workflow and the "anoid" workflow. Incorporates D20938.
Test Plan: Ran "arc weld". Ran "arc anoid".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21001
Summary: Depends on D20996. Ref T13395. Ports the updated version of this workflow from "experimental".
Test Plan: Ran `arc shell-complete` to install the hook, then shell-completed commands.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20997
Summary: Depends on D20993. Ref T13395. Merges the more-modern "alias" out of "experimental".
Test Plan: Defined and invoked aliases. This has some rough edges: notably, no easy list/delete flow.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20996
Summary: Ref T13395. Depends on D20991. Make "arc liberate" run as a toolset command, not a classic command.
Test Plan: Ran "arc liberate"; created this diff.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20992
Summary: Ref T13395. Make "arc help" run as a toolset command, not a classic command.
Test Plan: Ran "arc help".
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20991
Summary:
Depends on D20988. Ref T13395. Ref T13098. Ref T13203.
This brings all the "toolsets" code into "master". We try to run commands as toolsets commands first, then fall back to older code.
Since the "toolsets" class tree is mostly parallel to the older class tree, this isn't completely broken. Currently, all commands fall back.
Test Plan: Created this diff, ran various other commands. But this is probably a long shot from finished.
Maniphest Tasks: T13395, T13203, T13098
Differential Revision: https://secure.phabricator.com/D20990
Summary:
Ref T13395. Merge a lot of stuff which doesn't break existing workflows:
- Merge a UTF8 fix for "cmd.exe" on Windows.
- Merge minor changes to JSON linters.
- Merge some shell completion stuff.
- Merge some "arc anoid" fixes.
- Merge various Windows improvements to unit tests which interact with processes / the filesystem.
- Merge some other Windows path fixes.
- Merge a UTF8 character class fix.
- Merge script initialization.
- Merge unit test support scripts.
- Merge some initialization code.
- Merge Windows stdout/stderr-as-files code.
- Merge a bunch of code for making exec tests work on Windows.
- Merge more Windows unit test fixes.
- Merge "continue on failure" mode when loading symbols.
- Merge "GPC" order CLI fixes.
Test Plan: Ran `arc unit --everything`; created this change. There's likely some less-than-perfect code here.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20988
Summary: Fixes T13489. See that task for discussion.
Test Plan: Ran `arc lint` and `arc lint --output xml`. Faked a missing extension, ran `arc lint` (no problems) and `arc lint --output xml` (sensible error message).
Maniphest Tasks: T13489
Differential Revision: https://secure.phabricator.com/D20989
Summary:
Depends on D20986. Ref T13395. This //mostly// collapses the entire "experimental" branch into "master".
I plan to change the "Ref/Hardpoint" pattern to become future oriented, but this is more steps forward than sideways.
Test Plan: Ran various `arc` workflows.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20987
Summary: Ref T13395. Collapse changes to the linter workflow from "experimental" into "master".
Test Plan: Ran `arc lint`. Noted flag changes in changelog.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20986
Summary: Ref T13010. This brings the "--draft" flag (and a handful of related flags) to "master" from "experimental".
Test Plan: Ran "arc diff". This code has been in use on "experimental" for a long time.
Maniphest Tasks: T13010
Differential Revision: https://secure.phabricator.com/D20985
Summary: Ref T13395. Fixes a file-locking test and removes a logging test that's more trouble than it's worth.
Test Plan: Ran "arc unit --everything" locally, got a clean bill of health.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20984
Summary: Ref T13395. Currently, "phage" is required for various cluster operations. Bring the working code out of "experimental". This isn't the final form; "wilds" has a fancier version.
Test Plan: Ran phage workflows against the cluster.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20982
Summary: Ref T13395. Moves all remaining code in "libphutil/" into "arcanist/".
Test Plan: Ran various arc workflows, although this probably has some remaining rough edges.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20980
Summary:
Ref T13395. This code relies on PhutilHTML, but that's moving to Phabricator.
It has no callers in libphutil or Arcanist.
Test Plan: Looked for callers.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20978
Summary: Ref T13481. Some older versions of Git appear to not support "--" in these commands. Just drop it. This can lead to ambiguous results with certain obviously-silly remote names, but doesn't appear to lead to anything dangerous.
Test Plan: Will followup with user on ancient Git.
Maniphest Tasks: T13481
Differential Revision: https://secure.phabricator.com/D20952
Summary: Fixes T13481. When identifying the URI for a remote, fall back from "git remote get-url" to "git ls-remote --get-url" to "git config remote.<name>.url" based on command output and version tests.
Test Plan: Ran `arc land --hold`, rigged the subcommands to fail to try all three fallbacks, ran `arc land --hold --remote asdfasdf` to get an explicit failure.
Maniphest Tasks: T13481
Differential Revision: https://secure.phabricator.com/D20950
Summary: Fixes T13458. Emit an explicit message when "arc close-revision --finalize" bails out because the revision is not "Accepted".
Test Plan: Ran `arc close-revision [--finalize] ...` on various revisions, saw more clear messaging.
Maniphest Tasks: T13458
Differential Revision: https://secure.phabricator.com/D20915
Summary:
Ref T13432. Git has a "diff.suppressBlankEmpty" config option which makes it emit nonstandard diffs with trimmed trailing whitespace on unchanged blank lines.
Currently, we don't parse these diffs correctly. Even if we do in the future, emitting a more standard diff is desirable.
Explicitly disable this option when executing "git diff" so we build more standard diffs.
Test Plan:
- Configured this option.
- Modified a file with a blank line in it without changing the blank line, got this goofy display diff:
{F6985234}
- Applied patch, rediffed the same change, saw "-c diff.suppressBlankEmpty" in "--trace" output and got this sensible diff:
{F6985235}
Maniphest Tasks: T13432
Differential Revision: https://secure.phabricator.com/D20877
Summary: Ref T13434. Since "git p4 submit" gets more complicated when submitting merges, and empty merges (as with "--no-ff") seem to vanish, and it's not clear this is desirable or useful anyway, just make the "merge" strategy an explicit error with Perforce remotes.
Test Plan: Ran "arc land --merge ..." in a Git/Perforce repository, got an explicit error. Ran "arc land --squash ...", got existing working behavior.
Maniphest Tasks: T13434
Differential Revision: https://secure.phabricator.com/D20871
Summary:
Fixes T10650. It's valid to `arc land --remote origin --onto master` without first fetching that ref. If we can't find a local ref for a specified remote branch, try to fetch it before giving up.
(In the long run, this should be valid even if the remote branch does not exist at all and the user intends to create it -- see T12876 -- but this is a step toward that.)
Test Plan:
- Ran `rm .git/refs/remotes/origin/master`, then landed into "master".
- Before: "arc land" bailed out immediately.
- After: "arc land" fetches the missing ref.
```
$ arc land
TARGET Landing onto "master", the default target under git.
REMOTE Using remote "origin", the default remote under Git.
TARGET No local ref exists for branch "master" in remote "origin", attempting fetch...
FETCHED Fetched branch "master" from remote "origin".
...
```
Maniphest Tasks: T10650
Differential Revision: https://secure.phabricator.com/D20870
Summary: Ref T13434. Move some git-engine-specific handling of "arc land" arguments into the Git engine. This prepares to handle Perforce workflows.
Test Plan: Will "arc land" this change.
Maniphest Tasks: T13434
Differential Revision: https://secure.phabricator.com/D20867
Summary:
Ref T13428. Historically, "implode()" accepts arguments in either order. PHP 7.4+ warns about glue not being first.
Add a lint check when the second parameter is a static scalar, implying it is the glue parameter.
Test Plan: Ran unit tests. See next change.
Maniphest Tasks: T13428
Differential Revision: https://secure.phabricator.com/D20857
Summary:
Fixes T10321. Some reasonable but less-common workflows involve running `arc land` from a detached HEAD state.
When users do this, we currently try to delete the raw hash as though it were a branch during cleanup. Instead, detect if the thing we're thinking about deleting is a branch or not, and just leave it alone if it isn't.
Test Plan:
- Ran `git checkout <some hash>`, then `arc land --revision <some revision>`.
- Before, everything worked but cleanup tried to `git branch -D <some hash>`.
- After, everything worked and cleanup skipped branch deletion.
Maniphest Tasks: T10321
Differential Revision: https://secure.phabricator.com/D20786
Summary:
The current code assumes git-svn is always working from a remote called
`trunk`, but if the repository is initialized without the `-T` option it
will instead be called `git-svn`, and if `--prefix` is used (which is
set by default to `origin/` in Git 2+) the remote name will have the
specified prefix as well.
Instead, look at the `fetch` target refspec set in the git-svn config.
Fixes T13293.
Test Plan:
`arc land` without errors (or manually creating a `trunk` branch) from a
checkout made with Git 2.18.0 (verified this manually on a non-`-T`
checkout as well).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13293
Differential Revision: https://secure.phabricator.com/D19681
Summary:
Fixes T6854. The current format for `lint-test` files is somewhat inflexible and does not allow us to make assertions regarding the code or name of the linter messages (of class `ArcanistLintMessage`) that are raised. Specifically, the `${severity}:${line}:${char}` format is hardcoded in `ArcanistLinterTestCase`. In this diff, I extend the this format to achieve the following goals:
- Allow for the lint message code and name to be specified. Specifically, the full format is `${severity}:${line}:${char}:${code}:${name}`.
- Make all fields optional. `error:3:` will match any and all errors occuring on line 3.
- Provide more useful output when assertions fail. Specifically, output //all// lint messages that are missing and/or surplus. Previously, only the first lint message was output.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6854
Differential Revision: https://secure.phabricator.com/D11176
Summary:
I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods.
I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19730
Summary:
I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods.
I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20514
Summary: The recently-added, build-plan-behavior-aware check here does all of its own prompting, so we should skip the other prompting if it doesn't throw.
Test Plan: Will `arc land` something sketchy sooner or later.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20494
Summary:
See PHI1238. This "," in the regex can only make the lint binding fail if there is no "," in the version output string. In modern versions of Pylint, there is (apparently) no comma in the version string. Remove it.
See also <https://discourse.phabricator-community.org/t/arcanistpylintlinter-version-regex-issue/2688>
Test Plan:
```
$ pip install pylint
Traceback (most recent call last):
File "/Users/epriestley/Library/Python/2.7/bin/pip", line 6, in <module>
from pip._internal import main
ImportError: No module named pip._internal
```
¯\_(ツ)_/¯
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Differential Revision: https://secure.phabricator.com/D20505
Summary:
Since I'm in here for PHI1083:
- Add some "--" so we get correct behavior when you have a file named "master", a branch named "README.txt", etc.
- Stop using "%C" unnecessarily.
- Fix some untranslatable strings.
Test Plan: Ran `arc patch` a couple of times, ran the variations of `git` commands to catch anything weird with "--" handling.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20254
Summary:
See PHI1083. Previously, see PHI648 and D19475.
When you apply a submodule patch in Git, it leaves you with a working copy that has the "submodule pointer" dirtied but the actual submodule untouched:
```
$ git status
On branch ...
Changes to be committed:
(use "git reset HEAD <file>..." to unstage)
modified: philter
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: philter (new commits)
```
So, if you're applying `D123` and `submodule/` was previously pointed at commit "A" but `D123` updates it to point at commit "B", you get this after `git apply ...`:
- Git index says "submodule/ = B".
- On disk, "submodule/ = A".
Now, if you `git add --all` or `git commit --all`, git picks up the "change" on disk as an intended modification of the submodule. This puts the submodule back to "A" and overwrites/undoes the "pointer" update that's trying to make it point to "B".
To avoid this, update submodules after applying the patch.
Also, every time we modify the working copy, just update submodules.
Test Plan:
- Add a submodule in branch "B1", pointed at commit "A".
- Branch to create branch "B2". Update the submodule to point at commit "B". Commit this and `arc diff` it.
- Go back to "B1". Use `arc patch D...` to apply the revision you just created.
- Before change:
- "arc patch" applies the submodule change, so "pointer = B", "disk = A".
- "arc patch" runs "git commit --all", which looks at disk and sets "pointer = A".
- This isn't a change, so we fail with an empty commit.
- After change:
- "arc patch" applies the submodule change, so "pointer = B", "disk = A".
- "arc patch" updates submodules, so "pointer = B", "disk = B".
- "arc patch" runs "git commit --all", which now has a change, and commits "submodule = B".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20253
Summary:
Ref T13258. This makes "arc land" respect the new "Warn When Landing" behavior.
This will only work if you have very up-to-date APIs. Just fall back to the older code if the new API calls fail.
Test Plan: Ran `arc land` on a revision with builds in various states and with the different "Warn When Landing" behaviors. Saw appropriate warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20236
Summary:
See PHI1104. The older "differential.querydiffs" method includes the entire raw diff text for all the diffs associated with a revision in its response, but we: only care about the most recent diff; and don't care about the text at all.
For reasonably large changes with several updates, this can be significantly slow.
We can get this same information more efficiently from the modern "differential.diff.search", since D19386 (April 2018). The only trick is that we need a "revisionPHID", which we don't have on hand.
For now, just fetch the revision PHID. In the future, we can likely make adjustments so that we have the revision PHID already by the time we get here.
This may slow down the normal case very slightly (since we now do two calls instead of one), but it speeds up the bad cases dramatically.
Test Plan:
Ran `arc diff` to update a change in a local repository. `var_dump()`'d the old and new algorithm results, saw the same outcome.
Used `arc diff --trace` on an update to a change to verify that `differential.diff.search` is called but `differential.querydiffs` is not.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20221
Summary:
Ref T13161. See D20181. This allows the intraline highlighter to accept new ">" and "<" spans and apply a different style for them.
The input pattern is `list<segment>`. Each segment is `pair<wild kind, int byte_length>`, i.e. wrap the next `byte_length` bytes in a span of kind `kind`.
Before this change, the possible kinds of segements are `0` (no intraline diff, do not highlight) or `1` (intraline diff, highlight in bright color).
D20181 adds `<` (depth decreased) and `>` (depth increased). These are like `1`, but add a different class so the UI can handle them differently.
Test Plan: See D20181.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20182
Summary:
Ref T13249. See PHI810. We currently warn you when //all// reviewers are away, but not when only some reviewers are away.
This makes some amount of sense under the "anyone can accept anything" rules we sort of recommend, but a lot of installs realistically have tons of owner/package rules now.
Instead, if any reviewers are away, show the user exactly who is away and until when, then make sure they don't want to make any adjustments.
(We can do a better job of this after the toolsets change when we can use the new APIs, but this is an easy fix for now.)
Test Plan: Created a revision with multiple reviewers, either some or all of whom were away. Got appropriate output and prompt behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20172
Summary: Ref T13249. We currently allow `if (function_exists('X')) { X(); }` but not `if (defined('X')) { X; }`. Allow the latter.
Test Plan: See D20145, which linted clean with this patch in place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20146
Summary: See 30 prior patches. This is a fatal in PHP7, let's just hunt these down.
Test Plan: Ran unit tests. See next diff for results.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19931
Summary:
See PHI904. Ref T13210. Ref T13209. Currently, we have an `hg cat` construction which attempts to pass a literal `%p` to Mercurial. This fails because you can't pass `%` through `%s` outside of `wilds`.
It also uses `%C` to pass a list of file paths. This is broadly unsafe and can cause command execution if you modify a file named, e.g., `; rm -rf xyz` or similar. I think it would be difficult to turn this into an attack but it's fairly bad. This dates from D5144 in 2013.
Test Plan: With this patch, created D19757 which has valid binary data (see F5962134).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13210, T13209
Differential Revision: https://secure.phabricator.com/D19758
Summary:
Ref T13098. See PHI858. If you write this at the end of a message in `arc diff`:
```
Subscribers:
#projectname
# NEW DIFFERENTIAL REVISION
# Describe the changes in this new revision.
# ...
```
...we'll currently eat the `#projectname` as an instructional comment, even if it is followed by an empty line.
Instead, stop eating stuff once we hit the first empty line. (We escape empty lines in comments already.)
After T13098 I'll maybe adjust this to use a more explicit instruction escape, like `##`, since there's no reason we're bound to `#`.
Test Plan: Added a unit test and made it pass.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19639
Summary: I'm not sure if the upstream will be interested in this change, but we are writing a linter which works by running an external command on the entire repository. This can't be done with `ArcanistExternalLinter` at the moment, which meant that we ended up copy-pasting most of `ArcanistFutureLinter`. This would be a lot easier if we could override `willLintPaths` and `didLintPaths`, but these methods are currently marked as `final`. An alternative solution would be some sort of `ArcanistLinter::transformPath` method.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: faulconbridge, Korvin
Differential Revision: https://secure.phabricator.com/D19630
Summary:
Ref T13187. See PHI838. If two hunks are separated by 7 lines of context, we can render them as either:
```lang=diff
+ Hunk A
Context 1
Context 2
Context 3
Context 4
Context 5
Context 6
Context 7
+ Hunk B
```
...or:
```lang=diff
+ Hunk A
Context 1
Context 2
Context 3
@@ +1,2 -3,4 @@
Context 5
Context 6
Context 7
+ Hunk B
```
Since we get the same number of output lines either way and the first one is more human-readable, we picked that one.
However, `diff -u` does the second one. Since human-readability is probably less important than compatibility, change the behavior to be more similar to `diff -u`.
Test Plan: Added unit tests for the edge cases with default parameters (6 context lines, 7 context lines) and made them pass.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19603
Summary:
See https://discourse.phabricator-community.org/t/arc-not-supporting-git-2-17-1/.
When treating it a large file binary, we try to get the "old" and "new" content using `git ls-tree` and `cat-file`.
If the file is new or deleted, there is no old file, so we try to work with filename `null`.
Under git < 2.17.1, that gets treated as `git ls-tree -- .`, which falls in the next condition under "no such path".
In git 2.18, etc, this is an error.
Explicitly bail out if there is no filename.
Test Plan: Add a new, large (>4Mb) file, arc diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19513
Summary: See PHI718. Modern Mercurial with the "evolve" extension enabled may emit this field.
Test Plan: As D19262.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19498
Summary:
Ref T13151. See PHI648. With `arc patch --nobranch`, we update submodules a little too early.
I //believe// it is safe to just update them a little later, after the intermediate branch management logic runs.
Test Plan: Ran `arc patch --nobranch`, saw submodule update run later. Not 100% sure this doesn't cause weird issues, but I can't anticipate any.
Reviewers: amckinley, jmeador
Reviewed By: jmeador
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19475
Summary:
The Configuration Manager is supported by ArcanistUnitTestEngine but not support by the ArcanistConfigurationDrivenUnitTestEngine.
Added the configuration manager as one of the initially set properties of an ArcUnitTestEngine created by the ArcanistConfigurationDrivenTestEngine
Test Plan: Ran arc unit against a project without the change, verified the Configuration was none. Added this change and ran again and verified it was set
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19465
Summary:
Ref T13137. See PHI592. When you have a diff with 600MB of videos, we want to bail out of diff generation early (as soon as we realize what we're dealing with), not build an 800MB text diff in memory and then throw it away.
Support bailout //during// diff generation once we realize we're in over our heads.
This is approximate, but since the limit is fairly large (512KB by default) it isn't too important to be precise.
Test Plan: Rigged some callers to set various byte limits, generated diffs including diffs with large binaries. Got an appropriate diff or exeception depending on how low the limit was.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19444
Summary: Fixes T1246. See PHI637. See T13137. Computers have gotten a bit faster so we can probably bump this up a little and see if it causes problems. This is `O(N^2)` so the this should be less than twice as expensive in the worst case.
Test Plan:
Created a diff affecting characters on a very long line separated by more than 80 but fewer than 100 characters, got a good intraline diff out of it:
{F5605162}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T1246
Differential Revision: https://secure.phabricator.com/D19442
Summary:
Ref T13130. I wasn't able make this much better, but it looks like this is about ~20% faster on my system.
This kind of thing is somewhat difficult to micro-optimize because XHProf tends to over-estimate the cost of function calls. In XHProf, this looks much much faster than the old version (~100% faster) but the actual cost of `bin/conduit call --method differential.getrawdiff` hasn't improved that much. Still, it seems consistently faster across multiple runs.
Test Plan:
- Pulled binary diffs over Conduit with `bin/conduit call --method differential.getrawdiff`.
- Verified that they are byte-for-byte identical with the pre-change diffs, and look like they're ~20% faster.
- Profiled the differences and saw a far more dramatic improvement, but I believe XHProf is exaggerating the effect of this change because it tends to overestimate function call cost.
- Ran unit tests (from D19407), got byte-for-byte identical output under both 32bit and 64bit mode.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19408
Summary:
Ref T13130. I want to take a crack at improving performance here, but two possible approaches (inlining the actual encoding; using integers if they're big enough) aren't easy to test right now.
Restructure the tests so they can support these kinds of refactoring.
The "32bit" and "64bit" modes currently do the same thing, but I expect to introduce introduce separate encoding pathways in a future change, if the profiler says it actually helps.
(I'll hold this and everything that comes after it until I make meaningful performance improvements.)
Test Plan: Ran `arc unit`, got passes on tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19407
Summary:
Filenames are last in `git status --porcelain=2` lines; they
are not escaped in any way, despite the fields being
whitespace-delimited. `explode` thus happily chops apart filenames
with spaces in them, causing later git operations to operate only on
the filename up to the first space.
Split the lines into the right number of elements -- in all cases,
this is one more than the index we're using, since filenames come last.
Test Plan:
Altering a file with a space in its path, and running `arc diff -a`.
Added tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D19389
Summary: Ref T13110. We now degrade very large changes and I'm not convinced any user ever entered "n" at this prompt.
Test Plan: Ran `arc diff` to create this very revision.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19299
