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 D18651. This was missing an `unset()`.
Test Plan: Looked carefully.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18766
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:
Experimental branch. Ref T2543. Depends on D18742.
Add an "arc diff --draft" flag which holds revisions as drafts indefinitely.
Test Plan: Ran "arc diff --draft" when creating; ran "arc diff --draft" to try to update a revision and got a failure.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18743
Summary:
Depends on D18741. Switches to "differential.revision.edit" if we get a transaction list back from parsing (added by D18740).
This will allow us to use new transactions, notably "Hold as Draft" for "--draft".
Test Plan: Ran `arc diff`, got a revision via `differential.revision.edit`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18742
Summary:
Experimental branch. Depends on D18739. Ref T2543.
To prepare for `--draft`, consolidate `--preview` and `--only`.
The old `--preview` is now `arc diff --only`.
The old `--only` is now `arc diff --only --nounit --nolint`.
I think this is more clear than other ways we could do it, since the difference between "draft" and "preview" seems less clear than the difference between "draft" and "only".
Test Plan: Ran `arc diff --only` and `arc diff --only --nounit --nolint`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18740
Summary:
Experimental branch. Depends on D18738. Ref T2543.
To prepare for adding `--draft`, clean up some of the flags behaviors.
`--only` currently means `--preview --nolint --nounit`. I'm going to:
- Remove `--only`.
- Rename `--preview` to `--only`, since I think `--only` is a better flag for this behavior than `--preview`.
- If you want the old `--only`, you can `arc alias` it to `arc diff --only --nolint --nounit --browse` or similar.
Test Plan: Grepped for `'only'`, `only`, etc.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18739
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: Depends on D18651. Allows Mercurial to auto-detect depends-on revisions.
Test Plan: See D18652 and D18653. Didn't actually test the empty repository states but they're probably okay. The `experimental` branch is an untamed wild. ¯\_(ツ)_/¯
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18654
Summary:
Depends on D18645. Fixes T11343. Currently, when you "arc diff", we don't detect dependencies even if your working copy is on top of another open revision you authored.
Use the new Ref/Hardpoint infrastructure from the `experimental` branch to look up the revision associated with the base commit when you run `arc diff`. If it exists and there's exactly one open revision you authored associated with it, add "Depends on ..." to the summary. This is a bit rough but the whole `experimental` branch is a bit of a wilderness for now.
Also remove `--cache` (passthru flag for `arc lint --cache`, which I removed in D18643) and `getUnderlyingWorkingCopyRevision()` (no callsites).
(This won't yet work in Mercurial, and does not make sense in SVN.)
Test Plan: Created this revision, got an auto "depends on" up top there.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11343
Differential Revision: https://secure.phabricator.com/D18651
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
