Summary:
Ref T13098. I made this change with a machete and a hacksaw. Major ideas:
The `--json`, `--ugly`, and `--output` flags are a new `--format <json|default>` flag instead. Formatters (JSON, Console) are modularized instead of being hard-coded. In a future diff, I will probably modularize this more into a general "sink/output" object and implement `--target X` ("send results to harbormaster build target X") as an output/sink, too, and then when you run `--format json --target X` we just send all the results to two outputs, and one sends them to the console while the other one uploads them.
The `--everything`, `arc unit path path path ...`, and `--rev` flags don't work yet, and `arc unit` always behaves like `arc unit --everything`. This is fine for `arcanist/` since the whole test suite currently runs in 5 seconds. I expect to restore these more or less as they previously existed later, once the working copy / repository stuff is in better shape.
The `--engine` flag is gone. This is an old flag and obsolete with `.arcunit`. `arc unit` now requires `.arcunit` to exist.
Some other flags are gone but I expect to restore them, at least in some form, later on.
Much of this barely works, but it //appears// that all the tests run, so we can start putting coverage on `arc alias`, Windows shell escaping, etc.
Test Plan: Ran `arc unit`, got something that looks like unit test results. When some of them didn't work, got failures.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19710
Summary: Ref T13202. See PHI889. See D19702. A small part of that change affected libphutil, but got lost in the shuffle with all the Arcanist/wilds stuff in my local workspace.
Test Plan: See D19702.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19720
Summary:
Ref T13098. I've eventually made `arc unit` at least sort of run, but a few unit tests have an issue under the new code.
We have a couple of cases where we `print_r($stack, true)` or `print_r($exception, true)`, which is effectively the same because exceptions include a stack. In one case, we search in the stack for values to make sure `PhutilOpaqueEnvelope` is really masking secrets. In another case, we just use `print_r(...)` to label test cases, but some test data is exceptions.
Under the new code, the `arc unit` stack has access to a much larger "world" via variables reachable in stack frames, since it's connected to all toolsets/workflows and those objects are more deeply interconnected. This makes the output from `print_r($stack)` enormous and slow (20+ seconds) because of all the recursive referencing of complex variables.
In the case where we're looking at the stack, just print the last couple frames. Also add some positive code that searches for a known value in the stack.
In the case where we're describing variables, just drop the code. We don't really need to label these test cases, the "expect" value is sufficiently clear on its own.
Test Plan: Later, ran `arc unit` and saw it finish in a reasonable amount of time instead of hanging forever on these tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19709
Summary: Ref T13098. I eventually got unit tests sort-of-almost running and they caught these two issues. Since the tests don't get very far with these classtree problems, just fix them up now so when `arc unit` actually runs later on it can meaningfully run tests.
Test Plan: Ran `arc unit`, after other changes let it run, and saw fewer "class doesn't implement every abstract method" issues.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19708
Summary:
Ref T13098. I didn't fully capture all the nuances of the old algorithm, and the behavior we want is slightly more complicated. This adjust things to try to express the more complicated behavior in a relatively clear (?) way.
When a bunch of working copies are nested inside one another, first pick the deepest one, then ask it to pick the best option among all working copies of the same type.
For Git and Mercurial repositories, just pick the deepest one.
For Subversion repositories, pick the directory with `.arcconfig` if one exists, or the shallowest directory otherwise (this could be more sophisticated, some day).
The general idea here is that if `/a` is a Git repository and `/a/b/c` is a Git repository and you run inside `/a/b/c`, that should be the working copy.
But we have to use a different rule for Subversion because //every directory// had a `.svn` directory in it until SVN 1.7.
Test Plan:
Locally, with `core/lib/arcanist/`, got `arc` to anchor to `arcanist/` instead of `core/` with the new ruleset.
(This will eventually get more extensive Subversion testing.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19707
Summary:
Ref T13098. Ref T13198. Ref T12996. The major ideas here are:
Workflows must define a list of all the prompts they can raise, so that these prompts can be enumerated with `arc prompts <workflow>`.
Prompts themselves should respond properly to ^C (abort immediately) and we should be able to make them nonblocking in the future (particularly, we'd like to be able to continue reading bytes from subprocess buffers while the prompt is shown on screen).
This doesn't have a lot of fancy features yet (non-confirm prompts, default yes, prompts which don't abort on "N", etc) but those should be easy to add later.
In the future, you'll be able to configure a default answer to prompts either in a config file or at runtime with `--config prompts=x.y.z=N` or similar.
This removes the history/readline mode for prompts, where you could use the up arrow to cycle through older responses. I believe this was only really useful for "excuse" prompts and intend to remove those.
Test Plan:
Forced `arc shell-complete` to always prompt, then:
- Got prompted, answered "y", "n", "N", "", "quack". Got sensible behavior in all cases.
- Ran `echo | arc shell-complete`, got a TTY error.
- Ran `arc prompts`, `arc prompts shell-complete`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098, T13198, T12996
Differential Revision: https://secure.phabricator.com/D19706
Summary:
Ref T13098. Add ^C handling and some small bits:
- Update `arc weld`.
- Test that `arc weld filen<tab>` completes `filename` (it does).
- Add a "workflow stack" -- I plan to make it easier for `arc diff` to call `arc unit` / `arc lint` as formal sub-workflows, etc., and make "workflow X delegates to workflow Y" a more formal thing.
On interrupts:
- Workflows can do something when you press ^C.
- If they do, press ^C twice quickly to exit.
- Otherwise, we exit on the first ^C.
The major thing I'd like to do in the short-ish term is to make `phage` report status on interrupt, but some other workflows might make sense to have interrupt handlers (maybe long-running stuff like `arc upload` / `arc download`) and third parties may find creative uses for them.
Test Plan:
- Added some `sleep(...)` to WeldWorkflow.
- Interrupted, got program exit.
- Added interrupt handlers and interrupted, got interrupt handling.
- With interrupt handlers, interrupted twice. Got program exit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19703
Summary: Fixes T9116. Ref T13098. Fix shell completion handling of filenames with spaces in them. This is a big mess -- see T9116 for discussion and I'll walk through things below.
Test Plan:
These all now seem to do the right thing:
```
arc we<tab> -> arc weld (+space)
arc weld fo<tab> -> arc weld foo\ bar (+space)
arc weld 'fo<tab> -> arc weld 'foo bar' (+space)
arc weld src<tab> -> arc weld src/ (no space)
arc weld src/w<tab> -> arc weld src/work (no space)
arc weld src/work<tab><tab> -> suggests "workflow/", "workingcopy/"
arc shell-complete --gen<tab> -> arc shell-complete --generate
```
These also work, which I think is nice-to-have:
```
arc WEL<tab> -> arc weld
arc shell-complete --GEN<tab> -> arc shell-complete --generate
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098, T9116
Differential Revision: https://secure.phabricator.com/D19705
Summary:
Ref T13098. Major changes:
`arc shell-complete` now installs itself to your `~/.profile`. Running `arc shell-complete` again will update the hook and update the completion rules.
Completion rules work for all toolsets, so you can install once and then autocomplete in `arc`, `phage`, etc.
This code supports other shells in theory, and I developed most of it with ZSH support next to the Bash support. However, while actually testing ZSH support, I couldn't get it to even slightly work and found myself falling down a very, very deep rabbit hole of ZSH being entirely written in bash script and `${0🅰️h}` being a legitimate script construction. The existing ZSH support comes from one guy in 2012 and also does not work for me on `master`, so I ultimately removed it. Open to restoring it but I wasn't able to figure it out in 10 minutes of Googling and I'm not convinced it's worth 11 minutes of Googling.
I left a few rough edges here with notes on how to improve/fix them, but the basics all work.
Test Plan:
- Ran `arc shell-complete` under various stages of `~/.profile`, couldn't get it to do anything bad.
- Ran `arc lib<tab>`, `arc shell-complete --curr<tab>`, etc. Got sensible suggestions and completions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19700
Summary:
Ref T13098. The logging stuff is starting to get a little sketchy, so wrap it in several layers of class-based indirection before it can escape its cage.
This at least gives us an actual API and structure to work with later instead of scattering a lot of `fprintf(STDERR, ....)` all over the place.
Test Plan: Ran a few commands, got slightly more sensible/consistent logging out of them.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19699
Summary:
Ref T13098. This repairs Conduit integration. Conduit was meaningfully updated in the `experimental` branch so a lot of this is just deleting code I don't plan to support going forward.
This removes "conduit.timeout", "http.basicauth.user" and "http.basicauth.pass". I believe these were all crazy niche calls with essentially no legitimate use. We could provide extension support if anyone actually uses this stuff, now.
Fixes an old "phutil" reference in HTTPSFuture.
Builds Conduit engine support into `ArcanistWorkflow`. There's perhaps some argument for trying to not make this core, but every upstream thing we'll ever write probably wants it (`arc`, `phage`) and there's not much of a cost to making it core. Even non-core stuff may include extensions which expect Conduit support (for example, for reporting workflow metrics to Phabricator).
There's no authentication support yet, I'm planning to update "hosts" config handling next.
Test Plan:
```
$ echo '{}' | arc call-conduit conduit.ping
WARNING Ignoring unrecognized configuration option ("hosts") from source: User Config File (/Users/epriestley/.arcrc).
WARNING Ignoring unrecognized configuration option ("load") from source: Project Config File (/Users/epriestley/dev/core/.arcconfig).
{
"error": null,
"errorMessage": null,
"response": "secure001.phacility.net"
}
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19698
Summary:
Ref T13098. This leaves a lot of rough edges but nothing is overtly broken so here's where we're at so far:
Config sources get "scopes", like user configuration vs system configuration. The major reason for this is so that `arc set-config x y` can know where it's supposed to write. This is generalized enough that we can implement `arc set-config --system ...` and `arc alias --local ...` and so on relatively easily later, although scopes themselves are not modular (a third-party can't add a new type of config scope). Maybe we'll modularize this some day but it felt like that's probably YAGNI/overboard since we have no current use cases. For now, a source does not need to belong to any particular scope.
Config may be writable (like user config in `~/.arcrc`) or nonwritable (like `--config` flags). Writable config can now specify how to write to disk. Config files can actually write to disk now, although the only pathway for doing this that exists is via `arc alias`.
Aliases now parse properly and can write to disk. `arc alias` now lets you define aliases, and writes them to disk. **The first time you do this, your `~/.arcrc` file will be rewritten into a format which old `arc` can not read!** It's relatively easily to unmangle/repair these files so I'm planning to just let this happen.
When a toolset is invoked, it now reads and evaluates aliases. Aliases have a lot of new guard rails like suggesting the user try `arc draft` if they type `phage draft`, allowing alias chains, detecting cycles, and limiting chain length.
Workflows can provide help and argument lists in a more structured way. I've moved this to sub-objects: help is now on `WorkflowInformation` (instead of a bunch of different `getHelp()`, `getSynopsis()` methods) and arguments now have a `WorkflowArgument` object instead of a dictionary. I think this pattern is generally better for extending: it lets us add and change stuff with less impact (and greater explicitness) down the road.
`arc alias` now has reasonable help text and argument documentation. The `arc alias` (list) and `arc alias x` (details/remove) flows don't work yet but `arc alias x y` does.
`arc liberate` now uses the new help/argument stuff, although the help needs more beef eventually. I pruned a bunch of long-obsolete or questionable flags and renamed `--all` to `--clean` since `--all` sounds like "liberate all libraries", which is now the default behavior of `arc liberate`.
Test Plan:
You can now define chains of aliases. Finally!
```
$ arc draft4
WARNING Ignoring unrecognized configuration option ("hosts") from source: User Config File (/Users/epriestley/.arcrc).
WARNING Ignoring unrecognized configuration option ("load") from source: Project Config File (/Users/epriestley/dev/core/.arcconfig).
ALIAS arc draft4 -> arc draft3
ALIAS arc draft3 -> arc draft2
ALIAS arc draft2 -> arc diff
Usage Exception: Unrecognized argument '--draft'.
```
This also works now:
```
$ phage alias deploy-secure -- deploy --hosts secure001-4 --limit 1
```
General!
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19697
Summary:
See T13098. All ".arcconfig" files except "~/.arcrc" have config keys at top level.
"~/.arcrc" previously had "aliases" at top level, and currently has "hosts" at top level. "aliases" became standard configuration. I'd like to make "hosts" standard configuration too -- one reason to do this is to make automation with `--config-file` easier, so you can shove API tokens in a file somewhere (and not need a home directory). Another reason is just to standardize things.
If we read "~/.arcrc" and see a "config" key, put all those keys at top level and then fill in anything else left over so we end up with `~/.arcrc` that effectively looks like other "arcconfig" files. I'd possibly like to rename this file to "arcconfig" at some point, too, but that can happen later.
Test Plan: Ran `arc get-config`, which barely works, but now read my `~/.arcrc` somewhat more successfully.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19696
Summary:
Ref T13098. This is pretty rough, but sketches out the general shape of a more modern configuration flow. The new flow is very similar to the Phabricator flow.
Each configuration option is typed (string, bool, list of dictionaries, etc) so we can typecheck it, and each type is a class so the types are modular and can do fancy stuff. Some of this "fancy stuff" that I want to do includes:
- transparently rewriting/reformatting various options for modernness/consistency;
- having some options exposed as objects instead of raw JSON values (in particular, aliases);
- merging "list" options (like "aliases") in a modular way instead of by having hard-coded stuff that says "this particular option is magic gets merged instead of getting replaced when defined in multiple places".
Generally, this makes everything modular and extensible and gets rid of the hard-coded `switch (...)` stuff.
Test Plan: Ran `arc get-config`, it sort of almost worked.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19695
Ref T13098. I think I didn't turn this into an actual revision, but let
`ClassMapQuery` optionally continue if it encounters a class load failure.
If we don't allow this, it can become very difficult to modify or remove
some Arcanist classes since when you, say, remove a Workflow you can no
longer make it to "arc liberate" to update the map for the change.
This is currently used in roughly one place (in an upcoming diff) to let us
get through startup in Runtime and into the "liberate" workflow.
Summary:
Ref T13098. This is kind of a catch-all diff with stuff that didn't fit in prior diffs, and which fixes some bugs with that stuff now that I made it at least sort of reachable.
Beyond bugs, the general idea is to replace `ConfigurationManager` (a big class which knew about config-end-to-end) with a more modern/modular `ConfigurationEngine` using the standard Engine + EngineExtension modularity pattern.
Configuration becomes a `ConfigurationSourceList` of `ConfigurationSource` objects, each of which represents one source (a config file, `--config x=y`, etc). The various sources will have the logic to parse values (e.g., decode `x=y` flags or JSON files on disk). A new `--config-file` allows you to replace the system (`/etc/arcconfig`) and user (`~/.arcrc`) files.
This also gets rid of `--library` support entirely for now since it's kind of messy to bridge until Config works. I expect to either restore it or replace it with `arc install` and similar.
Test Plan: Ran `arc liberate`; it actually works now. (The Config stuff does not actually work yet.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19694
Summary:
Ref T13098. This is mostly for getting sensible results under Windows, hopefully instead of weird mixed paths with both `/` and `\`, at least in more cases.
You can pass in several components like `array('/path/to/something/', '/thing.c')` and they'll be concatenated with exactly one correct separator.
Test Plan: Used this in the new `WorkingCopy` stuff, which doesn't run yet. 💁
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19691
Summary:
Ref T13098. After libphutil/ and arcanist/ merged, some paths need to be adjusted. Try to organize things a little better, too.
Also, make `arc liberate` with no arguments just liberate all the libraries it can find.
Test Plan: Ran `arc liberate` and got a valid map rebuild, although I needed to apply some hacks on top of this to make the workflow reachable.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19690
Summary:
Ref T13098. Currently, `WorkingCopyIdentity` has rules for finding `.git/.svn/.hg` directories and `.arcconfig` files. It also has a ton of logic for reading and writing config files.
Generally:
- Rename `WorkingCopyIdentity` to `WorkingCopy`.
- Make it an abstract base with `Git`, `Mercurial` and `Subversion` subclasses, using the standard module/extension pattern.
- Throw out almost all of the config file logic. This logic is going to move into Config classes. It is not unique to working copies.
- Working copies retain the actual bit of this they need: knowing where stuff should go on disk.
This doesn't run yet, but sure looks a lot cleaner!
Test Plan: Doesn't run yet, since Config isn't working yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19689
Summary:
Ref T13098. We currently handle fatals by printing a message and returning an error code, since this was the most direct adaptation from the old `arcanist.php` script.
Instead, throw an exception and then handle it above, since we can reasonably do that in one place now.
Test Plan: Ran `arc`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19693
Summary:
Ref T13098. This logic comes from D3243 and was theoretically used to let you install `libphutil` with Homebrew. Since libphutil no longer exists and the other use cases I can come up with are questionable/obsolete, remove it.
(As we move into T5055, I expect to provide better tools for bundling/managing external dependencies.)
Test Plan: Ran `arc`, same as the old `arc`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19692
Summary:
Ref T13098. Historically, Phabricator was split into three parts:
- Phabricator, the server.
- Arcanist, the client.
- libphutil, libraries shared between the client and server.
One imagined use case for this was that `libphutil` might become a general-purpose library that other projects would use.
However, this didn't really happen, and it seems unlikely to at this point: Phabricator has become a relatively more sophisticated application platform; we didn't end up seeing or encouraging much custom development; what custom development there is basically embraces all of Phabricator since there are huge advantages to doing so; and a general "open source is awful" sort of factor here in the sense that open source users often don't have goals well aligned to our goals.
Turning "arc" into a client platform and building package management solidify us in this direction of being a standalone platform, not a standalone utility library.
Phabricator also depends on `arcanist/`. If it didn't, there would be a small advantage to saying "shared code + client for client, shared code + server for server", but there's no such distinction and it seems unlikely that one will ever exist. Even if it did, I think this has little value.
Nowadays, I think this separation has no advantages for us and one significant cost: it makes installing `arcanist` more difficult for end-users.
This will need some more finesssing (Phabricator will need some changes for compatibility, and a lot of stuff that still says "libphutil" or "phutil" may eventually want to say "arcanist"), and some stuff (like xhpast) is probably straight-up broken right now and needs some tweaking, but I don't anticipate any major issues here. There was never anything particularly magical about libphutil as a separate standalone library.
Test Plan: Ran `arc`, it gets about as far as it did before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19688
Summary:
Ref T13098. Currently, it lives in `init-script.php`. Move it to a separate file for similarity with `support/PhabricatorStartup.php`.
Two small changes here:
- `dirname(dirname(...))` adjustments for new path.
- Remove `memory_limit(-1)`, this is adjusted by the libphutil init script already.
Test Plan: Ran `arc`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19687
Summary: Ref T13098. The name of this option is needlessly obscure/verbose, and we now require it come first so it should be unambiguous even if we add `arc borrow-a-book` later.
Test Plan: Grepped for `--load-phutil-library`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19686
Summary:
Ref T13098. This makes "arc help" kind-of sort-of work again.
Previously, "arc help" was completely custom and hard-coded. Now it just proxies the generic libphutil "help" workflow. The two flows are substantially similar, although old `arc help` was a little more brief than the new version is. The old `arc help --full` was more similar to the default:
| Command | Old Behavior | New Behavior |
|---|---|---|
| `arc` | Tells you to run `arc help`. | Plan: make this a bit richer and give a quick summary of workflows. |
| `arc help` | Gives you a summary of workflows. | Gives you a detailed list of workflows. |
| `arc help --full` | Gives you a detailed list of workflows. | No such flag. |
| `arc --help` | Same as `arc help`. | Same as `arc`. Plan: same as `arc help`. |
| `arc help <workflow>` | Full workflow help. | Full workflow help. |
| `arc <workflow> --help` | As above. | As above. |
Overall this is largely the same and lets us delete a bunch of code.
Test Plan:
Ran `arc`, `arc help`, `arc --help`, `arc --help diff`, `arc diff --help`, `arc help diff`, `arc help help`.
These commands largely do something sensible now, with caveats per above.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19679
Summary:
Depends on D19677. Ref T13098. This gets enough of the new workflow/toolset stuff running to run `arc liberate` (without arguments) so that the new `arc` can bootstrap itself to continue development.
This change also moves some workflows which will be shared across toolsets (shell completion, versions, help, aliases) out of being `arc`-specific, to varying degrees of success. These workflows will need to be revisited and cleaned up so they work properly. They mostly fatal when run right now.
The Conduit flags have moved to the new `arc` parent workflow, but they'll likely move to some kind of support object so `my-company-thing megadiff` can accept Conduit flags. This is a more involved change, however.
Test Plan: Ran `arc liberate`, got the Wilds `arc` to rebuild its own map. Everything else crashes!
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19678
Summary:
Ref T13098. Depends on D19675. This change is headed to the `untamed-wilds` branch.
This change prevents `arc` from running `arc diff`, so I'm copy/pasting it.
This change also completely breaks `arc`, but I'm just generally trying to do this rewrite step-by-step so we have at least a bit of context to refer to in the future.
The major change here is to turn both `arc` and `phage` into scripts which start an `ArcanistRuntime`. This runtime then decides which workflows (like "diff", "patch", or "remote") are available based on `$argv[0]`. This turns `arc` into more of a CLI tools platform: we can build `phage` on it, third parties can build `my-companion-tool-thing`, etc. But all the different entry points can share a lot of infrastructure like: `help`, `alias`, shell-complete, configuration, Conduit, prompting, `--trace`, extension infrastructure, and so on.
`ArcanistRuntime` is roughly a slightly more modern version of `scripts/arcanist.php`. That will be removed eventually, but not everything has ported yet.
Test Plan: This code basically doesn't run yet, although the next patch runs at least a little bit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13098
Differential Revision: https://secure.phabricator.com/D19677
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
Summary: See PHI527. Ref T13116. The `--revision` flag currently fails if the argument is in the form `D123` instead of `123`. Normalize monogram arguments.
Test Plan: Ran `arc patch --revision Dxxx`, `arc patch --revision xxx`, `arc patch --revision xxx --diff yyy`, `arc patch`; got good behavior on the good ones and sensible error messages on the other ones.
Maniphest Tasks: T13116
Differential Revision: https://secure.phabricator.com/D19292
Summary:
STDERR output with `%`s in it could cause:
```
ERROR 2: fprintf(): Too few arguments at [/usr/local/arcanist/src/workflow/ArcanistFeatureWorkflow.php:170]
```
Test Plan: Untested.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D19261
Summary:
See PHI458. The help text and prompting for "arc diff --draft" aren't very clear about whether updating publishes or not, nor about whether you need to keep passing "--draft" every time.
Make these behaviors more clear.
Test Plan:
- Ran `arc help diff`, read text.
- Updated a draft with "--draft", got new warning but it went through.
- Updated a published revision with "--draft", got new error.
Differential Revision: https://secure.phabricator.com/D19229
Summary:
In Go 1.10 the output for tests was changed to have also a "(cached)" mode in
addition to the normal timing info printed. This is on by default. This adds
support for parsing these lines instead of erroring out on the regex.
Test Plan: Have a unit test included, and will continue to poke at it locally.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19161
