1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-14 19:02:40 +01:00
Commit graph

2112 commits

Author SHA1 Message Date
epriestley
32005f26a4 Move Phage to FuturePool
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
2020-04-05 05:56:21 -07:00
epriestley
099c2ae648 Introduce "FuturePool" to make it easier to manage an ongoing pool of futures
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
2020-04-03 12:05:09 -07:00
epriestley
368aec16a1 Update some ancient "set X=Y" environment code for new Windows execution without a shell
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
2020-04-02 13:50:57 -07:00
epriestley
d4d095dbf6 Make Windows escaping preserve "%" symbols in arguments
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
2020-04-02 13:44:13 -07:00
epriestley
5ce1d79717 Fix error behavior of "arc version" when it encounters a library which is not a working copy
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
2020-04-02 08:24:22 -07:00
epriestley
63276697eb Fix three Windows subprocess issues
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
2020-04-01 16:11:05 -07:00
epriestley
492113370a Fix two issues with Future key selection inside FutureIterator
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
2020-04-01 10:36:44 -07:00
epriestley
b1a712add8 Integrate "ServiceProfiler" into the base "Future"
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
2020-03-30 07:42:40 -07:00
epriestley
cb80f69715 Make "FutureIterator" queue management more formal
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
2020-03-30 07:42:09 -07:00
epriestley
6b75562c3e Make "exception" on Future a private property
Summary: Depends on D21034. Ref T11968. Continue modernizing Future.

Test Plan: Ran tests, created a revision.

Maniphest Tasks: T11968

Differential Revision: https://secure.phabricator.com/D21035
2020-03-30 07:34:51 -07:00
epriestley
4d55067fd8 Make the "result" property on Future private
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
2020-03-29 10:18:26 -07:00
epriestley
e20dce875c Resolve all futures inside FutureIterator
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
2020-03-29 10:18:06 -07:00
epriestley
3df48c9257 Remove the "timeout" parameter from "Future->resolve()"
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
2020-03-29 10:13:59 -07:00
Paul Tarjan
33b9728b5f Run ls-files from the root of the directory
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
2020-03-25 14:23:53 -07:00
epriestley
4c12c3119b Treat all PHP language-level errors as exceptions by default
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
2020-03-22 12:41:05 -07:00
epriestley
18799c1829 Switch file uploader in "arc diff" to use ConduitEngine
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
2020-03-20 12:19:17 -07:00
Paul Tarjan
97e050fce7 Use a named remote and branches for staging to help git-lfs
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
2020-03-20 11:57:26 -07:00
epriestley
66a6128239 Remove the "preg_quote()" lint rule and update the "__CLASS__" lint rule
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
2020-03-06 09:14:10 -08:00
epriestley
31653f6b77 Fix an issue where "arc" may fail on startup when trying to read older "default" config
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
2020-03-06 08:31:25 -08:00
epriestley
5451d28752 When "ArcanistRuntime" exits with a nonzero exit code, emit that exit code
Summary:
See <https://discourse.phabricator-community.org/t/failed-arc-patch-does-not-return-non-zero-exit-code/3584>.

Returning an integer from a top-level PHP file doesn't actually affect the process exit code, as much as I might wish it does.

Test Plan: Ran `arc patch adflsnadfsln; echo $?`, saw a nonzero exit code after this fix.

Differential Revision: https://secure.phabricator.com/D21037
2020-02-27 06:17:02 -08:00
epriestley
1b97f8b408 Update "arc upload" for Toolsets
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
2020-02-26 08:22:18 -08:00
epriestley
9bd5c23b2a Improve error handling in ArcanistRuntime when failing to load libraries
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
2020-02-25 14:07:22 -08:00
epriestley
9cd72baae9 Update Phage for toolsets and restore library loading behaviors
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
2020-02-23 09:30:53 -08:00
epriestley
de461bb179 Fix two "implode()" order issues arising from wilds/experimental collapse
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
2020-02-23 08:34:30 -08:00
epriestley
ee66b15bd4 Port "arc upgrade" to Toolsets
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
2020-02-17 11:33:17 -08:00
epriestley
d4e4271b57 Remove obscure features no longer supported by Toolsets from "classic" Arcanist
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
2020-02-17 09:28:39 -08:00
epriestley
eb6edb2739 Restore old expanded include path rules for workflows which fall through
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
2020-02-17 09:24:53 -08:00
epriestley
d3b77af8a5 Require "--" as an argument terminator when running noninteractively
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
2020-02-16 09:16:51 -08:00
epriestley
db8419f19b Port "arc weld" and "arc anoid" to Toolsets workflows, plus minor fixes
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
2020-02-16 09:16:24 -08:00
epriestley
8cd79d38af Port "arc shell-complete" to Toolsets
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
2020-02-14 09:16:46 -08:00
epriestley
70c6045c7f Update "arc alias" to modern workflows
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
2020-02-14 09:16:18 -08:00
epriestley
0c6ae6bbcf Port "arc version" to Toolsets
Summary: Ref T13395. Depends on D20992. Run "arc version" as a modern workflow, not a classic workflow.

Test Plan: Ran "arc version".

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20993
2020-02-14 09:15:58 -08:00
epriestley
cf9469e0d1 Port "arc liberate" to Toolsets
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
2020-02-14 09:14:56 -08:00
epriestley
0e95fcbb7f Port "arc help" to Toolsets
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
2020-02-14 09:14:30 -08:00
epriestley
c471983697 Collapse Arcanist toolsets from "wilds" into "master", as an overlay layer
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
2020-02-13 14:10:46 -08:00
epriestley
acf0607683 Merge utility/support changes from "wilds" to "master"
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
2020-02-13 14:10:09 -08:00
epriestley
0d62a10eda Don't depend on "XMLWriter" to load the lint renderer class tree
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
2020-02-13 13:38:52 -08:00
epriestley
8c4f6ce161 Merge the remainder of the "experimental" branch
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
2020-02-13 06:05:08 -08:00
epriestley
4c36860c43 Merge Arcanist lint changes from "experimental" branch
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
2020-02-13 06:04:47 -08:00
epriestley
26f853b634 Merge "--draft" flag and related changes from "experimental" to "master"
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
2020-02-12 16:54:18 -08:00
epriestley
a8a50b2fc0 Make "arcanist/" unit tests pass
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
2020-02-12 16:20:30 -08:00
epriestley
61e059984a Merge "phage" from "experimental"
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
2020-02-12 15:53:23 -08:00
epriestley
9b74cb4ee6 Fully merge "libphutil/" into "arcanist/"
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
2020-02-12 15:17:38 -08:00
epriestley
a36e60f0a3 Move HTML-only intraline diff code to Phabricator
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
2020-02-12 13:13:23 -08:00
epriestley
21a1828ea0 Omit "--" in older fallback commands for Git remote URIs
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
2020-01-23 16:50:54 -08:00
epriestley
70c0fd3f22 In Git, fall back across versions more cleanly when trying to get the URI for a remote
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
2020-01-23 15:19:28 -08:00
epriestley
cc850163f3 When "arc close-revision --finalize ..." skips closing a revision, print a message
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
2019-11-18 20:31:51 -08:00
epriestley
cc1ff38843 When generating diffs in "arc diff", disable Git config option "diff.suppressBlankEmpty"
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
2019-10-29 10:14:40 -07:00
epriestley
73943d1bc9 Make "arc land --merge" an explicit error when targeting a Perforce remote
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
2019-10-28 11:59:42 -07:00
epriestley
7383c2f4e6 In "arc land", when "remote/onto" does not exist locally, try to fetch it before giving up
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
2019-10-28 11:31:18 -07:00