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:
Ref T13490. Historically, "arc" was eager to load external libraries from many locations. I plan to make this significantly more structured/safer in the future (see T5055), but this rule got dropped as the "experimental" and "wilds" branches collapsed to "master".
Restore "next to arcanist" and "inside externals/includes" as valid search locations for classic workflows.
Test Plan: Ran "arc" commands in a working copy with a "load" library configured.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21004
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
