1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-08 16:02:39 +01:00
Commit graph

2086 commits

Author SHA1 Message Date
epriestley
eb6edb2739 Restore old expanded include path rules for workflows which fall through
Summary:
Ref T13490. Historically, "arc" was eager to load external libraries from many locations. I plan to make this significantly more structured/safer in the future (see T5055), but this rule got dropped as the "experimental" and "wilds" branches collapsed to "master".

Restore "next to arcanist" and "inside externals/includes" as valid search locations for classic workflows.

Test Plan: Ran "arc" commands in a working copy with a "load" library configured.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21004
2020-02-17 09:24:53 -08:00
epriestley
d3b77af8a5 Require "--" as an argument terminator when running noninteractively
Summary:
Depends on D21001. Ref T13490.

  - Require "--" to terminate arguments when running noninteractively.
  - Don't correct spelling when running noninteractively (we still suggest corrections).
  - Remove old "phage" wrapper script.
  - Fix an issue where "argv" could implicitly generate a parseable "--argv" flag.
  - Accept "--" as an argument terminator even when there is no argument list.
  - Update some strings to use more modern quoting style.
  - Make workflows decline to handle signals by default.
  - Allow "arc weld" to weld non-UTF8 files together.

Test Plan: Ran various commands. Welded non-UTF8 files.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21002
2020-02-16 09:16:51 -08:00
epriestley
db8419f19b Port "arc weld" and "arc anoid" to Toolsets workflows, plus minor fixes
Summary: Ref T13490. Update the "weld" workflow and the "anoid" workflow. Incorporates D20938.

Test Plan: Ran "arc weld". Ran "arc anoid".

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21001
2020-02-16 09:16:24 -08:00
epriestley
8cd79d38af Port "arc shell-complete" to Toolsets
Summary: Depends on D20996. Ref T13395. Ports the updated version of this workflow from "experimental".

Test Plan: Ran `arc shell-complete` to install the hook, then shell-completed commands.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20997
2020-02-14 09:16:46 -08:00
epriestley
70c6045c7f Update "arc alias" to modern workflows
Summary: Depends on D20993. Ref T13395. Merges the more-modern "alias" out of "experimental".

Test Plan: Defined and invoked aliases. This has some rough edges: notably, no easy list/delete flow.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20996
2020-02-14 09:16:18 -08:00
epriestley
0c6ae6bbcf Port "arc version" to Toolsets
Summary: Ref T13395. Depends on D20992. Run "arc version" as a modern workflow, not a classic workflow.

Test Plan: Ran "arc version".

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20993
2020-02-14 09:15:58 -08:00
epriestley
cf9469e0d1 Port "arc liberate" to Toolsets
Summary: Ref T13395. Depends on D20991. Make "arc liberate" run as a toolset command, not a classic command.

Test Plan: Ran "arc liberate"; created this diff.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20992
2020-02-14 09:14:56 -08:00
epriestley
0e95fcbb7f Port "arc help" to Toolsets
Summary: Ref T13395. Make "arc help" run as a toolset command, not a classic command.

Test Plan: Ran "arc help".

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20991
2020-02-14 09:14:30 -08:00
epriestley
c471983697 Collapse Arcanist toolsets from "wilds" into "master", as an overlay layer
Summary:
Depends on D20988. Ref T13395. Ref T13098. Ref T13203.

This brings all the "toolsets" code into "master". We try to run commands as toolsets commands first, then fall back to older code.

Since the "toolsets" class tree is mostly parallel to the older class tree, this isn't completely broken. Currently, all commands fall back.

Test Plan: Created this diff, ran various other commands. But this is probably a long shot from finished.

Maniphest Tasks: T13395, T13203, T13098

Differential Revision: https://secure.phabricator.com/D20990
2020-02-13 14:10:46 -08:00
epriestley
acf0607683 Merge utility/support changes from "wilds" to "master"
Summary:
Ref T13395. Merge a lot of stuff which doesn't break existing workflows:

    - Merge a UTF8 fix for "cmd.exe" on Windows.
    - Merge minor changes to JSON linters.
    - Merge some shell completion stuff.
    - Merge some "arc anoid" fixes.
    - Merge various Windows improvements to unit tests which interact with processes / the filesystem.
    - Merge some other Windows path fixes.
    - Merge a UTF8 character class fix.
    - Merge script initialization.
    - Merge unit test support scripts.
    - Merge some initialization code.
    - Merge Windows stdout/stderr-as-files code.
    - Merge a bunch of code for making exec tests work on Windows.
    - Merge more Windows unit test fixes.
    - Merge "continue on failure" mode when loading symbols.
    - Merge "GPC" order CLI fixes.

Test Plan: Ran `arc unit --everything`; created this change. There's likely some less-than-perfect code here.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20988
2020-02-13 14:10:09 -08:00
epriestley
0d62a10eda Don't depend on "XMLWriter" to load the lint renderer class tree
Summary: Fixes T13489. See that task for discussion.

Test Plan: Ran `arc lint` and `arc lint --output xml`. Faked a missing extension, ran `arc lint` (no problems) and `arc lint --output xml` (sensible error message).

Maniphest Tasks: T13489

Differential Revision: https://secure.phabricator.com/D20989
2020-02-13 13:38:52 -08:00
epriestley
8c4f6ce161 Merge the remainder of the "experimental" branch
Summary:
Depends on D20986. Ref T13395. This //mostly// collapses the entire "experimental" branch into "master".

I plan to change the "Ref/Hardpoint" pattern to become future oriented, but this is more steps forward than sideways.

Test Plan: Ran various `arc` workflows.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20987
2020-02-13 06:05:08 -08:00
epriestley
4c36860c43 Merge Arcanist lint changes from "experimental" branch
Summary: Ref T13395. Collapse changes to the linter workflow from "experimental" into "master".

Test Plan: Ran `arc lint`. Noted flag changes in changelog.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20986
2020-02-13 06:04:47 -08:00
epriestley
26f853b634 Merge "--draft" flag and related changes from "experimental" to "master"
Summary: Ref T13010. This brings the "--draft" flag (and a handful of related flags) to "master" from "experimental".

Test Plan: Ran "arc diff". This code has been in use on "experimental" for a long time.

Maniphest Tasks: T13010

Differential Revision: https://secure.phabricator.com/D20985
2020-02-12 16:54:18 -08:00
epriestley
a8a50b2fc0 Make "arcanist/" unit tests pass
Summary: Ref T13395. Fixes a file-locking test and removes a logging test that's more trouble than it's worth.

Test Plan: Ran "arc unit --everything" locally, got a clean bill of health.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20984
2020-02-12 16:20:30 -08:00
epriestley
61e059984a Merge "phage" from "experimental"
Summary: Ref T13395. Currently, "phage" is required for various cluster operations. Bring the working code out of "experimental". This isn't the final form; "wilds" has a fancier version.

Test Plan: Ran phage workflows against the cluster.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20982
2020-02-12 15:53:23 -08:00
epriestley
9b74cb4ee6 Fully merge "libphutil/" into "arcanist/"
Summary: Ref T13395. Moves all remaining code in "libphutil/" into "arcanist/".

Test Plan: Ran various arc workflows, although this probably has some remaining rough edges.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20980
2020-02-12 15:17:38 -08:00
epriestley
a36e60f0a3 Move HTML-only intraline diff code to Phabricator
Summary:
Ref T13395. This code relies on PhutilHTML, but that's moving to Phabricator.

It has no callers in libphutil or Arcanist.

Test Plan: Looked for callers.

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D20978
2020-02-12 13:13:23 -08:00
epriestley
21a1828ea0 Omit "--" in older fallback commands for Git remote URIs
Summary: Ref T13481. Some older versions of Git appear to not support "--" in these commands. Just drop it. This can lead to ambiguous results with certain obviously-silly remote names, but doesn't appear to lead to anything dangerous.

Test Plan: Will followup with user on ancient Git.

Maniphest Tasks: T13481

Differential Revision: https://secure.phabricator.com/D20952
2020-01-23 16:50:54 -08:00
epriestley
70c0fd3f22 In Git, fall back across versions more cleanly when trying to get the URI for a remote
Summary: Fixes T13481. When identifying the URI for a remote, fall back from "git remote get-url" to "git ls-remote --get-url" to "git config remote.<name>.url" based on command output and version tests.

Test Plan: Ran `arc land --hold`, rigged the subcommands to fail to try all three fallbacks, ran `arc land --hold --remote asdfasdf` to get an explicit failure.

Maniphest Tasks: T13481

Differential Revision: https://secure.phabricator.com/D20950
2020-01-23 15:19:28 -08:00
epriestley
cc850163f3 When "arc close-revision --finalize ..." skips closing a revision, print a message
Summary: Fixes T13458. Emit an explicit message when "arc close-revision --finalize" bails out because the revision is not "Accepted".

Test Plan: Ran `arc close-revision [--finalize] ...` on various revisions, saw more clear messaging.

Maniphest Tasks: T13458

Differential Revision: https://secure.phabricator.com/D20915
2019-11-18 20:31:51 -08:00
epriestley
cc1ff38843 When generating diffs in "arc diff", disable Git config option "diff.suppressBlankEmpty"
Summary:
Ref T13432. Git has a "diff.suppressBlankEmpty" config option which makes it emit nonstandard diffs with trimmed trailing whitespace on unchanged blank lines.

Currently, we don't parse these diffs correctly. Even if we do in the future, emitting a more standard diff is desirable.

Explicitly disable this option when executing "git diff" so we build more standard diffs.

Test Plan:
  - Configured this option.
  - Modified a file with a blank line in it without changing the blank line, got this goofy display diff:

{F6985234}

  - Applied patch, rediffed the same change, saw "-c diff.suppressBlankEmpty" in "--trace" output and got this sensible diff:

{F6985235}

Maniphest Tasks: T13432

Differential Revision: https://secure.phabricator.com/D20877
2019-10-29 10:14:40 -07:00
epriestley
73943d1bc9 Make "arc land --merge" an explicit error when targeting a Perforce remote
Summary: Ref T13434. Since "git p4 submit" gets more complicated when submitting merges, and empty merges (as with "--no-ff") seem to vanish, and it's not clear this is desirable or useful anyway, just make the "merge" strategy an explicit error with Perforce remotes.

Test Plan: Ran "arc land --merge ..." in a Git/Perforce repository, got an explicit error. Ran "arc land --squash ...", got existing working behavior.

Maniphest Tasks: T13434

Differential Revision: https://secure.phabricator.com/D20871
2019-10-28 11:59:42 -07:00
epriestley
7383c2f4e6 In "arc land", when "remote/onto" does not exist locally, try to fetch it before giving up
Summary:
Fixes T10650. It's valid to `arc land --remote origin --onto master` without first fetching that ref. If we can't find a local ref for a specified remote branch, try to fetch it before giving up.

(In the long run, this should be valid even if the remote branch does not exist at all and the user intends to create it -- see T12876 -- but this is a step toward that.)

Test Plan:
  - Ran `rm .git/refs/remotes/origin/master`, then landed into "master".
  - Before: "arc land" bailed out immediately.
  - After: "arc land" fetches the missing ref.

```
$ arc land
 TARGET  Landing onto "master", the default target under git.
 REMOTE  Using remote "origin", the default remote under Git.
 TARGET  No local ref exists for branch "master" in remote "origin", attempting fetch...
 FETCHED  Fetched branch "master" from remote "origin".
...
```

Maniphest Tasks: T10650

Differential Revision: https://secure.phabricator.com/D20870
2019-10-28 11:31:18 -07:00
epriestley
a76054f8d6 Update "arc help land" to reference Perforce support
Summary: Fixes T12668. Ref T13434. This is far from exhasutive, but suggest that Perforce probably works.

Test Plan: Read documentation.

Maniphest Tasks: T13434, T12668

Differential Revision: https://secure.phabricator.com/D20869
2019-10-28 11:25:32 -07:00
epriestley
ca66743905 Support Perforce/Git repositories in "arc land"
Summary: Ref T13434. Detect perforce remotes and use "git p4" commands in place of "git" commands when operating in Perforce mode.

Test Plan:
  - Landed "master" onto itself, saw master update.
  - Landed "feature1" onto clean "master", saw master update.
  - Landed "feature2" onto dirty "master", saw master stay dirty.
  - Landed with "--hold", got sensible submit instructions.

Maniphest Tasks: T13434

Differential Revision: https://secure.phabricator.com/D20868
2019-10-28 11:25:00 -07:00
epriestley
9c7bbb760a Move Git-specific "arc land" parsing of "--onto" and "--remote" into GitLandEngine
Summary: Ref T13434. Move some git-engine-specific handling of "arc land" arguments into the Git engine. This prepares to handle Perforce workflows.

Test Plan: Will "arc land" this change.

Maniphest Tasks: T13434

Differential Revision: https://secure.phabricator.com/D20867
2019-10-28 11:24:34 -07:00
epriestley
da6d4f85ee Add a lint check for deprecated argument order to "implode()"
Summary:
Ref T13428. Historically, "implode()" accepts arguments in either order. PHP 7.4+ warns about glue not being first.

Add a lint check when the second parameter is a static scalar, implying it is the glue parameter.

Test Plan: Ran unit tests. See next change.

Maniphest Tasks: T13428

Differential Revision: https://secure.phabricator.com/D20857
2019-10-17 09:09:08 -07:00
epriestley
3cdfe1fff8 When running "arc land" from a detached HEAD, don't try to delete the source ref
Summary:
Fixes T10321. Some reasonable but less-common workflows involve running `arc land` from a detached HEAD state.

When users do this, we currently try to delete the raw hash as though it were a branch during cleanup. Instead, detect if the thing we're thinking about deleting is a branch or not, and just leave it alone if it isn't.

Test Plan:
  - Ran `git checkout <some hash>`, then `arc land --revision <some revision>`.
  - Before, everything worked but cleanup tried to `git branch -D <some hash>`.
  - After, everything worked and cleanup skipped branch deletion.

Maniphest Tasks: T10321

Differential Revision: https://secure.phabricator.com/D20786
2019-09-05 05:31:00 -07:00
epriestley
d92fa96366 Fix two "msort()" vs "msortv()" issues in "arc land"
Summary: See <https://github.com/phacility/arcanist/pull/242>. Ref T13303. A little more fallout turned up in `arc`.

Test Plan: `grep msort(`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13303

Differential Revision: https://secure.phabricator.com/D20605
2019-06-20 16:07:13 -07:00
epriestley
1ef9409817 Update "arcanist/" for "topological" API changes
Summary: Ref T13325.

Test Plan: Grepped for `topograph`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13325

Differential Revision: https://secure.phabricator.com/D20598
2019-06-20 16:05:21 -07:00
Asher Baker
7329bc7c32 Fix arc land on odd/modern git-svn checkouts
Summary:
The current code assumes git-svn is always working from a remote called
`trunk`, but if the repository is initialized without the `-T` option it
will instead be called `git-svn`, and if `--prefix` is used (which is
set by default to `origin/` in Git 2+) the remote name will have the
specified prefix as well.

Instead, look at the `fetch` target refspec set in the git-svn config.

Fixes T13293.

Test Plan:
`arc land` without errors (or manually creating a `trunk` branch) from a
checkout made with Git 2.18.0 (verified this manually on a non-`-T`
checkout as well).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13293

Differential Revision: https://secure.phabricator.com/D19681
2019-05-23 10:58:42 +01:00
Joshua Spence
dd514e268b Modify the lint-test file format to allow for more powerful assertions
Summary:
Fixes T6854. The current format for `lint-test` files is somewhat inflexible and does not allow us to make assertions regarding the code or name of the linter messages (of class `ArcanistLintMessage`) that are raised. Specifically, the `${severity}:${line}:${char}` format is hardcoded in `ArcanistLinterTestCase`. In this diff, I extend the this format to achieve the following goals:

- Allow for the lint message code and name to be specified. Specifically, the full format is `${severity}:${line}:${char}:${code}:${name}`.
- Make all fields optional. `error:3:` will match any and all errors occuring on line 3.
- Provide more useful output when assertions fail. Specifically, output //all// lint messages that are missing and/or surplus. Previously, only the first lint message was output.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley, chad

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6854

Differential Revision: https://secure.phabricator.com/D11176
2019-05-21 13:48:30 +10:00
Wenzheng Jiang
82445bb605 Let lint rules support anonymous classes
Summary: Ref T4334. Depends on D19740. Improve some lint rules so they can handle anonymous classes.

Test Plan: Ran updated tests

Reviewers: joshuaspence, #blessed_reviewers, epriestley

Reviewed By: joshuaspence, #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T4334

Differential Revision: https://secure.phabricator.com/D19741
2019-05-15 11:11:10 +10:00
Joshua Spence
6a8e76db32 Allow buildFutures and resolveFutures to be overridden
Summary:
I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods.

I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19730
2019-05-15 09:10:07 +10:00
Joshua Spence
fceac878f1 Allow setCustomSeverityRules to be overridden in subclasses
Summary:
I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods.

I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20514
2019-05-15 09:06:13 +10:00
Joshua Spence
4f583dded3 Call $linter->setEngine in linter tests
Summary: We aren't calling `$linter->setEngine($engine)`, even though we do have an `$engine`. This causes unit tests for any linters which require an engine to fail.

Test Plan: Ran the unit tests for a [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php | third-party linter]].

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20515
2019-05-15 09:01:54 +10:00
epriestley
9ccbbee336 Fix a potential double-prompt in "arc land" when landing with ongoing builds
Summary: The recently-added, build-plan-behavior-aware check here does all of its own prompting, so we should skip the other prompting if it doesn't throw.

Test Plan: Will `arc land` something sketchy sooner or later.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20494
2019-05-14 09:27:53 -07:00
epriestley
b61e890a0c Remove unnecessary "," from Pylint version regex
Summary:
See PHI1238. This "," in the regex can only make the lint binding fail if there is no "," in the version output string. In modern versions of Pylint, there is (apparently) no comma in the version string. Remove it.

See also <https://discourse.phabricator-community.org/t/arcanistpylintlinter-version-regex-issue/2688>

Test Plan:
```
$ pip install pylint
Traceback (most recent call last):
  File "/Users/epriestley/Library/Python/2.7/bin/pip", line 6, in <module>
    from pip._internal import main
ImportError: No module named pip._internal
```

¯\_(ツ)_/¯

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Differential Revision: https://secure.phabricator.com/D20505
2019-05-14 07:30:21 -07:00
epriestley
9830c9316d Make minor correctness changes to some "arc patch" command execution
Summary:
Since I'm in here for PHI1083:

  - Add some "--" so we get correct behavior when you have a file named "master", a branch named "README.txt", etc.
  - Stop using "%C" unnecessarily.
  - Fix some untranslatable strings.

Test Plan: Ran `arc patch` a couple of times, ran the variations of `git` commands to catch anything weird with "--" handling.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20254
2019-03-07 13:00:04 -08:00
epriestley
73804f0039 Fix a case where "arc patch" could skip submodule changes
Summary:
See PHI1083. Previously, see PHI648 and D19475.

When you apply a submodule patch in Git, it leaves you with a working copy that has the "submodule pointer" dirtied but the actual submodule untouched:

```
$ git status
On branch ...
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   philter

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   philter (new commits)
```

So, if you're applying `D123` and `submodule/` was previously pointed at commit "A" but `D123` updates it to point at commit "B", you get this after `git apply ...`:

  - Git index says "submodule/ = B".
  - On disk, "submodule/ = A".

Now, if you `git add --all` or `git commit --all`, git picks up the "change" on disk as an intended modification of the submodule. This puts the submodule back to "A" and overwrites/undoes the "pointer" update that's trying to make it point to "B".

To avoid this, update submodules after applying the patch.

Also, every time we modify the working copy, just update submodules.

Test Plan:
  - Add a submodule in branch "B1", pointed at commit "A".
  - Branch to create branch "B2". Update the submodule to point at commit "B". Commit this and `arc diff` it.
  - Go back to "B1". Use `arc patch D...` to apply the revision you just created.
  - Before change:
    - "arc patch" applies the submodule change, so "pointer = B", "disk = A".
    - "arc patch" runs "git commit --all", which looks at disk and sets "pointer = A".
    - This isn't a change, so we fail with an empty commit.
  - After change:
    - "arc patch" applies the submodule change, so "pointer = B", "disk = A".
    - "arc patch" updates submodules, so "pointer = B", "disk = B".
    - "arc patch" runs "git commit --all", which now has a change, and commits "submodule = B".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20253
2019-03-07 12:52:27 -08:00
epriestley
f6b8480adc Implement "Warn When Landing" behavior for Build Plans in Arcanist
Summary:
Ref T13258. This makes "arc land" respect the new "Warn When Landing" behavior.

This will only work if you have very up-to-date APIs. Just fall back to the older code if the new API calls fail.

Test Plan: Ran `arc land` on a revision with builds in various states and with the different "Warn When Landing" behaviors. Saw appropriate warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13258

Differential Revision: https://secure.phabricator.com/D20236
2019-03-06 06:47:53 -08:00
epriestley
96fde137a1 Improve performance of "arc diff" updates for changes with large diff text
Summary:
See PHI1104. The older "differential.querydiffs" method includes the entire raw diff text for all the diffs associated with a revision in its response, but we: only care about the most recent diff; and don't care about the text at all.

For reasonably large changes with several updates, this can be significantly slow.

We can get this same information more efficiently from the modern "differential.diff.search", since D19386 (April 2018). The only trick is that we need a "revisionPHID", which we don't have on hand.

For now, just fetch the revision PHID. In the future, we can likely make adjustments so that we have the revision PHID already by the time we get here.

This may slow down the normal case very slightly (since we now do two calls instead of one), but it speeds up the bad cases dramatically.

Test Plan:
Ran `arc diff` to update a change in a local repository. `var_dump()`'d the old and new algorithm results, saw the same outcome.

Used `arc diff --trace` on an update to a change to verify that `differential.diff.search` is called but `differential.querydiffs` is not.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20221
2019-02-28 08:00:01 -08:00
epriestley
9581dd0f52 Add Arcanist support for highlighting indent change intraline diffs
Summary:
Ref T13161. See D20181. This allows the intraline highlighter to accept new ">" and "<" spans and apply a different style for them.

The input pattern is `list<segment>`. Each segment is `pair<wild kind, int byte_length>`, i.e. wrap the next `byte_length` bytes in a span of kind `kind`.

Before this change, the possible kinds of segements are `0` (no intraline diff, do not highlight) or `1` (intraline diff, highlight in bright color).

D20181 adds `<` (depth decreased) and `>` (depth increased). These are like `1`, but add a different class so the UI can handle them differently.

Test Plan: See D20181.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161

Differential Revision: https://secure.phabricator.com/D20182
2019-02-19 12:17:32 -08:00
epriestley
07a208d8fc In "arc diff", warn when some reviewers are away even if not everyone is away
Summary:
Ref T13249. See PHI810. We currently warn you when //all// reviewers are away, but not when only some reviewers are away.

This makes some amount of sense under the "anyone can accept anything" rules we sort of recommend, but a lot of installs realistically have tons of owner/package rules now.

Instead, if any reviewers are away, show the user exactly who is away and until when, then make sure they don't want to make any adjustments.

(We can do a better job of this after the toolsets change when we can use the new APIs, but this is an easy fix for now.)

Test Plan: Created a revision with multiple reviewers, either some or all of whom were away. Got appropriate output and prompt behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20172
2019-02-15 14:44:37 -08:00
epriestley
7e61e43f65 Add version check whitelists for constants to the version compatibility lint rule
Summary: Ref T13249. We currently allow `if (function_exists('X')) { X(); }` but not `if (defined('X')) { X; }`. Allow the latter.

Test Plan: See D20145, which linted clean with this patch in place.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20146
2019-02-12 05:19:28 -08:00
epriestley
25c2381959 Statically detect "continue" inside "switch"
Summary: See 30 prior patches. This is a fatal in PHP7, let's just hunt these down.

Test Plan: Ran unit tests. See next diff for results.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19931
2018-12-28 00:01:12 -08:00
epriestley
eb732555a7 Fix a "continue;" inside a switch in ArcanistPHPCompatibilityXHPASTLinterRule
Summary: See <https://discourse.phabricator-community.org/t/arc-diff-fails-on-incompatible-linter-rule-with-php7-3/2198>.

Test Plan: Looked at the code carefully.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19868
2018-12-12 09:22:12 -08:00
Aviv Eyal
3534d2baca Small ReMarkup fix
Summary: See https://discourse.phabricator-community.org/t/2086

Test Plan: paste new content in comment box, see it markedup

Reviewers: #blessed_reviewers, epriestley, amckinley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D19794
2018-11-07 21:56:19 -08:00
epriestley
83661809e5 Work around a Windows escaping issue and security conecern in "hg cat --output ..."
Summary:
See PHI904. Ref T13210. Ref T13209. Currently, we have an `hg cat` construction which attempts to pass a literal `%p` to Mercurial. This fails because you can't pass `%` through `%s` outside of `wilds`.

It also uses `%C` to pass a list of file paths. This is broadly unsafe and can cause command execution if you modify a file named, e.g., `; rm -rf xyz` or similar. I think it would be difficult to turn this into an attack but it's fairly bad. This dates from D5144 in 2013.

Test Plan: With this patch, created D19757 which has valid binary data (see F5962134).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13210, T13209

Differential Revision: https://secure.phabricator.com/D19758
2018-10-26 07:28:50 -07:00