1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-12-05 05:02:43 +01:00
Commit graph

2081 commits

Author SHA1 Message Date
epriestley
5ef6599239 [Wilds] When reading ".arcrc" files, modernize the data we read in-process if they're in an older format on disk
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
2018-09-21 17:00:20 -07:00
epriestley
c64f86c2f6 [Wilds] Flesh out most of the new Config objects
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
2018-09-21 16:57:57 -07:00
epriestley
23aaf85eaf [Wilds] Allow class loading to continue on failure
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.
2018-09-21 16:56:36 -07:00
epriestley
412484022b [Wilds] Prepare for more modular configuration management
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
2018-09-21 16:54:00 -07:00
epriestley
11599cedb6 [Wilds] Implement a Filesystem::concatenatePaths(...) method
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
2018-09-21 16:52:38 -07:00
epriestley
c05bbd7be6 [Wilds] Allow "arc liberate" to liberate itself again
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
2018-09-21 16:50:58 -07:00
epriestley
d936257018 [Wilds] Rewrite WorkingCopyIdentity in a more modern/modular way
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
2018-09-21 16:48:00 -07:00
epriestley
9a94aa216b [Wilds] Slightly simplify fatal handling during "arc" setup
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
2018-09-21 16:46:05 -07:00
epriestley
fe0c293895 [Wilds] Remove include_path mangling and drop support for "externals/includes"
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
2018-09-21 16:44:11 -07:00
epriestley
8e0e07664a [Wilds] Remove libphutil
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
2018-09-21 16:38:53 -07:00
epriestley
f0608eef9b [Wilds] Remove the defunct "scripts/arcanist.php" script
This is now ArcanistRuntime, etc. I neglected to turn this into a revision but
it isn't a very interesting change.
2018-09-21 16:03:06 -07:00
epriestley
d8f660ec6f [Wilds] Move ArcanistRuntime to support/ArcanistRuntime.php
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
2018-09-21 16:00:30 -07:00
epriestley
d62830195c [Wilds] Rename "--load-phutil-library" to "--library" in new arc
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
2018-09-21 15:59:25 -07:00
epriestley
baadc1f842 [Wilds] Sort of make "arc help" work again
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
2018-09-21 15:58:35 -07:00
epriestley
8e51f89c79 [Wilds] Make "arc liberate" run in the untamed wilds
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
2018-09-21 15:57:36 -07:00
epriestley
7d05dbec15 [Wilds] Rewrite "arc" entrypoints for toolsets
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
2018-09-21 15:55:08 -07:00
epriestley
9d59e9e590 Merge branch "master" into "experimental". 2018-08-24 17:44:52 -07:00
epriestley
e1e93271e6 Make the ArcanistBundle algorithm do what "diff -u" does when hunks are arguably mergeable
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
2018-08-24 11:00:16 -07:00
epriestley
d9a4293ae7 Consolidate redundant "should should" from some linter help strings in Arcanist
Summary: See <https://phabricator.wikimedia.org/T201138>.

Test Plan: Read carefully.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19561
2018-08-03 14:36:41 -07:00
Aviv Eyal
875d018360 Fix arc diff when adding large new file with new git
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
2018-07-09 17:59:15 +00:00
epriestley
222800a86e Parse Mercurial changeset evolution "instability" log field
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
2018-06-19 16:15:30 -07:00
epriestley
c1b1e69b8f Merge branch "master" into "experimental". 2018-06-07 12:03:15 -07:00
epriestley
df7313bdf2 In "arc patch", update submodules slightly later
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
2018-06-07 12:03:08 -07:00
Tim McJilton
b199ca8086 [ARCUNIT] Set the ConfigurationManager of ConfigurationDrivenUnitTestEngines
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
2018-06-05 20:58:47 +00:00
epriestley
1a451a2e9a Merge branch "master" into "experimental" 2018-06-01 14:58:53 -07:00
epriestley
d581c453b8 Allow diff generation via ArcanistBundle to be limited to an approximate maximum byte size
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
2018-05-14 09:09:06 -07:00
epriestley
a1aec701e3 Raise the intraline diff hard limit from 80 to 100 characters
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
2018-05-09 13:38:20 -07:00
epriestley
290821d3f2 (experimental) Merge branch "master" into "experimental". 2018-04-30 11:40:15 -07:00
epriestley
a604548101 Slightly improve base85 performance for 64-bit systems
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
2018-04-27 12:04:04 -07:00
epriestley
bcab677a7a Restructure base85 unit tests to support inlining and multiple encoding pathways
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
2018-04-27 12:03:14 -07:00
Alex Vandiver
ad3087e5e1 Correctly parse git status --porcelain=2 output with filenames with spaces
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
2018-04-19 19:17:16 -07:00
epriestley
73f5afd441 Remove "very large change" warning from Arcanist
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
2018-04-05 06:50:57 -07:00
epriestley
e44a2d3ac0 Improve argument parsing for "arc patch --revision Dxxx"
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
2018-04-03 10:56:46 -07:00
epriestley
925c60e7b8 Merge "master" into "experimental".
This picks up a Mercurial parser fix and a handful of other things.
2018-03-26 14:12:41 -07:00
epriestley
b8c9c385a7 Survive extra "obsolete:" log output from the Mercurial evolve extension
Summary: See PHI502; see <https://bugzilla.mozilla.org/show_bug.cgi?id=1448137>.

Test Plan: I spent all of three minutes trying to install the `evolve` extension without success and gave up, but this probably does the right thing based on the example output in the Bugzilla issue.

Differential Revision: https://secure.phabricator.com/D19262
2018-03-26 14:12:15 -07:00
Alex Vandiver
bf3d32e34e Remove accidental sprintf injection in error reporting
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
2018-03-26 13:56:50 -07:00
epriestley
49a3ae9dad Improve help and prompts for the "--draft" flag
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
2018-03-14 20:24:42 -07:00
Nathan LeClaire
dcd7ef66d0 Add support for Go 1.10 (cached) output
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
2018-03-02 16:34:25 -08:00
epriestley
be1dd7e2ba Robustly fuse files together with arc weld
Summary: Fixes T8236. I played around with a lot of variations of this but in the end it felt like the simple version was best.

Test Plan: Ran `arc weld a.txt b.txt`, observed very robust fusion of materials.

Maniphest Tasks: T8236

Differential Revision: https://secure.phabricator.com/D19081
2018-02-13 15:15:36 -08:00
epriestley
349109426c Clarify what "--everything" means in "arc lint" and "arc unit"
Summary:
Fixes T13061. Both `arc lint` and `arc unit` accept an `--everything` flag, but the documentation isn't quite clear about what these flags do.

They act as though every //tracked// file in the repository (`git ls-files`, `hg manifest`, or `svn list -R`) is included in the argument list.

They do not lint/test ignored files (and I think almost all users would be very surprised if they did).

They also don't lint/test untracked files (files you have not yet used `git add`, `svn add`, or `hg add` on). This is slightly more contentious but we have good reasons for doing it (e.g., `git ls-files` often outperforms `find .` by a large margin) and I believe users very rarely use `--everything` in a situation where they have untracked files. The only real exception I can come up with is linter configuration/development, as in PHI343, and it seems okay to have a slightly surprising behvaior here.

Make the documentation more clear about what is in scope.

We could also rename these to `--nearly-everything` or whatever, but I think the name is probably clear enough given current information about how confusing this is (specifically: only rarely, in unusual cases).

Test Plan:
  - Grepped for documentation about these flags.
  - Ran `arc help lint`, `arc help unit`, `arc unit --everything x`, `arc lint --everything x` and read all the new messages.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13061

Differential Revision: https://secure.phabricator.com/D18989
2018-02-05 12:26:41 -08:00
epriestley
e64cbd3ba7 Merge branch "master" into "experimental". 2018-01-16 13:58:09 -08:00
epriestley
2e02332216 Add trailing tabs when generating synthetic Git diffs for files with spaces
Summary:
Fixes T8768. See PHI294. See that task for more details.

Git, Mercurial, `diff`, and `patch` have conspired to make things weird. To correctly handle files with spaces in the way everything else does and expects, we need to emit semantic trailing whitespace literals.

Test Plan:
- Created a file with spaces in it in a Mercurial repositroy, committed it, diffed it into a revision.
- Used `arc patch` to apply the change to a clean copy of the repository.
- Before patch: Mercurial incorrectly creates a file named `X`, not a file named `X Y.txt`.
- After patch: `arc patch` commit is identical to genuine commit.
- Also added test coverage. The other general behaviors here are fairly well covered already.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8768

Differential Revision: https://secure.phabricator.com/D18869
2018-01-16 13:57:27 -08:00
epriestley
3d06bd4c56 Fix a minor 'buildableStatus' error in "arc land"
See D18837. I made a mistake here with rigging this for testing; this line was
changed incorrectly.
2017-12-26 08:33:35 -08:00
Alex Vandiver
9144658e69 Accelerate working tree operations in git
Summary:
Currently, `arc` on `git` uses the following commands to examine the
state of the working tree and history; example times for a no-op diff in a
165k-file working tree are also shown:

```
1)  git diff --no-ext-diff --no-textconv --submodule=short --raw 'fb062d4ecce5d9c1786b7bfc8a0dedf6b11fdd96' --
  = 1,722,514 us

2a) git diff --no-ext-diff --no-textconv --submodule=short --raw 'HEAD' --
  = 1,715,507 us

2b) git ls-files --others --exclude-standard
  = 2,359,202 us

3)  git diff-files --name-only
  = 1,333,274 us
```

Steps (2a) and (2b) are run concurrently; this results in a total elapsed
wallclock time of approximately 5.4 seconds.  This is inefficient -- all four of
the above steps must both load the index and examine the working copy, which may
be slow operations when large repositories are used.  Additionally, none of the
effort of those stat calls on the working tree, or load time of the index, is
shared across the processes.

Step (1) is called from `getCommitRangeStatus`, which was split out in D4095; it
is currently never called on its own, only ever from `getWorkingCopyStatus`,
where it it combined with `getUncommittedStatus`.  The current behavior of the
method is to return the set of changes //either// in local commits //or//
uncommitted in the working tree, which duplicates work that
`getUncommittedStatus` is intended to do.  Changing the behavior of this method
(in Git, and other VCSes) to only examine _committed_ status seems both inline
with the name of the method and the original description of it in D4095 -- and
also serves to make it much faster, as it is an operation that need not inspect
the working tree at all.

Steps (2a), (2b), and (3) attempt to gather the state of the working copy, and
as such are all I/O bound but must examine nearly identical data.  For git
2.11.0 and higher, we can instead rely on the machine-parseable `git status
--porcelain=2` format, which provides the information from all of these commands
at once.  It also allows additional performance improvements, as `git status`
has been the focus of several optimizations in the latest versions of git (the
untracked cache and fsmonitor services, for instance), which are not available
in the lower-level `diff`, `ls-files`, and `diff-files` commands.

This has the added benefit of fixing a bug noticed in T9455, in that uncommitted
or unstaged changes in modules can now be detected, regardless of if they also
have changed their base commit.  It further resolves a bug where `.gitmodules`
appeared to have unstaged changes, when in reality the unstaged changes were in
submodules elsewhere in the tree.

For backwards compatibility with versions of git < 2.11.0, the old code is left
in place.  It is possible that the simpler output from v1 of `git status
--porcelain` would also suffice for some of the above benefits, but the payoff
of parsing yet another format is deemed insufficient; users wishing improved
performance should simply upgrade `git`.

Alltogether, these result in the following, for a no-op diff in a
165k-working-file tree:

```
1) git diff --no-ext-diff --no-textconv --submodule=short --raw 'fb062d4ecce5d9c1786b7bfc8a0dedf6b11fdd96' HEAD --
 = 9,227 us
2) git status --porcelain=2 -z
 = 739,964 us
```

...for a total of 749ms, an improvement of 4.7s.

Depends on D18841.

Test Plan: Existing tests.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18842
2017-12-23 20:12:50 -08:00
Alex Vandiver
f34467914c Add tests for git submodules, based on current behavior
Summary:
This adds tests that detail the current behavior of `arc` in
the presence of `git` submodules.

Test Plan: No behavior change; wrote the tests such that they pass.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18841
2017-12-23 20:01:20 -08:00
epriestley
c784b56920 Make "arc land" accommodate a minor API change in Harbormaster build statuses
Summary:
See PHI261. Currently "arc land" shows every build staus (passed, failed, building, etc) as yellow. Intended behavior is that passed builds are green, failed builds are red, and so on.

This is because of an unintended API change a while ago in D16356. Since the only impact was a cosmetic color issue, this escaped notice until now.

Additionally, try to use the modern `harbormaster.build.search` if it is available.

Test Plan:
  - Ran `arc land` with running builds, got reasonable coloration.
  - Faked the new method not being available, still got sensible behavior from the old method.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18837
2017-12-23 11:39:19 -08:00
Alex Vandiver
249f3a80fe Default the prompt for "Amend HEAD with these patches" to true
Summary:
Most users, if they have gone through the trouble of
accepting the auto-fixes, are most likely going to want to take those
changes and attempt to land with them.  Assuming "Y" for this prompt
streamlines for the more likely flow.

Test Plan: `arc lint`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18824
2017-12-12 01:43:16 -08:00
epriestley
f4c80a114d Make "arc land" prompt on "Changes Planned" revisions more explicit
Summary: Fixes T10233. See PHI231. Users sometimes believe this warning is a bug and/or don't understand how they're supposed to resolve it.

Test Plan: Ran `arc land` on a revision in "Changes Planned", got a sensible prompt. Ran `arc land` on a revision in another non-accepted state, got more or less the old prompt.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10233

Differential Revision: https://secure.phabricator.com/D18807
2017-11-30 13:51:50 -08:00
epriestley
057bdb1c71 Properly exclude others' revisions from automatic "Depends On"
Summary: See D18651. This was missing an `unset()`.

Test Plan: Looked carefully.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18766
2017-11-07 15:36:14 -08:00
epriestley
9054604214 Fix some lint rendering issues when lines prior to other identical lines are removed
Summary: See PHI191. This is a rehash of an earlier fix, but we didn't have a test case for this half yet.

Test Plan:
  - Added a failing test, made it pass.
  - Added a linter like the one in PHI191, ran it, got a valid lint result instead of an exception.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18759
2017-11-01 17:18:59 -07:00