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: Depends on D16364. See that revision for discussion.
Test Plan: Ran `arc get-config history.immutable` in `arcanist/`, saw setting.
Reviewers: chad, fooishbar
Reviewed By: fooishbar
Differential Revision: https://secure.phabricator.com/D16365
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
