1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-12-29 17:00:58 +01:00
Commit graph

2016 commits

Author SHA1 Message Date
epriestley
2e02332216 Add trailing tabs when generating synthetic Git diffs for files with spaces
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
2018-01-16 13:57:27 -08:00
epriestley
3d06bd4c56 Fix a minor 'buildableStatus' error in "arc land"
See D18837. I made a mistake here with rigging this for testing; this line was
changed incorrectly.
2017-12-26 08:33:35 -08:00
Alex Vandiver
9144658e69 Accelerate working tree operations in git
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
2017-12-23 20:12:50 -08:00
Alex Vandiver
f34467914c Add tests for git submodules, based on current behavior
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
2017-12-23 20:01:20 -08:00
epriestley
c784b56920 Make "arc land" accommodate a minor API change in Harbormaster build statuses
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
2017-12-23 11:39:19 -08:00
Alex Vandiver
249f3a80fe Default the prompt for "Amend HEAD with these patches" to true
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
2017-12-12 01:43:16 -08:00
epriestley
f4c80a114d Make "arc land" prompt on "Changes Planned" revisions more explicit
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
2017-11-30 13:51:50 -08:00
epriestley
9054604214 Fix some lint rendering issues when lines prior to other identical lines are removed
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
2017-11-01 17:18:59 -07:00
epriestley
0989343a4e Correct some lint console renderer issues with files missing trailing newlines
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
2017-10-20 10:49:07 -07:00
epriestley
617c2e46d6 Make "line" and "char" strictly optional in ArcanistLintMessage
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
2017-10-17 14:30:31 -07:00
epriestley
76b54ce0a9 Fix parsing of Git branches with common and useful name "0"
Summary: See <https://discourse.phabricator-community.org/t/arc-has-a-spurious-failure-on-arc-diff/521>. Oh, PHP!

Test Plan: Created a branch named "0", ran `arc diff`. Before: fatal. After: this beautiful revision.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18678
2017-10-04 10:20:48 -07:00
Alex Vandiver
c804c50260 Don't show a blank line if there is no user data
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
2017-09-20 17:54:40 -07:00
epriestley
b836557b12 Correct lint rendering when patching trailing whitespace in files
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
2017-09-20 12:07:41 -07:00
Austin McKinley
df81d79d77 Add explicit limits to unit test/lint error names
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
2017-09-14 12:12:08 -07:00
epriestley
cbc785ddce Fix a similar lint rendering issue when trimming identical lines out of patches
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
2017-09-05 17:12:23 -07:00
epriestley
7371277d20 Fix a prefix/suffix counting issue in Arcanist lint rendering
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
2017-09-05 13:09:46 -07:00
epriestley
d9cb5b18fb Cheat our way through arc version on Windows for the moment
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
2017-09-01 18:06:00 -07:00
epriestley
ad8214456a Restore ANSI highlighting of lint sections to lint output
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
2017-08-31 13:29:18 -07:00
epriestley
213ed3ff15 Restore the caret pointer ("^") for lint lines which only have a character offset
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
2017-08-31 13:28:47 -07:00
epriestley
86779b1526 Render lint patches which have newlines in a less misleading way
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
2017-08-31 13:28:31 -07:00
epriestley
be67df6118 Extract and cover the logic for "trimming" a lint message
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
2017-08-31 13:28:01 -07:00
epriestley
ab0d81bca2 Add basic test coverage for lint console rendering
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
2017-08-31 13:27:44 -07:00
Alex Vandiver
5eda40337b Fix missing whitespace in arc linters --help message
Test Plan: Ran `arc linters --help`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D18344
2017-08-04 13:12:59 -07:00
Asher Baker
836768bdcc Fix ArcanistPHPCloseTagXHPASTLinterRule always bailing out
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
2017-07-22 03:48:37 +01:00
epriestley
7bb8dbabce Reduce the strength of "arc executing on arc" from an error to a warning
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
2017-07-21 12:09:59 -07:00
Stephanie Ren
165df12046 Consistently use phutil_console_confirm().
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
2017-07-06 16:27:36 -07:00
epriestley
c04f141ab0 Remove obsolete "arc land" flags: --update-with-merge/rebase, --delete-remote
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
2017-06-09 08:12:37 -07:00
Alex Vandiver
129d51fa09 If the base commit for arc patch does not exist locally, try to fetch it
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
2017-05-18 13:13:22 -04:00
Hubert Kowalski
3c4735795a Detect trailing spaces and tabs
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
2017-05-03 03:29:14 -07:00
epriestley
27b51e6192 Fix two minor issues with "arc download"
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
2017-04-28 07:45:25 -07:00
Austin McKinley
5d0f5afca8 Add ArcanistRaggedClassTreeEdgeXHPASTLinterRule to Phutil linter map
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
2017-04-25 10:58:21 -07:00
Alex Vandiver
146693307f Make exception reporting from arc be in red
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
2017-04-20 15:20:15 -07:00
epriestley
a59cfca5f1 Upgrade "arc upload" to use SHA256
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
2017-04-04 16:56:35 -07:00
epriestley
c4e84550fc Allow "arc upload" to work correctly if it can not hash content
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
2017-04-04 16:24:27 -07:00
epriestley
82b7cd778a Make "arc download" use "file.search" if available
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
2017-04-04 16:16:11 -07:00
Jon Parise
d1db9a72b5 Improve the flake8 line-matching regex
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
2017-03-24 09:19:06 -07:00
Jakub Vrana
3b6b523c2b Fix errors found by PHPStan
Test Plan: None.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17376
2017-02-18 09:24:19 +00:00
Jakub Vrana
d0353d2381 Fix errors found by PHPStan
Test Plan:
Ran `phpstan analyze -a autoload.php arcanist/src` with `autoload.php` containing:

  <?php
  require_once 'libphutil/src/__phutil_library_init__.php';
  require_once 'arcanist/src/__phutil_library_init__.php';

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17367
2017-02-16 13:54:43 +00:00
epriestley
460b0e46ee Fix a property name collision in ArcanistHgServerChannel
Summary:
See D2665. Here, `channel` is the Mercurial wire protocol channel name ("o" = "output", "e" = "error", etc), not the underlying channel from `PhutilProxyChannel`.

Rename the property so there's no collision.

Test Plan:
```
$ ../../core/lib/arcanist/scripts/hgdaemon/hgdaemon_client.php . --trace --skip-hello
array(3) {
  [0]=>
  int(0)
  [1]=>
  string(103) "capabilities: getencoding runcommand
encoding: UTF-8
pid: 73263e20cf21273d50f1f66cab6e0f7c74f4864e67f0f"
  [2]=>
  string(0) ""
}

Executed in 30647 us.
```

Reviewers: vrana, chad

Reviewed By: vrana

Differential Revision: https://secure.phabricator.com/D17366
2017-02-16 05:44:10 -08:00
Aviv Eyal
13596cd10f Add back pht()s and tsprintf()s to arg set-config
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
2017-02-16 11:08:45 +00:00
Aviv Eyal
bc9b70508e arc set-config: warn about unknown keys
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
2017-02-15 13:38:57 +00:00
Alex Vandiver
f3037bf216 [git] Override diff.submodule so git diff output is always parseable
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
2017-02-13 17:40:26 -08:00
Josh Cox
224986af63 Provide a better error message when an invalid ID is given to arc patch
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
2017-02-08 04:53:41 -05:00
epriestley
ade25facfd Fix a bad interaction between "arc diff --reviewers" and "the first line of a message is always a title"
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
2017-01-05 14:35:39 -08:00
Chris Burroughs
c243cbbd9f tighten remote URI error handling with idiosyncratic remote names
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
2016-12-29 10:35:59 -05:00
epriestley
fad8584431 Make "aliases" show up in "arc get-config" help
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
2016-10-21 16:08:45 -07:00
Mukunda Modell
2962504855 Fix an DOMDocument error with NoseTestEngine running on PHP 7
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
2016-10-09 16:28:51 -05:00
epriestley
2ad15c499a Don't compute intraline diffs if the input fails a coarse check for being huge
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
2016-10-07 07:42:19 -07:00
Alex Vandiver
483e985d08 Check both UNIX- and Windows-style paths from linter output
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
2016-09-21 14:29:42 -07:00
Josh Cox
9e82ef979e Added a warning prompt if the user tries to use an API cert instead of a CLI cert
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
2016-08-25 11:34:34 -04:00