Summary:
Ref T12996. `arc lint` can render in different formats with `--output <format>`.
Previously, these were a big `switch()` statement and a bunch of hard-coded stuff.
Modularize them so that that third parties could add new renderers, and this code can be refactored toward parallelizing the `lint` step.
This has a small behavioral change: we no longer hide "autofix" messages. I'd like to generally simplify the number of amend/autofix flags here, so this edges us toward that slightly.
Test Plan: Ran `arc lint` with all the `--output` flags in states with warnings and no warnings, saw sensible-seeming behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12996
Differential Revision: https://secure.phabricator.com/D18645
Summary:
Ref T12996. See also T9749. This option is the opposite of `--lintall`, and means "don't show warnings on changed lines".
However, it's confusing (and I think no one uses it), and we can reasonably infer what you mean if you give us `--rev`. Simplify this logic a bit and choose the "all / only changed" behavior based on the only sensible thing we can reasonably do given the flags.
As with all the other cruft here, I'm pretty sure this came from FB.
Test Plan: Grepped for `only-changed`, `lintAll`, created this revision.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12996
Differential Revision: https://secure.phabricator.com/D18644
Summary: Ref T12996. This is another half-baked Facebook-specific feature. Clear it out of the way so `arc lint` can be modernized more easily.
Test Plan: Grepped for `cache`, created this revision.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12996
Differential Revision: https://secure.phabricator.com/D18643
Summary:
Ref T12996. Fixes T9749. This is a very old Facebook-specific thing which relies on other server-side Facebook-specific things and doesn't work properly anyway.
I don't actually remember what the use case for this flag was. It was either "the codebase has a million warnings, so showing warnings on files/lines you touched isn't good enough", or "weird warnings that raise lint on other lines", or some variation of those.
Regardless, I believe this feature benefits at most one install (Facebook circa 2011), and likely zero. It occasionally confuses normal users.
T9749 suggests a possible replacement workflow which is likely more practical, but I'd like to see a real problem description before considering this again.
Test Plan: Created this revision, grepped for `only-new`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12996, T9749
Differential Revision: https://secure.phabricator.com/D18642
Summary: Ref T12996. This was a very old FB-specific feature that caused some kind of server-side lint thing to run. You can/should do this now with Harbormaster instead. Nothing else ever used it and it has no role in the upstream.
Test Plan: Grepped for `asynclint`, created this revision.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12996
Differential Revision: https://secure.phabricator.com/D18641
Summary: Ref T12996. See PHI84. This is a remnant from long ago that no longer functions.
Test Plan: Created this revision. Grepped for `no-diff`. Grepped for `diff-result`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12996
Differential Revision: https://secure.phabricator.com/D18640
Summary:
SKIP lines, for instance, often have no UserData; there is no
reason to display a content-less blank line.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18632
Summary: Ref PHI48. If a patch removes all of the lines at the end of a file, we can get some array index errors.
Test Plan: Added failing test, made it pass.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18631
Summary:
Fixes T12981.
See new `arc lint` output: P2071
See new `arc unit` output: P2072
Test Plan: Ran `arc unit/lint/diff` and observed new error instead of a Conduit error
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12981
Differential Revision: https://secure.phabricator.com/D18603
Summary:
Fixes T12981.
See new `arc lint` output: P2071
See new `arc unit` output: P2072
Test Plan: Ran `arc unit/lint/diff` and observed new error instead of a Conduit error
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12981
Differential Revision: https://secure.phabricator.com/D18603
Summary: Ref T9846. See PHI48. See D18538 for a similar fix. We can contract the suffix lines too much if, e.g, a newline after another newline is removed. Prevent contraction to fewer than 0 lines.
Test Plan: Added a failing test, made it pass.
Reviewers: chad
Reviewed By: chad
Subscribers: alexmv
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18541
Summary: Ref T9846. See PHI48. For replacing text in the form "ABC" with "ABBC", the trimmer had a bug.
Test Plan: Added failing tests, fixed 'em.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18538
Summary: Fixes T8291. See PHI52. This is papering over the real issue (T8298) but it's a 10-second patch so just improve things slightly for now.
Test Plan: Ran `arc version` locally; patch confirmed on a Windows system by an affected user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8291
Differential Revision: https://secure.phabricator.com/D18518
Summary: Fixes T9846. This restores the last missing feature, ANSI highlighting of diff sections.
Test Plan:
Added a mode so we can actually test this stuff, activated that mode, wrote unit tests.
Did a bunch of actual lint locally too and looked at it, all seemed sane.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18512
Summary: Ref T9846. This was dropped when I refactored how things are rendered; restore it.
Test Plan: Added unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18511
Summary:
Ref T9846. This rewrites the rendering algorithm in a mostly-compatible way and fixes the major issue.
- Includes test coverage for removing a newline, from T12765.
- Includes test coverage for mangling an XML tag, from T9846.
This omits two features, which I'll port forward separately:
- For one-line patches, highlighting the patched section.
- For zero-line patches, putting a little caret ("^") under the character where the warning occurred.
I'll restore these features in a followup change.
Test Plan: Ran unit tests, linted a few things.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18510
Summary:
Ref T9846. Sometimes, a lint message says to replace "the big bad wolf" with "the huge bad wolf": that is, the original and replacement text are the same at the beginning, or the end, or both.
To make this easier for humans to understand, we want to just show that "big" is being replaced with "huge", not that the entire phrase is being replaced.
This logic currently happens inline in console rendering. Pull it out and cover it so a future change can simplify console rendering.
Test Plan: Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18509
Summary: Ref T9846. The algorithm here is fairly invovled, so lay down some test coverage before breaking it.
Test Plan: Ran tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9846
Differential Revision: https://secure.phabricator.com/D18508
Summary:
D13794 changed ArcanistPHPCloseTagXHPASTLinterRule to ignore inline HTML blocks, but selectDescendantsOfType returns an AASTNodeList (which always exists).
Instead, check that the count() of the node list is > 0.
empty.lint-test had to be changed, it wouldn't have been accepted had this rule not been broken before it was commited.
Added tests to cover ArcanistPHPCloseTagXHPASTLinterRule in the future.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D18271
Summary:
See PHI13. This was introduced a very, very long time ago in D311 and D312, and I think T168 was the original report.
It prevents `arc` from being used in some semi-reasonable (maybe?) automation workflows where you're hooking some version of "Land Revision" up to `arc land`. This isn't necessarily the right approach, but I think the concession here to make this work is small.
Running `arc` against another copy of `arc` makes `arc unit` not work, but we provide a good error message. Most other `arc` operations still work correctly.
All of these situations are bizarre edge cases but I think we can safely warn and continue here. Even if we revert this behavior later, almost no one should be affected, since this essentially only impacts users developing `arc` itself.
Test Plan: Ran one copy of `arc` against another, saw a warning instead of an error. `arc unit` failed, but with a good error.
Reviewers: chad, jmeador
Reviewed By: jmeador
Differential Revision: https://secure.phabricator.com/D18264
Summary:
`PhutilConsole->confirm()` is vestigial as noted in efcd70c, as
`PhutilConsole` may be deprecated sometime in the future. `PhutilConsole`
was developed as a tool for T4281 but the feature has been removed
since.
Replace existing occurences of the `PhutilConsole->confirm()` pattern with
`phutil_console_confirm()`. There should be no change in functionality
since the two functions are interchangeable.
Test Plan: Manually tested by running `bin/arc lint`, `bin/arc diff --preview`, `bin/arc land --preview`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, alexmv
Differential Revision: https://secure.phabricator.com/D18185
Summary:
Fixes T12815. During the last update to "arc land", some flags were disabled but remained in place in case we needed to retain them.
It now seems reasonably clear that we do not. The "rebase" and "merge" strategies for landing were replaced by a better "headless" strategy which seems to avoid the original issues, so these flags no longer do anything or reasonably could do anything.
`--delete-remote` is slightly more ambiguous (e.g., see T12650 and maybe others) but the only real use case is "git push = save changes".
Test Plan:
Ran `arc land --update-with-rebase`, was told the flag does not exist.
Grepped for affected flags/symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12815
Differential Revision: https://secure.phabricator.com/D18108
Summary:
If the commit does not exist locally, aborting still leaves
the user checked out on the branch. In nearly all cases, all that is
necessary is a fetch -- but the branch must also be cleaned up. This
leads to the pattern of:
```
arc patch D12345
[...base commit does not exist...]
^C
git checkout master
git branch -D arcpatch-D12345
git fetch
arc patch D12345
```
Solve this common problem by simply trying to fetch once if the commit does not
exist locally.
Test Plan: Ran `arc patch` on a recent diff.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17949
Summary: Ref T12655 - this change properly detects trailing spaces or tabs (or combinations of thereof) on end of lines.
Test Plan: Use Text lint with trailing whitespace rule on files with spaces, tabs or combinations of thereof. Should properly detect and fix all those.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Itms
Maniphest Tasks: T12655
Differential Revision: https://secure.phabricator.com/D17814
Summary:
Ref T12651. Ran into these during D17799:
- Use `getStatusCode()` to put the actual status code into the message.
- If we fail but wrote an empty file to reserve the filename, clean it up.
Test Plan:
- Faked the error, `phlog()`'d the exception.
- Saw sensible exception message.
- Saw empty file get cleaned up.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12651
Differential Revision: https://secure.phabricator.com/D17800
Summary: Fixes T12555.
Test Plan:
Added this class to the codebase and ran `arc liberate`:
```
<?php
class FooBar {
public static function doTheFoo() {
return 'foobar';
}
}
```
Ran `arc lint` and observed this warning:
```
Warning (XHP87) Class Not `abstract` Or `final`
This class is neither `final` nor `abstract`, and does not have a
docblock marking it `@concrete-extensible`.
1 <?php
2
>>> 3 class FooBar {
4 public static function doTheFoo() {
5 return "foobar";
6 }
```
Added a `final` modifier to `FooBar`'s declaration and observed the warning went away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12555
Differential Revision: https://secure.phabricator.com/D17787
Summary:
Many other status updates (such as "Builds passed!") show up
in bright background colors, making them more salient than a final
fatal "Exception".
Make exception reporting be just as colorful, so it stands out.
Test Plan:
Added an explicit `throw new Exception("!!!")` and saw it
in red.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17748
Summary: Fixes T12464. Moves "arc upload" to SHA256 where applicable.
Test Plan: Ran `arc upload` against a server with D17620 twice, saw it skip the actual upload the second time.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17622
Summary:
Ref T12464. This is similar to D17619 and prepares us to move to SHA256 in the client.
Note that it's fine if `arc` and Phabricator disagree about hashing algorithms. We don't really trust the client anyway, so if things are mismatched clients will just end up transferring a bit more data instead of getting to cheat when Phabricator already has copies of data.
Test Plan: Ran `arc upload`, got a clean upload.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17621
Summary: Fixes T8348. Just use normal HTTP GET to download files if the server is sufficiently modern.
Test Plan: Downloaded various files with `--as`, `--show`, large files, small files, old server, new server.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8348
Differential Revision: https://secure.phabricator.com/D17614
Summary:
Because the character offset group is optional, the logic for dealing
with the previous index-based match object was more complex than it
needs to be. Switching to named groups makes this clearer.
As an example of how this was causing a problem, when the character
group *was* present (at index 3), it was being appending to the linter's
name in the call to ->setName(), resulting in confusing linter output.
We may have also been setting the character offset to the error code's
string, too, which is further nonsense.
Because we reliably capture the flake8 error code, I don't think there's
a need to append it to the linter's name at all now, so I removed that
part (so it's always just `flake8`), but it's not a problem to restore.
Lastly, I hoisted `$regex` out of the loop because it's a constant and
de-indenting it gave me enough room to continuing writing it all on one
line.
Test Plan: Tested primarily with flake8 3.3.0
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17552
Summary:
Ref T2794. This doesn't do anything useful, but since I'm planning to start using this stuff to do real deployments I don't want to lose it if my laptop gets hit by a bus.
This just adds a skeletal `bin/phage` with enough code that I can add actual workflows to Phacility repositories and get `bin/phage remote --hosts ...` working without the code only existing on my laptop.
Test Plan: Ran `bin/phage`, saw nothin'.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2794
Differential Revision: https://secure.phabricator.com/D17379
Summary: See D17357
Test Plan: invoke and still see output in English?
Reviewers: #blessed_reviewers, joshuaspence, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17362
Summary: See T12266. Warn when the user tries to set a value we will probably not read.
Test Plan: `arc set-config foo bar`, see fancy warning.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17357
Test Plan:
Removed a submodule with `diff.submodule` set to `log`, saw
`arc diff` error; with this change, it no longer does.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10881
Differential Revision: https://secure.phabricator.com/D17327
Summary:
Fixes T8937. Previously when running `arc patch D9999999999` or `arc export --revision 99999999` with a non-existent diff or revision ID you would get a rather unhelpful error message. Now you'll get a slightly more helpful error message:
```
$ arc patch D99999999
Exception
Couldn't find a revision or diff that matches the given ID
(Run with `--trace` for a full exception trace.)
```
Test Plan: Ran arc patch with a valid revision and saw it patch successfully. Ran again with an invalid revision, saw the error message.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T8937
Differential Revision: https://secure.phabricator.com/D17325
Summary:
Fixes T12069. We implement "arc diff --reviewers" (and "--cc") by parsing a faux message with "Reviewers: ...".
After D17122, the first line of the message is always interpreted as a title, so the text ends up in the message body.
Instead, use a placeholder title so these fields are never initial fields.
Test Plan: Ran `arc diff --reviewers dog`, got only one "Reviewers" field.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12069
Differential Revision: https://secure.phabricator.com/D17147
Summary:
`git ls-remote` has an unusual way to indicate a URL was not
found: echoing back user input
```
$ git ls-remote --get-url does_not_exist
does_not_exist
$ echo $?
0
```
`getRemoteURI` handles checking for remotes other than 'origin', but
the error handling always matched against the string 'origin'
regardless of remote name.
Test Plan:
With a git config along the lines of:
```
[remote "my_special_name"]
url = ssh://secure@secure.phabricator.com/diffusion/ARC/arcanist.git
fetch = +refs/heads/*:refs/remotes/my_special_name/*
[branch "master"]
remote = github
merge = refs/heads/master
[remote "github"]
# url = git@github.com:phacility/arcanist.git
fetch = +refs/heads/*:refs/remotes/github/*
```
and running in a branch tracking `master` (github). `arc which` would
(without this diff) show:
```
The remote URI for this working copy is "github".
```
With this diff, `arc which` correctly shows:
```
Unable to determine the remote URI for this repository.
```
When diffing against a tracking branch with a propertly configured
remote (the happy path), `arc which` still correctly identifies the
remote URI:
```
The remote URI for this working copy is
"ssh://secure@secure.phabricator.com/diffusion/ARC/arcanist.git".
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, chad, epriestley
Differential Revision: https://secure.phabricator.com/D17110
Summary: Ref T10895. This allows `arc browse <commit name>` to resolve to a revision, if an associated open revision exists.
Test Plan: Ran `arc browse` with various arguments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10895
Differential Revision: https://secure.phabricator.com/D16933
Summary:
Ref T10895. Currently, the "browse as a commit" code does these lookups, but the code can't be reused.
For T10895, I want to introduce "browse as revision", but it will need to do the same ref lookup. Separate this as a hardpoint so the code can be shared via hardpoint/ref infrastructure.
Test Plan: Ran `arc browse master` and similar commands.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10895
Differential Revision: https://secure.phabricator.com/D16929
Summary:
Ref T10895. This modularizes Arcanist settings, but doesn't do anything with them yet.
When users type `arc browse X`, and only provide one "X", and there are several ways to interpret it, prompt them to choose one.
Test Plan: Created a file named `T234`, ran `arc browse T234`, was prompted to interpret it as an object or a path.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10895
Differential Revision: https://secure.phabricator.com/D16928
Summary:
Ref T10895. This mostly modularizes `arc browse` and puts it on ref/hardpoint infrastructure. Feels okay-ish? Major gripes:
- Messaging for "some stuff won't work because you're in a random directory, not a working copy" could be better, but I think I want something like the "Guidance" infrastructure for this.
- The `requiresStuff()` / `desiresStuff()` interactions on Workflow continue to feel bad, but I think I can sneak by without fixing those for now.
- I want to improve some of the other UI/UX stuff but this diff is already gigantic.
Test Plan: Ran `arc browse .`, `arc browse master`, `arc browse README.md`, inside and outside working directories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10895
Differential Revision: https://secure.phabricator.com/D16925
Summary:
Ref T11355. Ref T10895. Ref T11518. This is heading to `experiemntal`. This may or may not be a good idea, but basically it's a more generic version of `Query` classes in Phabricator.
This starts creating generic objects ("CommitRef", "BranchRef") which have attachable properties, like many Phabricator objects do. Here, they're formalized (and theoretically extensible), as "hardpoints".
So: a hardpoint is something on an object which you can attach stuff to, but which we don't start with the data for.
All of the logic for actually figuruing out how to attach stuff to hardpoints is also modular. `Loader` classes have code for loading stuff onto objects. For example, `ArcanistMercurialBranchCommitHardpointLoader` knows how to run `hg log` to build the commit for a branch.
One issue is that `arc feature` in Mercurial is 100% bookmarks, so maybe I should actually be making `ArcanistRefRef` here. But we can probbbably deal with that later.
This moves us somewhat closer to T11355 and T11518, although the immediate thing I want to do with it is define an `ArcanistObjectNameRef` and use hardpoints to load URIs for it for T10895.
Overall, I expect this will see some revision in future changes, and perhaps most of it will go away.
Test Plan: Ran `arc branch` / `arc feature` in Git and Mercurial repositories.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T10895, T11518, T11355
Differential Revision: https://secure.phabricator.com/D16857
Summary:
Ref T10895.
NOTE: I'm going to land this and other changes to a new `experimental` branch until `arc` is more substantially rebuilt, since everything I touch feels like it requires me to rebuild 30 other things first.
Currently, many `arc` workflows are unnecessarily slow because they call `conduit.connect` on startup. There's no need to do this with the modern way the API works, and we've generally moved away from explicit version testing to more granular capability testing on specific workflows.
Additionally, some workflows like `arc patch` are huge messes (see T11434) because they're trying to run in anonymous mode but it doesn't really work with all the upfront stuff Conduit does now. It's not possible, in the general case, for a workflow to know upfront if it needs Conduit or not.
And:
- `ArcanistWorkflow` has piles of Conduit logic, but should not.
- Pooling Conduit requests isn't very easy.
- There's a lot of general cruft around the workflow.
- We should drop certificate support.
This pulls out Conduit into a separate on-demand class with modern support, future pooling, less cruft, inline handling of login issues, and generally less garbage.
Also adds an `--anonymous` flag, mostly to make testing easier.
Test Plan: Ran `arc browse`, used `--anonymous` and `--trace`, fiddled with credentials, got approximatley the same behavior that mainline `arc` has.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T10895
Differential Revision: https://secure.phabricator.com/D16921
Summary: Fixes T11758. This was one some spooky magic but is more-or-less a normal config setting now.
Test Plan:
- Ran `arc get-config`, saw help about "aliases".
- Ran `arc set-config aliases`, saw guidance about using `arc alias`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11758
Differential Revision: https://secure.phabricator.com/D16745
