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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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:
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:
`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: 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
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: 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
Summary:
In php 7, DOMDocument::loadXML emits an error when supplied with
an empty string as input. For example, I got this error:
ERROR 2: DOMDocument::loadXML(): Empty string supplied as input
This change simply checks for empty and returns an empty array
rather than attempting to parse an empty xml document.
Test Plan: ran `arc diff` on a repo that uses nosetestengine
Reviewers: #blessed_reviewers!
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16672
Summary:
Fixes T11744. Because intraline diffs are expensive to generate, we already bail out and decline to generate them for very long lines.
However, we currently split the inputs into lists of characters first, then check how long they are and make a decision to bail. For //huge// inputs (e.g., 1MB+), this is too late: just splitting them has a large CPU/RAM cost.
(These inputs are rare in normal source, but can appear in, e.g., JSON files written without newlines.)
Instead, add an extra "are the inputs really huge?" check first, and bail early if they are.
Test Plan:
- Generated a 1MB "change a file full of Q to a file full of R" diff.
- Before change: purged changeset cache; took about 7 seconds to load.
- After change: purged changeset cache; took about 1 second to load.
- Viewed some normal diffs to make sure intraline edits still displayed correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11744
Differential Revision: https://secure.phabricator.com/D16683
Summary:
Paths are passed into linters using UNIX-style slashes (/), as returned from the version control system; however,
`Filesystem::readablePath` swaps them to Windows-style (\) on
Windows when storing the names of the files with lint messages. This causes no lint message's path to match the set of
changed files, and thus no lint warnings are ever produced.
If a lint message's file is not found using the provided filename, also try looking up the UNIX-style filename, on Windows when determining if a lint mesage is "relevant."
Fixes T11248.
Test Plan: Ran `arc lint` on Windows.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T11248
Differential Revision: https://secure.phabricator.com/D16579
Summary: Fixes T9692. Instead of disallowing API tokens entirely, we're going to just warn the user that they might not want to do that. After that, they can proceed if they want to.
Test Plan:
Run arc install-certificate.
Manually go to `Settings → Conduit API Tokens` in the web UI.
Generate an API token explicitly, which should have the form api-******.
Paste that into the prompt on the CLI.
It will give you a warning prompt then ask if you'd like to proceed anyway (defaults to No).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T9692
Differential Revision: https://secure.phabricator.com/D16448
Summary:
The behavior (and name) of this function was changed in
D16405, but the documentation was not updated to reflect the new
contract.
Test Plan: Untested; pure doc changes.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16425
Summary:
Since 737f5c0df9 arcanist shows [y/N] two times, when arcanist asks you, if you want to proceed, if you want to update a not owned revision. Ths patch fixes
that, so that Arcanist shows [y/N] only once, like at other situations, when Arcanist asks you a question.
Ref T11489
Test Plan: Updated a not owned revision.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Tags: #arcanist
Maniphest Tasks: T11489
Differential Revision: https://secure.phabricator.com/D16415
Summary:
Ref T7148. When uploading files from the CLI with a particular view policy, we may not respect it if the file is unique (so the data isn't already known) and small (so it doesn't invoke the chunker).
This is rare (and may never have happened outside of testing) because:
- production dumps are always larger than the minimum chunk size;
- only cluster stuff uses `setViewPolicy()`;
- the default policy is "Administrators" anyway, which is safe.
However, I caught it in local testing, so fix it up.
Test Plan: Used `bin/host upload --file ...` to upload a small, unique file. Verified it uploaded with the correct custom view policy ("No One") rather than the default view policy ("Administrators").
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7148
Differential Revision: https://secure.phabricator.com/D16408
Summary:
The previous parser failed when only one of the old/new filenames was
quoted, as with a rename of a Unicode filename to a non-Unicode
filename, or vice versa. It also incorrectly parsed custom prefixes,
even going to far as to encode this logic in the tests: a diff of
"src/file dst/file" which is not a rename should not be recorded as
changing "src/file", but rather "file".
Switch to using the "rename from" / "rename to" as the source of truth
for old and current filenames when complex renames are done. This
matches the logic that git itself uses to parse patches; the contents
of the `diff --git` line are merely viewed as a simplest-case
prediction, with renames handled later should it not match.
The diff parser already had logic to parse "rename from" / "rename to"
lines and extract their information. As such, this diff consists
primarily of removing logic from the `splitGitDiffPaths` method, and
allowing it to quietly fail.
This resolves two ambiguity mentioned in comments and tests: in
renaming "old file old" to "file", as well as the renaming of
"old file with spaces" to "new file with spaces" when neither are
quoted.
Test Plan:
`arc unit`. Many of the existing test cases no longer
applied to the `splitGitDiffPaths` method; they were moved into a new
test method which also supplies values for "rename from" and "rename
to" lines.
As noted in the summary, this alters one of the expected values of an
existing test. Specifically, the following diff is a file addition of
`file` with custom prefixes, and not the addition of "dst/file":
```
diff --git src/file dst/file
new file mode 100644
index 0000000..1269488
--- /dev/null
+++ dst/file
@@ -0,0 +1 @@
+data
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16405
Summary:
`parseGitRawDiff` dealt with the `A`, `M`, and `D` status flags from
`git diff --raw`, for file additions, modifications, and deletions
respectively. However, it failed to cope with `C` and `R` flags, for
copies and renames. Git version 2.9 and above default to resolving
renames, even in `git diff --raw` output, making this lack of support
only salient now (though users with Git's `diff.rename` set
encountered it previously).
Those two flags differ from the other three in that they offer both the
source and destination filename, separated by a tab. As
`parseGitRawDiff` was not aware of this property, it returned a
"filename" of `"oldfile\tnewfile"`. This is surfaced in several
places, including as passed to linters as a filename to check.
Needless to say, this file is nearly guaranteed to never exist on
disk.
Detect both the `C` and `R` flag types, and generate either a file
addition, or a pair of addition/deletion entries.
Test Plan:
Renamed a file, with a linter that printed each file it was
called with.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: jboning, Korvin
Differential Revision: https://secure.phabricator.com/D16387
Summary:
Fixes T11435. This isn't a perfect solution since there's a little code duplication, but a perfect solution is probably a bit more involved.
See T11435 for some discussion. In particular, most `git diff` commands already get this flag via `ArcanistGitAPI->getDiffBaseOptions()`.
Test Plan: Will land this change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11435
Differential Revision: https://secure.phabricator.com/D16375
Summary: Ref T11409. Add lint to detect using `[...]` to define arrays. This doesn't work in PHP 5.2/5.3, which some of our users run.
Test Plan:
- Ran `arc unit`.
- Ran `arc lint src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php` to detect the issue in T11409.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11409
Differential Revision: https://secure.phabricator.com/D16357
Summary: Fixes T7489. Depends on D16332, which moved this code to libphutil.
Test Plan:
```
$ arc banch --bystatus
(Assuming 'banch' is the British spelling of 'branch'.)
(Assuming '--bystatus' is the British spelling of '--by-status'.)
...
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7489
Differential Revision: https://secure.phabricator.com/D16333
Summary:
The `cp` and `mv` commands, when run from a Windows
environment, error out. Use library functions from `Filesystem`
for cross-platform compatibility.
Test Plan:
Ran `arc lint` with auto-fix lint errors on both Linux and
Windows.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Spies: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16273
Summary: Fixes T11222. This was lazy-future-proofed for Conduit SSH support, but users are boundlessly creative. Check protocols explicitly.
Test Plan:
```
$ arc install-certificate a.b:1/
Usage Exception: Server URI "a.b:1/" must include the "http" or "https" protocol. It should be in the form "https://phabricator.example.com/".
```
- Also went through a successful workflow with a URI in the form provided in the example.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11222
Differential Revision: https://secure.phabricator.com/D16188
Summary:
Ref T4631. Ref T10939. I don't have any good solutions here; this is perhaps the least-bad one.
- This prompt is misleading/confusing in the presence of Herald/Owners.
- This prompt is likely of very little value for experienced reviewers.
- When it works, this prompt may be of some value for new reviewers, but getting it wrong is probably more confusing than getting it right is helpful, and there is a more accurate version of the warning in the web UI that new users are likely to see.
- In the long run, this code should not live in the client.
Test Plan: Created this revision without specifying reviewers, probably didn't get prompted.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4631, T10939
Differential Revision: https://secure.phabricator.com/D16139
Summary: Ref T10227. This converts weird hard-codey magic to the new HTTPEngineExtension.
Test Plan: See D16090.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10227
Differential Revision: https://secure.phabricator.com/D16091
Summary: Ref T7643. This moves the prefix/suffix smoothing behavior back to the old one, which seems better for code diffs.
Test Plan: `arc unit --everything`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16074
Summary:
Ref T7643. We've done the smoothing from D16068 for a long time, it just wasn't part of EditDistanceMatrix.
Since it now is, call it instead of keeping a copy of the logic around.
Previously, the unit tests were testing un-smoothed diffs. Just have them test smoothed diffs instead, since nothing actually uses unsmoothed diffs.
Test Plan: Pasted a raw diff into the web UI, got a sensible inline diff for it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16069
Summary: See D15828 - arc is reporting file size as `0` for unexisting files - make it stop.
Test Plan: `arc diff` with empty, deleted, added files - see size reported as `null` when appropriate.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin
Differential Revision: https://secure.phabricator.com/D16059
Summary: Fixes T10431. Updates the getAliases() method to read every `arc` configuration file.
Test Plan:
Added an `arc duck` alias to `/etc/arcconfig`, ran `arc duck`, saw "quack".
Added an `arc pig` alias to `~/.arcrc` via `arc alias`, ran `arc pig`, saw "oink oink".
Reviewers: nevogd, chad, #blessed_reviewers
Reviewed By: chad, #blessed_reviewers
Subscribers: eadler, epriestley
Tags: #twitter
Maniphest Tasks: T10431
Differential Revision: https://secure.phabricator.com/D15342
Summary:
When doing svn copy, or svn mv, a SynthenticAdditionDiff is generated.
If the path is a directory, an error will occur when checking the
mime-type of the directory. Immediately after the properties check,
the function returns null if the path is a directory. Move this
check to before the properties check to avoid exiting with an error.
```
Command failed with error #1!
COMMAND
svn propget 'svn:mime-type' '/home/trasz/svn/ports/cad/py-pycam'@
STDOUT
(empty)
STDERR
svn: warning: W200017: Property 'svn:mime-type' not found on '/home/trasz/svn/ports/cad/py-pycam@'
svn: E200000: A problem occurred; see other errors for details
(Run with `--trace` for a full exception trace.)
```
Test Plan: Created differentials of changes with `svn copy` and `svn mv`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Tags: #subversion
Differential Revision: https://secure.phabricator.com/D15985
Summary:
`arc which` is currently broken for svn repos.
Fixes T6635
(I think I independently wrote an identical change to yours)
Test Plan: `∴../../p/arcanist/bin/arc which` on a svn repo.
Reviewers: aik099, epriestley, #blessed_reviewers
Reviewed By: aik099, epriestley, #blessed_reviewers
Subscribers: aik099, Korvin
Maniphest Tasks: T6635
Differential Revision: https://secure.phabricator.com/D15922
Summary:
See rARC3ffed59bd7. Currently, when a unit test includes a syntax error, it is raised in an unclear way ("error at line 10, char 1: XHP1 Unknown lint message!").
This is because each test case only activates rules it wants to test, so we lose the ID/name for the syntax message. However, we always want to test this and the lint engine can always raise it.
To get a better error message, include it unconditionally. So a test for rule `X` really tests two rules: syntax, and `X`.
Test Plan:
Ran `arc unit` at HEAD, got a better test failure:
```
FAIL ArcanistCallTimePassByReferenceXHPASTLinterRuleTestCase::testLinter
In 'call-time-pass-by-reference.lint-test', expected lint to raise error on line 10 at char 8, but no error was raised. Actually raised:
error at line 10, char 1: XHP1 PHP Syntax Error!
```
NOTE: This doesn't pass tests yet, it just makes the test failure easier to understand. I'll see about fixing the test in the next change.
Reviewers: chad, richardvanvelzen
Reviewed By: richardvanvelzen
Differential Revision: https://secure.phabricator.com/D15819
Summary: See D15678. A node containing a return type hint is added right before the statement body node. This updates all relevant linter rules to now retrieve the body from the correct index.
Test Plan: Ran `arc unit --everything`, inspected every single message to make sure all xhpast test cases succeeded. Also grepped for `getChildByIndex(5` and `getChildOfType(5`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15814
Summary: It is currently not possible to apply multiple linter standards because `$value` was used instead of `$standard`.
Test Plan: Was able to apply multiple linter standards.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: eadler, thaiphv, Korvin
Differential Revision: https://secure.phabricator.com/D15212
Summary:
Flake8 extensions are allowed to use their own letter-prefixed codes. For
example, the flake8-debugger extension emits 'T002'-tagged messages.
This change relaxes getLintCodeFromLinterConfigurationKey() to also recognize
extension codes. Otherwise, attempting to configure message severities for
e.g. 'T002' would result in an exception.
Messages from extensions continue to default to ERROR severity, as they did
before this change.
Test Plan:
Successfully reduced the severity of 'T002' to a warning via .arclint:
"severity": {
"T002": "warning"
}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15810
Summary:
This is mostly just a personal quality-of-life fix. I run this command fairly often and having it return a little faster is nice.
This replaces a `git show` for each individual branch with a big `git for-each-ref` which we were already running anyway. This is quite a bit faster.
This command also occasionally hangs or segfaults for me while executing the huge pile of subprocesses. This is unreliable to reproduce, probably some bug in some PHP extension I have, and likely hard to narrow down, and this approach is better in every way anyway.
Test Plan:
- Ran `arc branch` in Git, observed faster output (in my `phabricator/`, about 2000ms -> 1200ms).
- Ran `arc feature` in Mercurial.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15735
Summary:
It is common practice in Wikimedia's projects to amend a contributor's
change without taking over authorship of the change. We found that
the only enforcement of commandeering before amending is in arcanist,
not validated server-side. While it would be fairly straightforward to
maintain this as a patch to arcanist, I thought I would see if upstream
is willing to support making this optional.
With this change, amending without commandeering is enabled by a flag in
`.arcconfig` and it defaults to the old behavior.
For background see [wmf T121751](https://phabricator.wikimedia.org/T121751)
Test Plan:
* ran `arc patch D146` to locally apply a revision that I did not author,
* made a trivial change and amended the commit.
* ran `arc diff --update D146 HEAD^` to send the update to differential
* Saw that https://phabricator.wikimedia.org/D146 updated as it should.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: greggrossmeier, aklapper, Luke081515.2, Korvin, dereckson
Maniphest Tasks: T10584
Differential Revision: https://secure.phabricator.com/D15468
Summary:
Fixes T10707. Currently, `arc backout` creates a commit message which includes questionably-helpful "tips" in the message itself.
Strip these out.
Test Plan:
Used `arc backout` to revert any commit, then `git show` to see the generated message.
- Before patch: included tips.
- After patch: no tips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10707
Differential Revision: https://secure.phabricator.com/D15573
Summary:
Remove couple of references to callsigns:
- `arc which` now prints repository name
- `getShouldAmend()` can now use new format of commit name
a quick git-grep looks like the remaining references are all about `repository.callsign` config.
Ref T4245
Test Plan:
- `arc which` on a repository with no callsign
- trigger `requireCleanWorkingCopy()`, see both "Do you want to amend this change" and "Do you want to create a new commit" prompts.
- fire this diff with new code.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15472
Summary:
Ref T10093. Right now, Phabricator kind of guesses that `arc` probably pushed stuff to the staging area.
This can cause confusing/misleading errors later, if it didn't actually push.
Instead, tell Phabricator that we pushed, so we can raise more tailored messages in the web UI (e.g., make "Land Revision" say "this wasn't pushed to the staging area" instead of "whoops, error!!~").
Test Plan:
Ran `arc diff` a few times, then looked in the database for properties.
{F1161655}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10093
Differential Revision: https://secure.phabricator.com/D15426
Summary:
Fixes T10509. Pushing changes to staging can be inefficient. What happens, roughly, is:
- Master is at commit "W" -- "W" is the most recent published commit in the main repository.
- The local working copy has one change on top of that, "X", so its history is commits "A, B, C, D, E, F, ..., U, V, W, X".
- The remote has some other previous changes that I or other users have made, maybe like "A, B, C, ..., S, T, U, Y" and "A, B, C, ..., T, U, V, Z", from previous pushes to staging areas.
- "X", "Y" and "Z" will never actually make it to master, because they'll be squash-merged/amended by `arc land`.
So the local says "I want to push 'X'", and the remote says "I know about 'Y' and 'Z', are those in the history of 'W'? You only need to send me new stuff if they are".
But they aren't, so the local says "nope, so here's the whole history for you". This is slow and sends a ton of data that the remote already has over the network.
In theory, Git could use a slightly different algorithm to tell the local about more commits, but this is hard, rarely useful, and not the kind of thing I'd be excited about changing if I was the Git upstream.
Instead, when pushing "X", also push "W", to trick Git into telling future clients about it.
Now, the remote should say "I know about 'W', 'Y' and 'Z'", and the local will say "oh, great, 'W' is in history, here's just the changes since then".
Also, fail `arc diff` if the push to staging fails, and tell users to use `--skip-staging`. This code has been in production for a while and doesn't seem to have any issues, and a failed push to staging prevents builds, lands, etc.
Test Plan:
- Ran `arc diff`, saw two changes push.
- Ran `arc diff --base arc:empty`, saw only one change push.
- Ran `arc diff` with an intentionally broken staging area, saw an error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10509
Differential Revision: https://secure.phabricator.com/D15424
Summary: Fixes T10511. If you `arc browse --branch x/y/z`, we do not encode the URI properly.
Test Plan:
Ran `arc browse --branch x/y/z/ something.c`.
Before, got an error about "x" does not exist. This is wrong; the error should be about "x/y/z".
After, got the proper error:
{F1141096}
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T10511
Differential Revision: https://secure.phabricator.com/D15397
Summary: Clover reports generated from PHPUnit sometimes give false positives of lines that were not covered by a test that should have actually been not coverable, apparently due to inaccurate static analysis of files that weren't actually executed. This filters coverage reports of files that show no coverage which avoids these false positives. Fixes T10420.
Test Plan:
Looked at coverage reports of files before and after the change
Before: {F1124115}
After: {F1124113}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, aurelijus
Maniphest Tasks: T10420
Differential Revision: https://secure.phabricator.com/D15343
