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
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
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
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
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
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
Summary:
See PHI162. This corrects a couple more bugs:
- If the old file didn't end in a newline, we could end up printing two lines next to each other in the output.
- If the patch targeted "Line 6, character 1" instead of "line 5, character 3" in a file "1\n2\n3\n4\n5\n", we would fail to figure out what that meant when computing an offset because the last line has 0 characters on it.
Test Plan: Added failing unit tests, made them pass. Also tested with some fake linters similar to the ones described in PHI162.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18716
Summary: See PHI136. These are already optional on the server side in `HarbormasterBuildLintMessage`, and effectively mean "file-level issue", which is a bit niche but not unreasonable.
Test Plan: Checked that `HarbormasterBuildLintMessage` doesn't care if these keys exist, created this revision.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18711
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
