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: 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: 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: 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
