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

2232 commits

Author SHA1 Message Date
epriestley
cc23551a7d Update Paste help to include missing "--"
Summary: See PHI2027. This example command is missing "--", but it's required.

Test Plan: Ran the new command, no longer got an error about "--".

Differential Revision: https://secure.phabricator.com/D21623
2021-03-16 09:29:50 -07:00
epriestley
7ad4afb919 Correct a mistaken Phurl link in the "missing symbol" exception
Summary: See PHI2022. This link is missing the `/u/` part and currently 404's.

Test Plan: Followed the new link, got documentation.

Differential Revision: https://secure.phabricator.com/D21611
2021-03-12 09:51:18 -08:00
epriestley
7570dd0da1 Improve "PhutilJSON" handling of PHP-object JSON values
Summary:
Ref T13635. PHP native JSON functions sometimes represent JSON objects as PHP "stdClass" objects.

Accept this representation and emit it correctly in "PhutilJSON".

Test Plan: Added a test, made it pass.

Maniphest Tasks: T13635

Differential Revision: https://secure.phabricator.com/D21604
2021-03-11 12:43:08 -08:00
epriestley
2d6452acb5 In Arcanist, when trying to write to a file configuration source, create missing directories
Summary: The ".git/arc" directory may need to be created when writing to working copy configuration.

Test Plan:
  - Tried to store an answer to a prompt in a new working copy.
  - Before: error that ".git/arc" does not exist.
  - After: prompt saved to working copy configuration.

Differential Revision: https://secure.phabricator.com/D21588
2021-03-03 13:40:56 -08:00
epriestley
953d742a1a In "arc land", if rebasing a range fails, attempt to "reduce" it
Summary:
Ref T13576. See that task for discussion.

When a user runs `arc land --pick A`, they may be selecting a range of commits ("X..Y") which have ancestors ("V..W") that should NOT land.

We must slice "X..Y" out of history before we can merge it, to avoid landing changes from "V..W".

When "X..Y" is simple and linear, we can rebase the range to pick our desired slice out of history.

When "X..Y" includes merge commits, we frequently can not, and I could not identify any simple alternative. The best alternative I came up with is this "reduce" operation:

  - squash "into" onto Y, producing S, to guarantee there are no natural conflicts;
  - squash S onto X^, producing T, to get rid of the merge commits;
  - rebase T onto "into", producing R, to slice "X..Y" out of history;
  - squash R onto "into", producing Q. (R and Q will be the same, but this simplies the code.)

This feels flimsy and fragile, but I can't immediately find a way to break it. See T13576 for more discussion.

Test Plan:
  - Applied conflicting changes to `example.txt` in `master` and `feature1`.
  - Ran `arc land`, got a merge conflict.
  - Resolved the conflict with `git merge master`.
  - Ran `arc land`.
    - Before: merge conflict.
    - After: `arc land` resolves the merge correctly.
  - Stacked `feature2` on `feature1`, and made various mutations to `feature1` and `feature2`, then ran `arc land --pick feature2`. Changes made in `feature1` should not land, and they mostly do not. See T13576.

Maniphest Tasks: T13576

Differential Revision: https://secure.phabricator.com/D21590
2021-03-03 13:40:56 -08:00
epriestley
d72fad6461 Add a character marker to the "IMPLICIT COMMITS" warning in "arc land"
Summary:
Ref T13576. The "implicit commits" prompt in "arc land" shows a list of implicit and non-implicit commits.

The implicit commits are marked with a background color, but this doesn't survive if you copy/paste the output into a support ticket.

Make my life easier by also marking commits so the marker survives copy/paste.

Test Plan: Ran "arc land" with implicit commits, saw a copy-pastable indicator.

Maniphest Tasks: T13576

Differential Revision: https://secure.phabricator.com/D21589
2021-03-03 13:40:56 -08:00
epriestley
4399ee6b7f Temporarily disable all logfile writability checks
These cause too much trouble in too many cases.
2021-03-01 15:55:42 -08:00
epriestley
6d60422dbb Add a simple primitive for managing PHP runtime error logs
Summary:
Ref T13624. If we want to send PHP errors to a log, using the "error_log" configuration option catches the broadest set of errors across versions of PHP.

Configuring this disables errors on `stderr`, since they're sent to the log instead. We'd like them to go to both places; provide a simple wrapper for this. Also do a bit of writability testing.

Test Plan: Wrote errors to a new log, see followup changes.

Maniphest Tasks: T13624

Differential Revision: https://secure.phabricator.com/D21578
2021-02-26 14:54:41 -08:00
epriestley
9d5802cb9f Provide some "preg_*" wrappers which raise exceptions on failure
Summary: Ref T13608. Ref T13100. Ref T13586. Properly checking "preg_match()" and similar calls for failure and raising useful exceptions is complicated and error-prone. Provide wrapper functions with an API that's more consistent with the rest of the codebase: matches are returned; and errors raise detailed exceptions.

Test Plan: See next change.

Maniphest Tasks: T13608, T13586, T13100

Differential Revision: https://secure.phabricator.com/D21561
2021-02-19 11:16:09 -08:00
Vihang Mehta
f501f85eb8 Update golint install instructions
Summary:
These are the instructions from https://github.com/golang/lint.

The fetch location changed to `golang.org/x/lint/golint` from `github.com/golang/lint/golint`.
`-u` tells go to update the package and its deps if they exist.
`-u` Shouldn't strictly be necessary, but figured we might as well follow the instructions from `golint`.

Test Plan:
Enable golint without having it installed.
Ensure that the install instructions now show the new location.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21552
2021-02-10 10:03:39 -08:00
epriestley
239ad5c55d In "array_mergev()", guarantee the "call_user_func_array()" parameter list is a natrual list
Summary:
Ref T13588. The behavior of "call_user_func_array()" has changed in PHP8, and the function now attempts to use array keys as argument names.

This always fails when calling "array_merge()" (which does not accept named parameters), and may cause misbehavior in the general case.

Guarantee the argument is a natural list (with keys "0", "1", "2", ...).

Test Plan:
  - Behavior unchanged under PHP7.
  - User reports fixed behavior under PHP8, see <https://discourse.phabricator-community.org/t/daemon-fails-on-php-8-0-2-in-utils-php-array-merge-call-w-fix/4568>.
  - See T13588.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21551
2021-02-08 10:19:52 -08:00
epriestley
32fe933f3a Add a lint check for catching "Exception" without catching "Throwable"
Summary:
Ref T13588. For consistency of behavior between versions on either side of PHP7, any "catch (Exception)" block should generally have a "catch (Throwable)" block in the same catch list.

Raise a lint warning when "Exception" is caught without also catching "Throwable", since this is almost certainly not desired.

Test Plan: Added tests.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21542
2021-02-03 14:49:13 -08:00
epriestley
c1afa91f9f Annotate the unusual use of "$callback()" in "xsprintf()"
Summary: Ref T13588. See D21500. This syntax is unusual and there are some hidden complexities involved; annotate them. See D21500 for more discussion.

Test Plan: Read text, reviewed D21500.

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21541
2021-02-03 14:21:08 -08:00
epriestley
c51a996fb0 Detect and correct "final private" methods in lint
Summary:
Ref T13588. Marking a method "final private" has never been meaningful, and is an error in PHP8.

Add static analysis to detect (and correct) this issue.

Test Plan: Added unit tests, will lint "phabricator/".

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21539
2021-02-03 14:14:50 -08:00
epriestley
e2b6439a73 Allow lint to correct the spelling of builtin symbols
Summary: Ref T13598. This builds on D21537 and adds support for correcting the capitalization of builtin systems.

Test Plan:
  - Linted a file that uses "ExCePtIoN", got a lint correction.

Maniphest Tasks: T13598

Differential Revision: https://secure.phabricator.com/D21538
2021-02-03 13:26:33 -08:00
epriestley
08dbbbba5a When lint identifies an unknown symbol, attempt to correct it if it is miscapitalized
Summary:
Ref T13598. If you spell a symbol like "Polygon" as "PoLyGoN", you currently get an "unknown symbol" lint message. However, provided "Polygon" is a valid symbol, we can unambiguously correct the spelling of the symbol.

Note that this patch can only correct the spelling of application symbols, not builtin symbols (since none of the library maps contain builtin symbols).

Test Plan: {F8374599}

Maniphest Tasks: T13598

Differential Revision: https://secure.phabricator.com/D21537
2021-02-03 13:26:33 -08:00
epriestley
b2e715fc5a Provide "gitsprintf(...)" and disambiguate Git ref selectors
Summary: Ref T13589. See that task for discussion.

Test Plan:
  - Created this diff, ran most commands in isolation.
  - This change is difficult to test extensively.

Maniphest Tasks: T13589

Differential Revision: https://secure.phabricator.com/D21509
2021-01-13 12:31:15 -08:00
Jessica Clarke
172381260e Fix pyflakes tests for recent pyflakes versions
Summary:
Since 2.1.0 (commit 75bc0c03c145), pyflakes has included the Python
version and platform in its version output, so ignore it if present.

Since 2.2.0 (commit 6ba3f8e0b59b), pyflakes has included the column
number in its messages, so update the parser to include it and drop the
column number from the (only) test in order to work with both old and
new versions. Whilst here, assign names to the capture groups to make
the code clearer.

Test Plan: Ran arc unit

Reviewers: epriestley, joshuaspence, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21504
2021-01-11 04:52:29 +00:00
Jessica Clarke
09cff8611b Fix ArcanistJSHintLinterTestCase::testLinter for recent JSHint
Summary:
Recent JSHint improves the warning and attributes it to the equals sign
rather than the end of the expression (changed in 897e0359ce19, first
released in 2.11.0-rc1).

Test Plan: Ran arc unit with JSHint 2.12.0

Reviewers: epriestley, joshuaspence, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21503
2021-01-11 04:51:20 +00:00
Jessica Clarke
f64eb04300 Fix PhutilOAuth1FutureTestCase::testOAuth1SigningWithJIRAExamples for PHP 8
Summary:
PHP 8 deprecates openssl_free_key as the key is automatically freed, so
silence the warning in PhutilOAuth1Future::signString.

Test Plan: Ran arc lint --everything

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21502
2021-01-11 04:50:37 +00:00
Jessica Clarke
9589fd1866 Fix PhutilUTF8TestCase::testUTF8Convert for PHP 8
Summary:
In PHP 8 passing an invalid encoding to mb_convert_encoding raises a
ValueError (which extends Error not Exception), so fix the test to also
catch Throwable (but leave the explicit Exception case for PHP 5, which
lacks Throwable).

Test Plan: Ran arc unit --everything

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21501
2021-01-11 04:49:54 +00:00
Jessica Clarke
687cb41ace Fix ArcanistFormattedStringXHPASTLinterRule on older PHP after D21500
Summary:
Calling 'Foo::bar' is only supported since PHP 7, whereas the array form
is supported since PHP 5.4, which is below our PHP 5.5 baseline.

Test Plan: No regressions under PHP 8 and snippet tested on 3v4l

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21505
2021-01-11 04:40:35 +00:00
Jessica Clarke
90ac9a2ff2 Fix ArcanistFormattedStringXHPASTLinterRule for PHP 8
Summary:
PHP 8's sprintf raises a ValueError when encountering unknown format
specifiers (previously it would eat the argument and print nothing), so
linting format strings like %Ls dies with an uncaught ValueError.

Fix this by using a custom callback during linting to turn all format
specifiers into %s and replace the dummy null argument with the original
format specifier, ensuring we always end up providing valid input to the
sprintf at the end. This has the nice property that the output of the
call to xsprintf is the original format string, though any
transformation into valid input would do.

Test Plan: Ran arc lint

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21500
2021-01-11 04:04:59 +00:00
Jessica Clarke
0adef03fdf Fix PhutilTypeSpec's regex handling for PHP 8
Summary:
In previous versions, passing the wrong type to preg_match would give a
warning that could be suppressed by @ and caught by set_error_handler,
but as of PHP 8 this raises a TypeError and so remains uncaught. Thus
check up-front whether the provided value is a string.

This fixes linting arc itself when run with PHP 8, as includes and
excludes use "optional regex | list<regex>", so would previously try to
pass an array to preg_match for the first alternative and die.

Test Plan: Ran arc lint

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21499
2021-01-11 04:04:23 +00:00
Jessica Clarke
446dcf1ccd Fix error handler on PHP 8
Summary:
PHP 7.2.0 deprecated the 5th parameter and PHP 8 removed it, so stop
using it and provide a default value to avoid erroring with:

```
Too few arguments to function PhutilErrorHandler::handleError(), 4 passed and exactly 5 expected
```

Test Plan: Used to create this revision with PHP 8 on macOS

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21498
2021-01-11 02:02:16 +00:00
Jessica Clarke
3ab2b407db Remove final from private functions for PHP 8 compatibility
Summary:
This combination does not make sense and PHP 8 errors with:

```
Private methods cannot be final as they are never overridden by other classes
```

Thus remove the redundant final from all such functions.

Test Plan: Used to create this revision with PHP 8 on macOS

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21496
2021-01-10 22:05:20 +00:00
epriestley
4b3baca999 Fix a typo of "previously" in FutureIterator
Summary: Ref T13572. D21466 had a typo in a comment.

Test Plan: Read carefully.

Maniphest Tasks: T13572

Differential Revision: https://secure.phabricator.com/D21478
2020-10-16 14:23:18 -07:00
epriestley
ccf74a40dd Fix an issue where "phutil_utf8v()" could fatal when passed an integer
Summary:
See <https://discourse.phabricator-community.org/t/search-by-name-in-files-doesnt-support-number/4300>.

I can't exactly reproduce the original issue, but when a query like "quack 1234" is tokenized, we end up calling "phutil_utf8v(1234)", where the argument is an integer.

At least in recent versions of PHP, this fatals ("trying to access an offset of an integer"). Cast the argument first.

Test Plan: Searched for "quack 1234" in Files. Before: fatal accessing offset of integer; after: correct results.

Differential Revision: https://secure.phabricator.com/D21477
2020-10-16 09:22:22 -07:00
bootstraponline
04e340ab0f Fix rubocop lint tests
Summary: Fix tests to work with rubocop 0.92.0 released on September 25, 2020

Test Plan: Unit tests pass

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D21474
2020-09-30 15:19:04 +00:00
Paul Tarjan
7597f31b6a fail arc diff if second lfs push errors
Summary:
We are having issues where people run out of file descriptors and the first `git push` will succeed, but the
second one will not. We'd like the diff to not be created in this case as it leads to weird behavior like our tests
running against 0 changed files.

Test Plan: none

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21471
2020-09-28 16:20:56 -07:00
epriestley
a716c4e55f In "phutil_passthru()", "resolve()" the future rather than calling "execute()" directly
Summary:
See PHI1862. This code calls "execute()" on the future directly, but that skips some steps -- notably, ServiceProfiler hooks.

Call "resolve()", which has the same effect but includes desirable/expected side effects.

Test Plan: Changed a workflow to run "phutil_passthru('ls')", ran it with "--trace". Before: no execution in trace; after: execution in trace.

Differential Revision: https://secure.phabricator.com/D21470
2020-09-18 11:22:52 -07:00
epriestley
563dc2a993 In ConduitCallFuture, only call Conduit exception messages on Conduit exceptions
Summary: Ref T13582. When this code is reached with a raw HTTP exception, it currently fatals.

Test Plan:
  - Ran `arc branches --conduit-uri=http://example.org` (a bad Conduit URI).
  - Before: hard fatal with a bad method call.
  - After: non-Conduit exception raised to user. Not ideal, but a step forward.

Maniphest Tasks: T13582

Differential Revision: https://secure.phabricator.com/D21467
2020-09-17 13:20:24 -07:00
epriestley
8e5e49984d Fix a slow memory leak in long-lived FutureIterator objects, as used by FuturePool
Summary:
See T13572. FutureIterator does not release futures, so long-lived iterators (like the one that FuturePool may build) can end up leaking memory.

This affects the FuturePool used by the daemon overseer.

See T13572 for more discussion.

Test Plan:
  - Ran the simple FutureIterator script from T13572. Before: memory held during iteration, script grows without bound. After: memory released, script uses stable memory.
  - Ran the overseer with memory logging and an immediate wakeup from hibernation. Before: saw memory usage grow without bound at a rate of ~300MB/day. After: saw memory usage stable.

Differential Revision: https://secure.phabricator.com/D21466
2020-09-17 12:56:48 -07:00
epriestley
de209ec064 When raising a Conduit client exception, show the called method in the error message
Summary: Ref T13581. This message can be slightly more helpful in some cases by showing which method call failed.

Test Plan: Ran `arc branches` under the error condition in D21462, got a more useful error.

Maniphest Tasks: T13581

Differential Revision: https://secure.phabricator.com/D21463
2020-09-15 17:34:22 -07:00
epriestley
7112ee3d59 Fix additional "xsprintf()"-family static parameter errors
Summary:
Ref T13577. After the lint rule fix in D21453, it can identify more errors. Fix the errors it identifies in "arcanist/".

These all seem fairly obscure/benign.

Test Plan: Ran `arc lint` on the files before and after these changes. Did not specifically re-test these particular messages, but they mostly very obscure.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21456
2020-09-08 11:45:54 -07:00
epriestley
83e63aeb07 Allow AAST to extract string literal values from HEREDOCs
Summary:
Ref T13577. I'd like to `arc lint --everything` to find other bad calls to `pht()` and similar functions, but `n_HEREDOC` nodes currently can not generate a response to "getStringLiteralValue()".

Support literal extraction from heredocs.

Test Plan: Added a test, made it pass. Will lint everything.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21455
2020-09-08 11:45:54 -07:00
epriestley
1c327208d7 Fix a missing "pht()" parameter in HTTPSFuture
Summary: Ref T13577. This call is missing a parameter. After D21453, this is detected properly by lint. Provide the parameter.

Test Plan: Ran `arc lint` on HTTPSFuture before and after the change.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21454
2020-09-08 11:45:54 -07:00
epriestley
73847a4b19 Fix a false negative in lint for "xsprintf()"-family functions
Summary:
Ref T13577. This lint rule correctly detects the error in `pht('x %s y')` but the narrow test for `n_STRING_SCALAR` prevents it from detecting the error in `pht('x %s y'.'z')`.

Make the test broader.

Test Plan:
  - Ran `arc lint` on `HTTPSFuture.php`, got a detection of the issue in T13577.
  - Added a failing test and made it pass.

Maniphest Tasks: T13577

Differential Revision: https://secure.phabricator.com/D21453
2020-09-08 11:45:53 -07:00
epriestley
ceb082ef6b Give Futures clearer start/end and exception semantics
Summary:
Ref T13555. Currently:

  - If an exception is raised in "start()", the exception state is not set on the future.
  - Futures do not always call "startFuture()" before starting, and do not always call "endFuture()" once they become resolvable.
  - If you start an ExecFuture which immediately fails and then call "getPID()" on it, you get an unclear exception.

Simplify these behaviors:

  - In FutureIterator, only start futures which have not already started.
  - When starting a future on any pathway, run start code.
  - When a future becomes resolvable on any pathway, run end code.
  - Raise a more clear exception when calling "getPID()" on a future with no subprocess.

Test Plan: Faked a failing subprocess with "$proc = null", ran "bin/phd debug taskmaster" etc. Got clearer errors and more consistent future lifecycle workflows.

Maniphest Tasks: T13555

Differential Revision: https://secure.phabricator.com/D21423
2020-07-23 11:22:20 -07:00
epriestley
65cda1596f Preserve bookmarks across "hg rebase --keep --collapse", and destroy them before "hg strip/prune"
Summary:
See PHI1808. Currently, "arc land <some bookmark>" does not destroy the bookmark in Mercurial. There are three issues here:

  - "hg rebase --keep --collapse" moves bookmarks to the rewritten commits;
  - "hg strip" moves bookmarks backwards;
  - "hg prune" moves bookmarks backwards.

To get around "hg rebase", save and restore bookmark state.

To get around "hg strip" and "hg prune", explicitly destroy bookmarks pointing at commits before we strip/prune those commits.

Test Plan:
  - Ran "arc land <some bookmark> --trace". Saw arc reset the bookmark position after rebasing, and destroy the bookmark explicitly before stripping.
  - When the workflow exited, saw no more bookmark (previously: bookmark existed and pointed at a possibly-intermediate state).

Differential Revision: https://secure.phabricator.com/D21397
2020-07-08 17:43:15 -07:00
epriestley
354da1ddaa When saving and restoring local state in Mercurial, also save and restore bookmarks
Summary:
Ref PHI1808. In Mercurial, we must save and restore bookmark state explicitly.

  - Save and restore bookmarks.
  - Clean up concepts in "arc-ls-markers" slightly, so we don't need separate "isCurrent" and "isActive" flags, hopefully.

I believe the totality of Mercurial state is:

  - A (non-bare) working copy points at exactly one commit (which might be the empty/null commit, in an empty repository).
  - A working copy has exactly one active branch.
    - Each branch has zero or more heads.
    - Each head may be closed.
    - Each (non-null) commit belongs to exactly one branch.
    - Note that the active branch may have zero heads and zero commits which belong to it!
  - A working copy has zero or one active bookmark.

To capture this, we now emit:

  - A list of branch heads. If a branch head is a working copy commit, that head is flagged as active.
  - A list of bookmarks. If a bookmark is the current bookmark, that bookmark is flagged as active.
  - A single "branch-state" virtual marker. This covers the case where you have run "hg branch X" to create X, but no objects in the working copy actually correspond to X yet. It also covers the case where you are on a concrete branch, but not any head of that branch.
  - A single "commit-state" virtual marker. This always shows the current commit in the working copy.

Test Plan:
  - Useful states to test are:
    - Empty repository (not all commands currently work here).
    - Normal repository, on a bookmark.
    - Normal repository, no bookmark.
    - "hg up 123" to update to somewhere in history.
    - "hg branch X", to start a new branch with no commits.
  - Ran "arc branches" and "arc bookmarks" in various states. Saw generally sensible output.
  - Ran "arc land --hold ..." in various states against a failing remote. Saw generally sensible output, and saw working properly restored to the original state.

Differential Revision: https://secure.phabricator.com/D21396
2020-07-08 15:30:17 -07:00
epriestley
3633364bb9 Clean up push failure messaging in "arc land" slightly
Summary: Ref PHI1808. Currently, push failures are messaged awkwardly. Make this exception handling more selective and the user-facing behavior more readable.

Test Plan: Ran "arc land" against a failing remote, saw a human-readable message instead of a stack trace.

Differential Revision: https://secure.phabricator.com/D21395
2020-07-08 15:30:17 -07:00
epriestley
710bceab10 When "arc land" fails a Mercurial push, actually raise it as an exception
Summary: See PHI1808. Some refactoring of the "passthru" API resulted in error conditinos here being dropped. Instead, raise them as exceptions.

Test Plan: Forced "hg push" to fail, used "arc land" against a failed push, saw error behavior instead of "success" feedback.

Differential Revision: https://secure.phabricator.com/D21394
2020-07-08 15:30:17 -07:00
epriestley
41774ba9cc Fix additional Mercurial/Python compatibility issues in "arc land"
Summary:
Ref PHI1805. Under some combination of versions (Python 3.8?), "arc-ls-markers" is running into additional Python runtime issues.

Sprinkle more "b" around to resolve them? Also clean up a couple of plain "arc" issues.

Test Plan:
Landed a change in Mercurial.

Some of this works fine without changes in Python 3.7/2.7 against Mercurial 4.7/5.4, so this may not be exhaustive.

Differential Revision: https://secure.phabricator.com/D21393
2020-07-07 10:20:41 -07:00
epriestley
79a6dfd7a9 Fix a MarkerRef call to get the active bookmark in Mercurial
Summary: See PHI1805. This call is constructed improperly and can lead to a fatal in `arc patch` under Mercurial.

Test Plan: In Mercurial, ran a valid `arc patch` operation.

Differential Revision: https://secure.phabricator.com/D21391
2020-07-06 14:08:11 -07:00
epriestley
a5480609f8 Render the state tree in "arc branches" slightly more cleanly
Summary:
Ref T13546. Try using unicode box drawing characters to render a more obvious tree struture in "arc branches".

Unclear if this has enough support to use, but seems okay so far.

Test Plan: Ran "arc branches", saw a nicer tree display.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21390
2020-07-03 12:43:34 -07:00
epriestley
01e91dc260 Clean up some service profiler behavior in Conduit futures
Summary:
Correct two minor Conduit future issues:

  - FutureAgent shows up in the trace log as "<mystery>". Since it isn't doing anything useful, solve the mystery and drop it from the log.
  - Simply the ConduitFuture code for interacting with the service profiler now that a more structured integration is available.

Test Plan: Ran "arc branches --trace", saw conduit calls and no more "<mystery>" clutter.

Differential Revision: https://secure.phabricator.com/D21388
2020-07-01 06:37:31 -07:00
epriestley
b8a5191e3b Improve login/auth messages from Arcanist toolset workflows
Summary:
See PHI1802. After D21384, "arc land" and similar with no credentials now properly raise a useful exception, but it isn't formatted readably.

Update the display code to make it look prettier.

Test Plan: Ran "arc land" with no and invalid credentials, got properly formatted output.

Differential Revision: https://secure.phabricator.com/D21387
2020-07-01 06:37:31 -07:00
epriestley
65e4927dca Drop intended support for "--anonymous" from Arcanist Toolsets
Summary:
Currently, modern "arc" workflows accept and parse "--anonymous" but don't do anything with it.

I intend to move away from anonymous workflows, which only really supported a tiny subset of unusual workflows on open source installs but added significant complexity to login/auth behavior.

Drop support for this flag.

Test Plan: Grepped for "anonymous", didn't turn up any relevant hits.

Differential Revision: https://secure.phabricator.com/D21386
2020-07-01 06:37:31 -07:00
epriestley
17f2668d1f When tab-completing "arc" commands, suggest paths if the argument is empty and a path wildcard argument exists
Summary:
Currently, if you type "arc upload <tab>", we do not autocomplete the working directory. We should, but the "current argument" is the empty string and that's technically a prefix of every flag, so we suggest that you might want a flag instead.

You probably don't. Suggest paths in this case.

Test Plan:
  - Ran "arc upload <tab>", saw path completions.
  - Ran "arc land <tab>" (this workflow does NOT take paths as arguments), saw flag completions.

Differential Revision: https://secure.phabricator.com/D21385
2020-07-01 06:37:31 -07:00
epriestley
7e9f80971b Implement Conduit login prompt behavior as a pure FutureProxy, not a Future-like object
Summary:
See PHI1802. Currently, we can't raise a "you must login" error in a generic way at the beginning of a workflow because we don't know if a workflow needs credentials or not.

For example, "arc help" does not need credentials but "arc diff" does.

Additionally, some actual Conduit calls do not need credentials ("conduit.ping", "conduit.getcapabilities") and others do.

Although I'd like to simplify this eventually and move away from anonymous/unauthenticated "arc", this isn't trivial today. It's also possible for third-party code to add authenticated calls to "arc help", etc., so even if we could execute these tests upfront it's not obvious we'd want to.

So, for now, we raise "you must login" at runtime, when we receive an authentication error from Conduit.

This got implemented for Toolsets in a well-intentioned but not-so-great way somewhere in wilds/experimental, with an "ArcanistConduitCall" that behaves a bit like a future but is not really a future. This implementation made more sense when ConduitEngine was serving as a future engine, and FutureProxy could not rewrite exceptions.

After the Toolsets code was first written, ConduitEngine has stopped serving as a future engine (this is now in "HardpointEngine"). Since HardpointEngine needs a real future, this "show the user a login message" code gets bypassed. This results in user-visible raw authentication exceptions on some workflows:

```
[2020-06-30 21:39:53] EXCEPTION: (ConduitClientException) ERR-INVALID-SESSION: Session key is not present. at [<arcanist>/src/conduit/ConduitFuture.php:76]
```

To fix this:

  - Allow FutureProxy to rewrite exceptions (see D21383).
  - Implement "ArcanistConduitCall" as a FutureProxy, not a future-like object.
  - Collapse the mixed-mode future/not-quite-a-future APIs into a single "real future" API.

Test Plan:
- Created a paste with "echo hi | arc paste --".
- Uploaded a file with "arc upload".
- Called a raw method with "echo {} | arc call-conduit conduit.ping --".
- Invoked hardpoint behavior with "arc branches".
- Grepped for calls to either "resolveCall()" method, found none.
- Grepped for calls to "newCall()", found none.
- Grepped for "ArcanistConduitCall", found no references.

Then:

- Removed my "~/.arcrc", ran "arc land", got a sensible and human-readable (but currently ugly) exception instead of a raw authentication stack trace.

Differential Revision: https://secure.phabricator.com/D21384
2020-07-01 06:37:31 -07:00
epriestley
2daf9b16ae Improve resolution behaviors of FutureProxy
Summary:
See PHI1764. See PHI1802. Address two resolution behaviors for FutureProxy:

  - FutureProxy may throw an exception directly from iteration via "FutureIterator" (see PHI1764). This is wrong: futures should throw only when resolved.
  - FutureProxy can not change an exception into a result, or a result into an exception, or an exception into a different exception. Being able to proxy the full range of result and exception behavior is useful, particularly for Conduit (see PHI1802).

Make "FutureProxy" more robust in how it handles exceptions from proxied futures.

Test Plan:
Used this script to raise an exception during result processing:

```
<?php

require_once 'support/init/init-script.php';

final class ThrowingFutureProxy
  extends FutureProxy {

  protected function didReceiveResult($result) {
    throw new Exception('!');
  }

}

$future = new ImmediateFuture('quack');
$proxy = new ThrowingFutureProxy($future);
$iterator = new FutureIterator(array($proxy));

foreach ($iterator as $resolved) {
  try {
    $resolved->resolve();
  } catch (Exception $ex) {
    echo "Caught exception properly on resolution.\n";
  }
}
```

Before this change, the exception is raised in the `foreach()` loop. After this change, the exception is raised at resolution time.

Differential Revision: https://secure.phabricator.com/D21383
2020-07-01 06:37:30 -07:00
epriestley
98ca5cfa81 Remove an unused method in "ArcanistUploadWorkflow"
Summary: This method is private and has no callers. The code has moved to "FileUploader" in a prior change.

Test Plan: Grepped for callers, found none.

Differential Revision: https://secure.phabricator.com/D21382
2020-07-01 06:37:30 -07:00
epriestley
4b8a32ee02 Give Mercurial more plausible marker behavior
Summary: Ref T13546. Fixes some issues where marker selection in Mercurial didn't work, and selects "draft()" as the set of commits to show, which is at least somewhat reasonable.

Test Plan: Ran "arc branches" and "arc bookmarks" in Mercurial, got more reasonable output.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21380
2020-06-30 15:50:07 -07:00
epriestley
8c95dc0d29 Support date-range commit graph queries, and multiple disjoint commits in Git
Summary: Ref T13546. Allow the commit graph to be queried by date range, and Git to be queried for multiple disjoint commits.

Test Plan: Ran "arc branches" and future code which searches for alternate commit ranges for revisions.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21379
2020-06-30 15:50:06 -07:00
epriestley
c7093a2e57 In "arc branches", group linear sequences of published revisions together
Summary: Ref T13546. If your history includes a long linear sequence of published revisions, summarize them.

Test Plan: Ran "arc branches", saw better summarization of linear published revision sequences.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21367
2020-06-30 15:50:06 -07:00
epriestley
5d305909eb When a commit graph set has many commits, summarize them
Summary: Ref T13546. In cases where a given set has a large number of commits, summarize them in the output.

Test Plan: Ran "arc branches", saw long lists of commits (like the history of "stable" summarized).

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21366
2020-06-30 15:50:06 -07:00
epriestley
0ad3222d59 Improve grid layout in "arc branches" at various terminal widths
Summary: Ref T13546. Make "arc branches" use a flexible grid width and try to match the content to the display width in a reasonable way.

Test Plan: Ran "arc branches" at various terminal widths, got generally sensible output.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21365
2020-06-30 15:50:06 -07:00
epriestley
10c4a551ae Remove implicit sorting from "MarkerRefQuery"
Summary: Ref T13546. This is no longer necessary after the introduction of "msortv_natural()", which can handle natural string sorting.

Test Plan: Ran "arc branches", saw the same sorting applied.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21364
2020-06-30 15:50:05 -07:00
epriestley
cd19216ea2 Render "arc markers" workflows as a tree, not a list
Summary:
Ref T13546. Currently, each "land" workflow executes custom graph queries to find commits: move toward abstracting this logic.

The "land" workflow also has a potentially dangerous behavior: if you have "master > A > B > C" and "arc land C", it will land A, B, and C. However, an updated version of A or B may exist elsewhere in the working copy. If it does, "arc land" will incorrectly land an out-of-date set of changes.

To find newer versions of "A" and "B", we need to search backwards from all local markers to the nearest outgoing marker, then compare the sets of changes we find to the sets of changes selected by "arc land".

This is also roughly the workflow that "arc branches", etc., need to show local markers as a tree, and starting in "arc branches" allows the process to be visualized.

As implemented here ,this rendering is still somewhat rough, and the selection of "outgoing markers" isn't good. In Mercurial, we may plausibly be able to use phase markers, but in Git we likely can't guess the right behavior automatically and probably need additional configuration.

Test Plan: Ran "arc branches" and "arc bookmarks" in Git and Mercurial.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21363
2020-06-30 15:50:05 -07:00
epriestley
80f5166b70 Identify published commits in working copies by using remote configuration
Summary:
Ref T13546. When running "arc branches", we want to show all unpublished commits. This is often a different set of commits than "commits not present in any remote".

Attempt to identify published commits by using the permanent ref rules in Phabricator.

Test Plan: Ran "arc look published", saw sensible published commits in Git.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21378
2020-06-30 14:56:34 -07:00
epriestley
50f7a853b5 Load and map repository objects for remote URIs
Summary:
Ref T13546. Query and associate known Phabricator repositories to working copy remotes by normalizing and comparing URIs.

This primarily gives us access to "permanentRefRules" so we can tell which branches have published changes.

Test Plan: Ran "arc look remotes" in Git and Mercurial working copies, saw repositories map properly.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21377
2020-06-30 13:43:14 -07:00
epriestley
6bf7a40358 Provide "arc look", a user-facing inspection command
Summary:
Ref T13546. Currently, "arc which" provides some amount of inspection but it generally isn't very helpful to users and is too limited and inflexible. "arc inspect" is an internal/debugging workflow.

The new "arc look" is much more aggressively unhelpful.

Test Plan: I'm not sure if this command should allow you to continue at night, because it's too dark.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21376
2020-06-30 13:08:31 -07:00
epriestley
ffb027e85c Support generating remote refs in Git
Summary: Ref T13546. Allow construction of remote refs in Git; previously they were only supported in Mercurial.

Test Plan: Ran "arc inspect remote(origin)" in Git, got a ref.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21375
2020-06-30 13:08:19 -07:00
epriestley
89f9eb66a7 Support inspection of remote refs with "arc inspect remote(...)"
Summary: Ref T13546. Expose remote refs for inspection via "arc inspect". For now, this only works in Mercurial.

Test Plan: Ran "arc inspect remote(default)" in Mercurial, got a ref out.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21374
2020-06-30 13:07:25 -07:00
epriestley
b19985a4bd Copy repository URI normalization code from Phabricator to Arcanist
Summary: Ref T13546. Move toward smarter remote repository lookup by providing URI normalization code in Arcanist. This diff duplicates code from Phabricator; the next change will collapse it.

Test Plan: Ran unit tests.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21372
2020-06-30 13:07:12 -07:00
epriestley
c53c05e5b2 Introduce "phutil_partition()" and natural case sorting for "msortv(...)"
Summary:
Ref T13546. Pull some small utility changes out of the deeper stack of "land/markers" changes.

"phutil_partition()" makes it easier to write code that loops over a list grouping elements, then acts on each group. This kind of code is not terribly common, but often feels awkward when implemented with raw primitives.

"msortv()" can support "natural" sorting, which sorts "feature1", "feature2", ..., "feature10" in a more human-readable order.

Test Plan: Ran unit tests, used new behaviors elsewhere in "arc markers" workflows.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21371
2020-06-30 06:45:03 -07:00
epriestley
33484b43c9 Introduce "GridView", an updated version of "ConsoleTableView"
Summary:
Ref T13546. In a future change, I'm providing a fancier version of "arc branches" that requires more sophisticated table rendering.

Implement a new view which can do some fancier things, like handle alignment of multi-line table cells.

Test Plan: See future changes.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21360
2020-06-30 06:31:24 -07:00
epriestley
98bf58db4a Correct a leftover reference to "--keep-branch"
Summary: See <https://discourse.phabricator-community.org/t/arc-land-keep-branch-no-longer-works/4004/>. This flag is now "--keep-branches".

Test Plan: Grepped for "keep-branch".

Differential Revision: https://secure.phabricator.com/D21356
2020-06-30 06:30:51 -07:00
epriestley
f52222ad19 Add more "RepositoryRef" legacy status mappings
Summary: Ref T13546. The old "differential.query" call is still used to fill refs when all we have locally is hashes. Add some mappings to improve the resulting refs.

Test Plan: Viewed "arc branches", saw statuses colored more consistently.

Reviewers: ptarjan

Reviewed By: ptarjan

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21355
2020-06-30 06:30:21 -07:00
epriestley
b0a9ef8351 In "arc land" under Git, confirm branch creation
Summary: Ref T13546. If "arc land" would create a branch, warn the user before it does.

Test Plan: Ran "arc land --onto mtarse", a typo of "master".

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21354
2020-06-30 06:29:51 -07:00
epriestley
33bb0acf97 Collect scattered implementations of "getDisplayHash()" into RepositoryAPI
Summary: Ref T13546. All of LandEngine, LocalState, and RepositoryAPI implement "getDisplayHash()". Always use the RepositoryAPI implementation.

Test Plan: Grepped for symbols.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21353
2020-06-30 06:28:38 -07:00
epriestley
63f2e667b9 Update "arc land" display of build failures, and rename "DisplayRef" to "RefView"
Summary:
Ref T13546. Show ongoing and failed builds more clearly in "arc land" output.

Also rename "DisplayRef" (which is not a "Ref") to "RefView" with the goal of improving clarity, and let callers "build...()" it so they can add more status, etc., information.

Get rid of "[DisplayRef|RefView]Interface". In theory, future refs (say, in Phabricator) might not do anything here, but every Ref just ends up implementing it. This could perhaps be subclassed more narrowly in the future if necessary.

Test Plan: Ran "arc land", grepped for various symbols.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21352
2020-06-30 06:27:56 -07:00
epriestley
33dfa859d8 On Windows, don't try to set "stdin" nonblocking, as it does not work
Summary:
See <https://discourse.phabricator-community.org/t/arc-land-fail-unable-to-set-stdin-nonblocking/4006/>.

See also <https://bugs.php.net/bug.php?id=34972>.

Note that you can't ^C during a prompt (or at any other time) in Windows currently, see T13549.

Test Plan: On Windows, hit a prompt in "arc land", then answered it successfully.

Differential Revision: https://secure.phabricator.com/D21358
2020-06-12 14:29:52 -07:00
epriestley
1e09a0ee7e When a linter raises a message at a nonexistent line, don't fatal during rendering
Summary:
See PHI1782. If a linter raises a message at a line which does not exist in the file, render a confused warning rather than fataling.

This is a long-existing issue which was exacerbated by D21044.

Test Plan: Modified a linter to raise issues at line 99999. Before change: fatal in console rendering. After change: reasonable rendering.

Differential Revision: https://secure.phabricator.com/D21357
2020-06-12 12:31:02 -07:00
epriestley
92f860ae9b Improve "--hold", save/restore state, bookmark creation, and some warnings for "arc land" in Mercurial
Summary:
Ref T13546. Ref T9948.

  - Make "--hold" show the same set of commands to manually push that the normal workflow would use.
  - Make save/restore state work.
  - Make bookmark creation prompt for confirmation.
  - Improve / provide some additional warnings and help text.

Test Plan: Ran various increasingly complex "arc land" workflows, e.g. "arc land --hold --onto fauxmark1 --onto fauxmark2 --into default . --revision 118 --trace"

Maniphest Tasks: T13546, T9948

Differential Revision: https://secure.phabricator.com/D21351
2020-06-10 17:31:51 -07:00
epriestley
50c534b591 Correct some minor "arc land" workflow issues in Mercurial
Summary: Ref T9948. Ref T13546. Clean up some minor behaviors to allow "arc land" to function in the simplest cases again. Also, do a capability test for "prune" rather than just falling back.

Test Plan: Ran "arc land <mark>" in Mercurial, got changes pushed.

Maniphest Tasks: T13546, T9948

Differential Revision: https://secure.phabricator.com/D21350
2020-06-10 17:31:50 -07:00
epriestley
488a24c40a In "arc land" in Mercurial, inch closer to making complex branch/bookmark workflows function
Summary:
Ref T9948. Ref T13546. This change moves toward a functional "arc land" in Mercurial.

Because of how "bundlerepo.getremotechanges()" works, "hg arc-ls-markers" does not actually list markers in the remote that aren't different from local markers so it's hard to get anywhere with this.

Test Plan: Got somewhat-encouraging output from "arc land" and "hg arc-ls-markers", but too many things are still broken for this to really work yet.

Maniphest Tasks: T13546, T9948

Differential Revision: https://secure.phabricator.com/D21348
2020-06-10 17:31:50 -07:00
epriestley
727d73fec9 In "arc land", fix some coarse issues with build warnings
Summary: Ref T13546. In the new "arc land": actually reach build warnings; and show buildable URIs.

Test Plan: Ran "arc land ..." with intentionally broken builds, got more useful build warnings.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21347
2020-06-10 10:27:18 -07:00
epriestley
705c48effc Realign "arc land" closed/published warning around more modern language
Summary: Ref T13546. The modern constant from the modern API method for this state is "published", and this more narrowly covers the desired behavior (notably, excluding "Abandoned" revisions).

Test Plan: Ran "arc land ... --revision X" where "X" is a published revision, got an appropriate prompt.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21345
2020-06-10 10:27:18 -07:00
epriestley
3cad824e38 In "arc land" in Mercurial, show a tidier "ls-remote" command
Summary:
Ref T9948. Ref T13546. We must passthru "hg ls-remote" because it might prompt the user for credentials.

Since "ls-remote" is implemented as an exension and we can't rely on using stdout beacuse of passthru, the actual command we execute is:

```
$ hg --config extensions.arc-hg=<huge long path to extension> arc-ls-remote --output <huge long path to temporary file> -- remote
```

This is meaningless and distracting; show the intent of the command we're executing instead. Users can see the raw command in "--trace" if they're actually debugging behavior.

Test Plan: Ran "arc land" in a Mercurial repository, got a tidier command output.

Maniphest Tasks: T13546, T9948

Differential Revision: https://secure.phabricator.com/D21344
2020-06-10 10:27:17 -07:00
epriestley
b1f807f7ca Disambiguate various types of Mercurial remote markers with "hg arc-ls-remote"
Summary: Ref T13546. Ref T9948. It seems challenging to examine a remote in vanilla Mercurial. Provide an "hg arc-ls-remote" command which functions like "git ls-remote" so we can figure out if "--into X" is a bookmark, branch, both, neither, or a branch with multiple heads without mutating the working copy as a side effect.

Test Plan: Ran various "arc land --into ..." commands in a Mercurial working copy, saw apparently-sensible resolution of remote marker names.

Maniphest Tasks: T13546, T9948

Differential Revision: https://secure.phabricator.com/D21343
2020-06-10 10:27:17 -07:00
epriestley
1bb054ef47 Verify remotes ("paths") in Mercurial during "arc land"
Summary: Ref T13546. Parse "hg paths" and validate that the remotes "arc land" plans to interact with actually exist.

Test Plan: Ran "arc land" with good and bad "--into-remote" and "--onto-remote" arguments, got sensible validation behavior.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21342
2020-06-10 10:27:17 -07:00
epriestley
091aebe014 Refine "arc land" behavior when pushing "onto" a new branch
Summary:
Ref T13546. If the "onto" branch doesn't exist yet and has a "/" in it, we need to preface it with "refs/heads" explicitly.

Fix a "false/null" issue with argument validation.

Possibly, "arc land" should prompt you before creating branches in the remote.

Test Plan: Ran "arc land --onto does-not-exist/1.1" and created a branch.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21341
2020-06-08 16:56:46 -07:00
epriestley
ab70626c12 Support "arc land --pick" to pick specific changes out of a sequence
Summary:
Ref T13546. If you have "feature1", "feature2", etc., "arc land feature4" will now land the entire sequence.

Provide "arc land --pick feature4" to work more like the old "arc land" did. This cherry-picks the commits associated with "feature4", then cascades onto the ancestor of the range.

Test Plan: Ran "arc land --pick land14" to pick a change out of a stack.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21340
2020-06-08 16:30:53 -07:00
epriestley
7ddaed9aba Improve "arc land" behavior in the presence of merge conflicts and change sequences
Summary:
Ref T13546. When we encounter a merge conflict, suggest "--incremental" if it's likely to help.

When merging multiple changes, rebase ranges before merging them. This reduces conflicts when landing sequences of changes.

Test Plan: Ran "arc land" to land multiple changes. Hit better merge conflict messaging, then survived merge conflicts entirely.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21339
2020-06-08 16:30:41 -07:00
epriestley
b003cf9310 Remove "arc feature", "arc branch", "arc bookmark", and significant chunks of obsolete marker code
Summary: Ref T13546. Moves away from the older workflows in favor of "arc branches", "arc bookmarks", and "arc work".

Test Plan: Grepped for affected symbols, didn't find any callers.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21337
2020-06-08 16:27:31 -07:00
epriestley
3d64140ff3 Implement "arc work", to replace "arc feature"
Summary: Ref T13546. Fixes T2928. Adds a new "arc work" workflow which functions like the older "arc feature" workflow, but with modern infrastructure.

Test Plan: Used "arc work" to begin work on branches, bookmarks, and revisions in Git and Mercurial.

Maniphest Tasks: T13546, T2928

Differential Revision: https://secure.phabricator.com/D21336
2020-06-08 16:27:27 -07:00
epriestley
5abf0b96c8 Use MarkerRefs to resolve landing symbols in Mercurial
Summary: Ref T13546. Update the Mercurial code which finds default targets and maps symbols to targets under "arc land" to use the new MarkerRef workflow.

Test Plan: Ran "arc land" with (and without) various arguments in Mercurial, saw them resolve in a seemingly sensible way.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21335
2020-06-08 16:27:24 -07:00
epriestley
599ba0f999 Provide a more powerful query mechanism for "markers" (branches/bookmarks)
Summary:
Ref T13546. Various Arcanist workflows, and particularly the MercurialAPI, currently repeat quite a lot of code around parsing branches and bookmarks.

In modern Mercurial, we can generally use the "head()" and "bookmark()" revsets to do this fairly sensibly.

This change mostly adds //more// code (and introduces "arc bookmarks" and "arc branches" as replacements for "arc bookmark" and "arc branch") but followups should be able to mostly delete code.

Test Plan: Ran "arc branches" and "arc bookmarks" in Git and Mercurial.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21333
2020-06-08 16:27:20 -07:00
epriestley
e8c3cc3289 Allow "arc" to accept any prefix of a command as that command
Summary:
Ref T13546. Practically, this allows "arc branch" to run "arc branches".

(This risks overcorrection to some degree, but command correction only occurs if stdout is a TTY so the risk seems limited.)

Test Plan: Ran "arc branch", got "arc branches" as a correction.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21338
2020-06-08 16:26:58 -07:00
epriestley
31d08f9a8f Remove old Mercurial code testing for rebase and phase support
Summary: Ref T13546. The minimum required Mercurial version should now always have these features; if not, they should move to more modern feature tests.

Test Plan: Grepped for affected symbols.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21334
2020-06-08 16:26:39 -07:00
epriestley
78e9cc9c01 Add a check for ambiguous merge strategies after the "history.immutable" behavioral change
Summary: Ref T13546. When users hit a situation where we would select "squash" but would previously select "merge", prevent them from continuing under ambiguous conditions.

Test Plan: Ran "arc land" in Git with "history.immutable" true, false, and not configured.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21332
2020-06-08 16:26:18 -07:00
epriestley
c5192bde34 Allow users to save prompt responses in "arc" workflows
Summary: Ref T13546. Permit users to answer "y*" to mean "y, and don't ask me again".

Test Plan: Answered "y*" to some prompts, re-ran workflows, got auto-responses.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21331
2020-06-08 16:26:18 -07:00
epriestley
f3f31155b7 Format "arc land" passthru commands more nicely, and execute them from CWD
Summary:
Fixes T13380. Ref T13546. Use slightly nicer command formatting for passthru commands, to make it more obvious what's going on and which pieces of output are from "arc" vs various subcommands.

Also, execute repository API passthru commands from the working copy root. All other commands already did this, the older API just didn't support it.

Test Plan: Ran "arc land" in Git and Mercurial repositories, saw nicer output formatting.

Maniphest Tasks: T13546, T13380

Differential Revision: https://secure.phabricator.com/D21330
2020-06-08 16:26:18 -07:00
epriestley
0bf4da60f6 Make Mercurial use "hg shelve" and "hg unshelve" in dirty working copies in "arc land"
Summary: Ref T13546. Implement the equivalents of "git stash" in Mercurial.

Test Plan: Dirtied a working copy in Mercurial, ran "arc land", saw dirty changes survive the process.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21329
2020-06-08 16:22:44 -07:00
epriestley
4d61c00531 Improve final messages under "arc land --hold"
Summary: Ref T13546. Update some of the "arc land --hold" behavior to be more functional/consistent with the updated workflow.

Test Plan: Ran "arc land --hold" under various conditions, got sensible forward/restore instructions.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21328
2020-06-08 16:22:44 -07:00
epriestley
b62919f7e4 Show some "arc" help pages through a configurable pager, like "less"
Summary:
Fixes T5420. Some "arc help ..." is long and most similar commands send this kind of output through a pager.

Use a pager in at least some cases.

Test Plan: Ran "arc help land", got pager output. Ran "arc help land | cat", got raw output.

Maniphest Tasks: T5420

Differential Revision: https://secure.phabricator.com/D21327
2020-06-08 16:22:44 -07:00
epriestley
a30378a34a Update "arc help land"
Summary: Ref T13546. Provide more up-to-date help about the "land" workflow, including modern flags and behavior.

Test Plan: Read "arc help land".

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21326
2020-06-08 16:22:43 -07:00
epriestley
709c9cb6fb Improve the logic for identifying ambiguous commits and applying "--revision" to them
Summary: Ref T13546. This is mostly minor cleanup that improves behavior under "--revision".

Test Plan: Ran `arc land --into-empty` and `arc land --into-empty --revision 123` with ambiguous revisions in history to hit both the force and non-force outcomes.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21325
2020-06-08 16:22:43 -07:00
epriestley
8a53b5a451 When landing changes in an empty repository, merge cleanly in Git
Summary:
Fixes T12876. Ref T13546. When you make the first change in a new Git repository, "arc land" currently can not merge it because there's nothing to merge into.

Support merging into the empty state formally, reachable by using "--into-empty" (which should be uncommon) or "arc land" in an empty repository.

Test Plan:
  - Used "arc land --into-empty --hold ..." to generate merges against the empty state under "squash" and "merge" strategies in Git.
    - Got sensible result commits with appropriate parents and content.

Maniphest Tasks: T13546, T12876

Differential Revision: https://secure.phabricator.com/D21324
2020-06-08 16:19:55 -07:00
epriestley
57d0d690cc Modernize output when pruning branches in Git during "arc land"
Summary: Ref T13546. Make this output look more similar to other modern output.

Test Plan: Ran "arc land", saw consistent-looking output.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21319
2020-06-08 16:19:55 -07:00
epriestley
94f78cf87c Provide more information about merge progress in "arc land" under Git
Summary: Ref T13546. Communicate more progress information and provide additional details when merge conflicts occur.

Test Plan: Hit a merge conflict, saw more helpful output.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21318
2020-06-08 16:19:55 -07:00
epriestley
1552397c86 Sometimes discard already-closed revisions in "arc land"
Summary:
Ref T13546. When we find commits in history which are associated with already-closed revisions, and they weren't named explicitly on the command line, and we're using a squash strategy, discard them.

This generally happens when "feature2" is on top of "feature1", but "feature1" gets amended or branched elsewhere and lands independently.

Test Plan: Ran "arc land feature3" where prior revisions had already landed, got discards on the duplicated changes.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21322
2020-06-08 16:17:20 -07:00
epriestley
6fb84e5164 Add a synopsis and example for "arc help land"
Summary: Ref T13546. Small documentation fix. Mostly so I can have more things to land.

Test Plan: Ran "arc help land", saw help.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21321
2020-06-08 16:17:20 -07:00
epriestley
25afb93f7a In "arc land", rebase branches in natural order
Summary: Ref T13546. When "arc land" performs cascading rebases, do them in "feature1", "feature2", etc., order so they're easier to follow. The outcome is not dependent on execution order.

Test Plan: Landed a change which cascaded many other branches, saw more comprehensible update order.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21320
2020-06-08 16:17:19 -07:00
epriestley
68f28a1718 Substantially modernize the "arc land" workflow
Summary: Ref T13546. This has a lot of dangerously rough edges, but has managed to land at least one commit in each Git and Mercurial.

Test Plan:
  - Landed one commit under ideal conditions in Git and Mercurial.
  - See followups.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21315
2020-06-08 16:17:19 -07:00
epriestley
7d615a97e2 In "arc branch" output, sort branches updated in the same second by name
Summary:
Ref T13546. The new "arc land" workflow can rebase several branches per second. With branches like "feature1", "feature2", etc., this leads to out-of-order listing in "arc branch".

When two branches would otherwise sort to the same position, sort them by name.

Test Plan: Ran "arc branch" after a cascading rebase by "arc land", saw "land5", "land7", "land8", etc., instead of an arbitrary order.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21316
2020-06-08 16:04:12 -07:00
epriestley
86471fc0fe Remove "--ignore-unsound-tests" from "arc diff"
Summary: Ref T13544. This flag only disables a warning and should be a prompt default, not a flag.

Test Plan: Grepped for "ignore-unsound-tests", created this revision.

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21303
2020-06-08 15:58:27 -07:00
epriestley
3ed81d35a2 When "arc" receives SIGWINCH or other signals during display of a prompt, recover
Summary:
Ref T13546. Resizing the terminal window to send SIGWINCH, or other signals, may interrupt "stream_select()" with an error which upgrades to a RuntimeException.

When "stream_select()" fails, continue and test the stream itself.

Test Plan: Waited for a prompt, resized the window. Before patch: SIGWINCH interruption. After patch: undisturbed prompt.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21317
2020-06-08 15:56:59 -07:00
epriestley
0da395ffe4 Introduce "RepositoryLocalState", a modern version of "requireCleanWorkingCopy()"
Summary:
Ref T13546. Introduces a more structured construct for saving and restoring local repository state.

This is similar to old behavior, except that:

  - "arc.autostash" is no longer respected;
  - untracked changes are stashed; and
  - we do not offer to amend.

Test Plan: In future changes, saved and restored various permutations of local state.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21314
2020-06-08 15:40:01 -07:00
epriestley
7ac3b791b0 Provide modern config options for "arc land" configuration
Summary: Ref T13546. Adds modern support for "arc.land.onto", "arc.land.onto-remote", and "history.immutable".

Test Plan: Read configuration in a future change.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21313
2020-06-08 15:40:01 -07:00
epriestley
de607e9fbc Add modern refs and hardpoints for buildables, builds, and build plans
Summary: Ref T13546. Prepares "arc land" to use hardpoint queries to load build information.

Test Plan: Ran `arc inspect --explore revision(1234)`, got a full related object tree including build information.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21312
2020-06-08 15:39:27 -07:00
epriestley
c1a4bee4a1 Add "Author" and "Parent Revision" hardpoints to RevisionRefs
Summary: Ref T13546. These are used by a future "arc land" workflow to support the "Land changes you don't own?" and "Land changes with open dependencies?" prompts.

Test Plan: Ran a future "arc land" flow, hit both prompts.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21311
2020-06-08 15:10:20 -07:00
epriestley
0a4a841f8f Remove the "--less-context" flag from "arc diff"
Summary: Ref T13544. This is an obscure flag and almost never useful. You can accomplish the same goal, roughly, with "git diff | arc diff --raw -". See also PHI675. See also T13338.

Test Plan: Grepped for "less-context", created this revision.

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21301
2020-06-05 13:32:14 -07:00
epriestley
466de2d2e1 Remove "--encoding" flag from "arc diff"
Summary: Ref T13544. This flag is generally questionable, likely has no actual uses, and is slated for obsoletion and replacement elsewhere (see T13338).

Test Plan: Grepped for "encoding", found no relevant hits.

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21300
2020-06-05 13:27:42 -07:00
epriestley
bb81172eb7 Remove "haveUncommittedChanges" property from "arc diff"
Summary: Ref T13544. This property is private and has no writers.

Test Plan: Grepped for symbol.

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21299
2020-06-05 13:26:30 -07:00
epriestley
d0eb822e37 Remove "--lintall" and "--only-new" flags to "arc diff"
Summary:
Ref T13544. These flags change the behavior of the "arc lint" subprocess.

I believe there is no reason to ever use "arc diff --lintall". If you want to find extra warnings to fix, you can use "arc lint --lintall" to express this intent.

Use of "arc diff --only-new" almost certainly means your linters are raising messages at "error" severity which should instead be raised at "warning" severity. If you only care about fixing a particular type of error in changed code, it should be raised as a "warning". The correct remedy is to adjust the severity, not use "--only-new", which is a very broad, slow, complicated hammer.

Test Plan: Searched for "lintall" and "only-new" in this workflow. These flags still exist in "arc lint", but may be changed in the future. Generated this change.

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21298
2020-06-05 13:24:48 -07:00
epriestley
76dc154955 Remove lint and unit excuses and "--advice" and "--excuse" flags from "arc diff"
Summary:
Ref T13544. Long ago, "arc diff" started prompting the user to provide "excuses" when they submitted changes with failing lint or unit tests.

At the time, "arc" was generally more heavy-handed and the review workflow had fewer points where it provided feedback about lint and test issues.

As the workflow has evolved, there is now significantly more feedback (promotion behavior from Draft in Differential, warnings on "arc land", etc). These days, these prompts feel archaic and like they're just getting in the way.

When lint/unit have Harbormaster-triggered components, this prompt is also too early (since Harbormaster tests may fail or raise lint messages later). A modern version of this would look more like putting revisions in some kind of locked state until authors explain issues. It's possible that's worth building, but I'd like to see more interest in it. I suspect this feature is largely just a "nag" feature these days with few benefits.

Test Plan: Grepped for "advice", "excuse", "handleServerMessage", "sendMessage", "getSkipExcuse", "getErrorExcuse", got no hits. Generated this revision.

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21297
2020-06-05 13:22:00 -07:00
epriestley
ff3cea78ee Remove "--use-commit-message/-C" from "arc diff"
Summary:
Ref T13544. This flag was introduced in D1385 (2012) as part of a workflow which no longer exists. I can't recall anyone ever reporting an issue which involves its use and believe it is likely unused. It's not obvious to me why someone would use it in modern "arc".

(The same goal can be accomplished with "--message-file ...", although this requires more steps.)

Test Plan: Grepped for "use-commit-message" and "getCommitMessageFromCommit", ran "arc diff".

Maniphest Tasks: T13544

Differential Revision: https://secure.phabricator.com/D21296
2020-06-05 13:22:00 -07:00
epriestley
6af46f289a Support short aliases and repeatable arguments in Arcanist Workflow arguments
Summary: Ref T13546. Add support for short "-x" flags and repeatable "--say moo --say quack" flags.

Test Plan: In future changes, used "arc diff -m" and "arc land --onto ... --onto ...".

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21310
2020-06-05 12:47:43 -07:00
epriestley
7c80a9006d Add a "%?" ("hint") conversion to "tsprintf()"
Summary: Ref T13546. Future "arc land" workflows use this element to provide "hint" or "next step" suggestions to make error resolution easier.

Test Plan: Ran future "arc land" workflows, saw tips like "use --revision to do such-and-such" or "add these files to .gitignore".

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21309
2020-06-05 12:22:09 -07:00
epriestley
0e82474007 Support appending arbitrary lines to DisplayRef output
Summary:
Ref T13546. Several substeps in the new "arc land" flow benefit from this. For example:

  - When prompting "land revisions you don't own?", it's used to show authors.
  - When prompting "land revisions in the wrong state?", it's used to show the current states.

Test Plan: Ran future "arc land" workflows, got relevant contextual information via this mechanism.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21308
2020-06-05 12:22:09 -07:00
epriestley
fc3974ed70 Impose a HardpointEngine future parallelism limit
Summary:
Ref T13546. If we try to resolve several hundred hardpoint queries which execute subprocesses, we can currently hit system limits.

For now, limit resolution to 32 simultaneous futures. In the future, this should switch to `FuturePool` and possibly become more nuanced.

Test Plan: In a future change, ran `arc land --into-empty ...` to land thousands of commits. Before change, got a "proc_open()" error when launching too many simultaneous subprocesses. After change, this "worked".

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21307
2020-06-05 12:15:47 -07:00
epriestley
7378e2baad Remove special casing of "arc --version"
Summary: See PHI1765. You can find version information with "arc version". Remove this undocumented special-case.

Test Plan:
  - Ran `arc --version`, no longer got a workflow API exception.
  - Searched the documentation for references to `arc --version` (instead of `arc version`), found none.

Differential Revision: https://secure.phabricator.com/D21306
2020-06-02 08:58:06 -07:00
epriestley
0da1a2e17d Allow PhutilArrayCheck to accept a list of objects as a context
Summary: This simplifies merging a list-of-lists, which is a common pattern when asking a list of extensions to each provide some kind of list of items.

Test Plan: Used elsewhere in Piledriver.

Differential Revision: https://secure.phabricator.com/D21294
2020-05-30 03:37:42 -07:00
epriestley
a0c346bf63 Add a support class to simplify typechecking list-of-objects return values
Summary:
With some frequency, code wants to assert that some "$o->m()" returns a list of objects of type X, possibly with unique values for some "getKey()"-style method result.

Existing checks via `PhutilTypeMap` and `assert_instances_of()` aren't quite powerful enough to do this while producing an easily understandable error state. We want to know that the error arose from a call to "$o->m()" in particular.

Test Plan: Currently used elsewhere, in Piledriver code.

Differential Revision: https://secure.phabricator.com/D21293
2020-05-28 07:33:23 -07:00
epriestley
c76cfc8c82 Mark the wildcard argument to "arc liberate" as a path argument for shell completion
Summary: This is a path argument, and shell completion should suggest tabs.

Test Plan: Typed "arc liberate s<tab>", got path suggestions.

Differential Revision: https://secure.phabricator.com/D21292
2020-05-28 07:33:12 -07:00
epriestley
fce72b9c89 Make lint tests handle paths better and distinguish between "0" and "null" more carefully
Summary:
Ref T13543. Currently, the `cpplint` tests do not function because `cpplint` is passed a path which does not end in a suffix it recognizes.

Change the tempfile / path code to pass `linter path/to/example.c`-style linters a path they expect.

Then, correct some older code which was playing it fast-and-loose with "null" vs "0".

Test Plan: Ran `arc unit --everything`, got a clean bill of health on all the linters I have installed. (This is probably not all tests, since I have only a subset of linters installed locally that we have code for.)

Maniphest Tasks: T13543

Differential Revision: https://secure.phabricator.com/D21291
2020-05-27 12:38:11 -07:00
epriestley
e69aa32603 Fix an issue when rendering a lint message which removes whitespace at the end of a file
Summary:
Ref T13543. If a file ends in spaces and no newline, we'll emit a message suggesting removal of the spaces. This will effectively remove the line, but the code will then attempt to highlight text within the line.

Prior to D21044 this continued without raising an error and produced a reasonable result, but it now fatals. Insetad, don't try to highlight lines which no longer exist.

Test Plan: See T13543 for details.

Maniphest Tasks: T13543

Differential Revision: https://secure.phabricator.com/D21290
2020-05-27 11:28:16 -07:00
epriestley
25ee39b657 In the "cpplint" binding, raise messages on "line 0" without a line
Summary:
Ref T13543. The "cpplint.py" script may emit messages on line 0, but Arcanist doesn't accept these messages.

This is a small piece of a whole set of broader issues, but stop the bleeding for now.

Test Plan:
  - Ran `arc lint example.h` on a file with no `#ifndef` guard, and `cpplint` configured.
  - Cpplint raised a message at line "0".
  - Before change: arc choked when trying to render this.
  - After change: arc survives rendering.

Maniphest Tasks: T13543

Differential Revision: https://secure.phabricator.com/D21289
2020-05-27 10:59:10 -07:00
epriestley
e3030ebcad Allow construction of a ConduitEngine with a bare ConduitClient
Summary:
See PHI1735. "ConduitEngine" was once a future pool, but this has moved to "HardpointEngine". This class may no longer make much sense.

In Phacility code, "bin/host upload" depends on using the Uploader, which needs a "ConduitEngine", not a "ConduitClient". This workflow may use asymmetric key signing, which "ConduitEngine" does not support.

To unblock PHI1735, provide glue code between "Client" and "Engine". But a "more correct" change is probably removal of "Engine".

Test Plan:
  - Ran `bin/host upload`, uploaded files (with additional changes to wrap the Client).
  - Created this revision.

Differential Revision: https://secure.phabricator.com/D21260
2020-05-15 08:24:10 -07:00
Aviv Eyal
2d8156a727 update SSL error messge re:libphutil
Test Plan: ╰(*°▽°*)╯

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21258
2020-05-15 12:15:50 +00:00
epriestley
b76b9c4065 Add "HTTPSFuture->addCurlOption()" for raw access to "curl_setopt()"
Summary: Fixes T13533. This is a narrow, fragile API for a particular Kerberos use case on one install.

Test Plan:
- Set a non-scalar key, got an exception.
- Set <"duck", "quack">, got an exception from cURL that the value was invalid.
- Set a bunch of made-up options to arbitrary values, no errors. cURL accepts anything so there's nothing we can do about this.
- Set `CURLOPT_NOBODY` and saw the request behavior change, demonstrating that the call can produce effects.

Maniphest Tasks: T13533

Differential Revision: https://secure.phabricator.com/D21251
2020-05-14 09:16:36 -07:00
epriestley
6937d38947 Fix an initialization issue in VectorTree
Summary: Ref T13520. In unusual cases where there are no changes in a changeset list (e.g., empty commits) we can fatal when trying to iterate over an empty list of vectors.

Test Plan:
  - Created an empty commit.
  - Used "git show | pbcopy" to create a diff from it.
  - Viewed it in the web UI.
  - Before: fatal when iterating on `null`.
  - After: clean page.

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21221
2020-05-04 15:50:26 -07:00
epriestley
af9faba02f Add "--browse" and "--input" to "arc paste", and remove "--json" (which had no effect)
Summary:
Ref T13528. For consistency with other commands ("arc upload", "arc diff"), support a "--browse" flag to "arc paste".

Support "--input" as a more robust alternative to `x | y` (see T6996).

Test Plan: Ran `arc paste --browse --input X`, got a new paste in a browser window. Ran other variations of flags and parameters.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21203
2020-05-01 09:13:20 -07:00
epriestley
c0d151e0e9 Add "--browse" to "arc upload" and update behavior, particularly "--json"
Summary:
Ref T13528. Provide a "--browse" flag to open files after they are uploaded.

Update the code to use modern query strategies.

This impacts the output of "--json", which was just raw "file.info" output before and could not represent the same path being passed several times (`arc upload X X`).

Test Plan: Ran `arc upload` with `--browse` and `--json`.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21202
2020-05-01 09:13:01 -07:00
epriestley
5448fe2165 When recent PHP raises a "broken pipe" error in ExecFuture, treat it as a blocked stdin
Summary:
Ref T13528. If we start a subprocess that immediately exits and then write to it, we can get a broken pipe error.

Recent versions of PHP appear to raise this as an actual warning, and recent changes upgrade the warning to a runtime exception.

I can't find any way to tell if the RuntimeException is a broken pipe or something else, except by examining the text of the error string.

At least for now, treat this like a "blocked pipe" condition. Since the subprocess has exited and the bytes didn't write, this should generally be reasonable.

Test Plan:
  - Viewed a file in Paste with an extension that Pygments does not have a lexer for.
  - This causes Pygments to exit immediately with an "unrecognized lexer" error. This closes the pipe, and the next write will fail with a broken pipe error.
  - Before patch: fatal on broken pipe.
  - After patch: clean resolution of the future and error condition.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21199
2020-05-01 09:12:43 -07:00
epriestley
a77cfb023d When a proxy future wraps a future which throws an exception, resolve with an exception
Summary:
Ref T13528. When you call `$future->resolve()`, we currently guarantee it is resolved by calling `FutureIterator->resolveAll()`.

`resolveAll()` does not actually "resolve()" futures: it guarantees that they are ready to "resolve()", but does not actually call "resolve()".

In particular, this means it does not throw exceptions.

This can lead to a case where a Future has "resolve()" called directly (e.g., via a FutureProxy), uses "FutureIterator" to resolve itself, throws an exception inside "FutureIterator", the exception is captured and attached to the Futuer, then the outer future tries to access results. This fails since it's out-of-order.

This can happen in practice with syntax highlighting futures, which may proxy pygments futures.

Instead, "resolveAll()" before testing for exaceptions.

Test Plan:
  - Locally, tried to highlight a Paste with an unrecognized lexer extension using Pygments.
  - Before patch: fatal when trying to access results of a Future with no results (because it has an exception instead).
  - After patch: resolution throws the held exception properly.
  - (See also next change.)

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21198
2020-05-01 09:12:22 -07:00
epriestley
696ec3f975 Work around "mb_check_encoding(<stringlike-object>)" warning in particular versions of PHP
Summary: Fixes T13527. Some versions of PHP strictly require that we pass a string value, and reject "stringlike" objects (objects which implement "__toString()").

Test Plan: Ran unit test, although this is somewhat aspirational because my local PHP version isn't affected.

Maniphest Tasks: T13527

Differential Revision: https://secure.phabricator.com/D21193
2020-04-30 07:19:45 -07:00
epriestley
284139a24e Restore the ":(attr:filter=lfs)" test for LFS
Summary:
See D21190. The ".gitattributes" approach fails when ".gitattributes" is in a subdirectory (or global). These are probably unusual cases, but at least one is known in the wild.

Instead:

  - Restore the ":(attr:filter=lfs)" test, which seems to be the fastest accurate test available in modern Git.
  - If the test fails, assume the repository is not LFS. This only impacts users running very old versions of Git.

Test Plan:
  - In LFS and non-LFS repositories, created diffs. Saw correct detection again.
  - Broke the command on purpose, saw LFS detection conclude "no LFS", but not fail disastrously.

Subscribers: ptarjan

Differential Revision: https://secure.phabricator.com/D21192
2020-04-29 21:04:44 -07:00
epriestley
ade9b51a1f Detect LFS by looking for tracks in ".gitattributes" instead of using "ls-tree"
Summary:
See PHI1718. See also <https://discourse.phabricator-community.org/t/arc-diff-fails-due-to-git-cmd-fails/3680/>.

Currently, `arc diff` detects Git LFS with `git ls-files -z -- ':(attr:filter=lfs)'` magic. This is an accurate test, but does not work on older Git.

Try a simpler, dumber test and see if that will work. If this also has issues, we can try this stuff:

  - do version detection;
  - pipe the whole tree to `git check-attr`;
  - try a command like `git lfs ls-files` instead, which is probably a wrapper on one of these other commands.

Test Plan:
  - In a non-LFS repository, ran "arc diff" and saw the repository detect as non-LFS.
  - In an LFS repository, ran "arc diff" and saw the repository detect as LFS.

Differential Revision: https://secure.phabricator.com/D21190
2020-04-29 16:23:44 -07:00
epriestley
6ec09b2f48 Replace "PhutilFileTree" with a more abstract "VectorTree"
Summary:
Ref T13520. Replace "FileTree" with a "VectorTree" that does roughly the same thing. The major goals are:

  - Compress trees which contain sequences of child directories with no sibilings.
  - Build hierarchies of paths where path components may include renames.

This is approximately similar to "FileTree" and similar to client logic in the new paths panel.

Test Plan: See next change.

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21182
2020-04-28 12:09:56 -07:00
epriestley
b81818b287 Accommodate PHP 7.4 changes to certain "preg_match_all()" calls
Summary:
Ref T13518. The result format of this call changed in PHP 7.4, which causes us to emit "-1" matches because "-1" survives `array_filter()`.

Filter results in a way that should survive both result formats.

Test Plan: Ran `arc unit --everything` under PHP 7.4.

Maniphest Tasks: T13518

Differential Revision: https://secure.phabricator.com/D21173
2020-04-26 08:40:08 -07:00
epriestley
bf76fa547d Make "arc <workflow> --help" work again for workflows which haven't updated yet
Summary:
See <https://discourse.phabricator-community.org/t/help-is-no-longer-present-for-arc-subcommands-in-todays-stable/3786>.

The "--help" flag ends up falling through to the old "arcanist.php", where it becomes lost. Catch it earlier so "arc diff --help" prints diff help, for instance.

Test Plan: Ran `arc help diff`, `arc diff --help`, `arc --help diff`, and similar commands for updated workflows; got help.

Differential Revision: https://secure.phabricator.com/D21168
2020-04-25 08:57:28 -07:00
epriestley
68f050bd14 Allow HTTPFuture callers to disable processing of "Content-Encoding" response headers
Summary: Ref T13507. In Phabricator, we perform a specific "Accept-Encoding: gzip" setup test and want to manually decode the result. Allow callers to disable handling of "Content-Encoding".

Test Plan: Ran all Phabricator setup checks.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21121
2020-04-15 06:28:25 -07:00
epriestley
377ed2ed8d If the Conduit server asserts it has the "gzip" capability, compress requests
Summary:
Ref T13507. For various messy reasons we can't blindly assume the server supports "gzip" -- but if the server tells us it does, we're on firmer ground.

If the server returns an "X-Conduit-Capabilities: gzip" header and we have compression support locally, compress subsequent requests.

This restores D21073, which was reverted by D21076.

Test Plan: With a gzip-asserting server, added debugging code and ran various "arc" commands. Saw the 2nd..Nth calls hit compression code.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21119
2020-04-14 16:50:54 -07:00
epriestley
a77da426af If the Conduit client supports gzip, make calls with "Accept-Encoding: gzip"
Summary:
Ref T13507. Add "Accept-Encoding: gzip" to requests if we can decompress responses.

When we receive a compressed response, decompress it.

Test Plan: Added debugging code, ran some commands, saw smaller payloads over the wire and inline decompression.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21118
2020-04-14 16:23:53 -07:00
epriestley
890b57de1e In "phutil_loggable_string()", encode every byte above 0x7F
Summary:
Ref T13507. Currently, this function is a bit conservative about what it encodes, and passing it a string of binary garbage may result in an output which is not valid UTF8.

This could be refined somewhat, since it's less than ideal if the input has valid UTF8. The ideal behavior for byte sequences where all bytes are larger than 0x7F is probably a variation of "phutil_utf8ize()" that replaces bytes with "<0xXX>" instead of the Unicode error glyph.

For now, just err on the side of mangling.

Test Plan: Dumped various binary payloads in the new gzip setup check, saw sensible output in the web UI.

Maniphest Tasks: T13507

Differential Revision: https://secure.phabricator.com/D21117
2020-04-14 16:03:12 -07:00
epriestley
9d0100bda7 Only inject legacy Arcanist workflows into "help" if run from the context of an Arcanist runtime
Summary: Ref T13490. This code is reachable from Phabricator binaries; only inject the legacy stuff if we're in an Arcanist stack.

Test Plan: Ran `bin/conduit help` from `phabricator/`.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21113
2020-04-14 13:24:00 -07:00