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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary: Ref T13490. More-or-less straightforward upgrade to modern calls.
Test Plan: Created and viewed pastes.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21104
Summary:
Ref T13490. One of the biggest issues users are hitting in modern "arc" is that workflows don't appear in "arc help" until they're updated.
Since there's still some work to do and gluing them in isn't terribly difficult, at least get things connected for now.
Test Plan: Ran "arc help", "arc help diff".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21101
Summary: Ref T13490. These workflows don't seem worth the maintenance cost, see T13488 for discussion.
Test Plan: Grepped for methods which looked like they might only be called by these flows, only dug up the backout stuff.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21100
Summary: Ref T13490. The "RevisionRef" is currently based on "differential.query" data, but all calls other than the hash-based lookup can move to "differential.revision.search".
Test Plan: Ran revision workflows like `arc inspect` and `arc browse`.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21099
Summary: Ref T13490. I recently made "tsprintf()" more strict, but we do sometimes use "%d" and this is reasonable to support.
Test Plan: Ran "arc branch".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21098
Summary:
Ref T13490. This is mostly straightforward.
- Drop "--show" in favor of "--as -".
- Drop support for 4+ year old "file.info" API.
- Use modern stream-to-disk support so we get a real progress bar and don't need to buffer files into memory.
Test Plan: Downloaded various files, including large files.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21097
Summary: Ref T13490. Moves "arc amend" to Toolsets with modern ref/hardpoint code.
Test Plan: Ran "arc amend --show", "--revision", etc. Hit all the prompts and errors, probably?
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21095
Summary:
Ref T13490. I'm continuing to move toward modernizing "amend", which needs to load some objects by symbol at top level.
Add an API to support this.
Test Plan: Added an API caller to "arc inspect", ran it and hit the new code. Things seemed to work.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21094
Summary: Ref T13490. Make terminal strings work more like HTML does in Phabricator, and make it easier to display refs.
Test Plan: Added some display code, ran `arc inspect` to hit it, saw a nice ref printed.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21093
Summary:
Ref T13490. I'm attempting to update "arc amend", but it needs to fetch revision authors to raise an "amending a revision you don't own" error.
Support user-symbol resolution.
Along the way, this introduces some infrastructure for abstracting away iteration over a multi-page Conduit result set.
Test Plan:
- Ran "arc inspect ..." for various "user(...)" queries.
- Set page size to 3 and issued a general query, saw the future page it away properly.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21092
Summary:
Ref T13490.
- Support resolution of revision symbols into revision objects.
- Align commit inspection better against commit symbols.
- Make "arc inspect --explore" recursively load all object hardpoints.
- Add a "commit message" hardpoint to "RevisionRef".
Test Plan: Used "arc inspect" to resolve commits and revisions. Used "--explore" to see the whole tree.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21091
Summary:
Ref T13490. Frequently, we know the symbolic name of a commit (like "master") but need the immutable identifier for it (the commit hash).
Provide a Ref and Query for doing this lookup.
Test Plan: Ran `arc inspect symbol(...)` with various symbols, saw appropriate resolutions, nulls, or errors.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21090
Summary:
Ref T13490. "ExecFuture" can raise a "CommandException" when the subprocess fails. In most cases, this is desirable, since we usually do not expect subprocesses to fail.
Currently, "execx()" (which raises these exceptions) is the synchronous default (with the more verbose "exec_manual()" as an alternative if you expect the command may return an error code), but the Future variation has no way to hint that external resolution should raise an exception if the process exits with an error.
Since the "HardpointEngine" yield construct can resolve external futures, add a mode for this to simplify implementing "HardpointQuery" classes a bit. Without this, they either need to retain "$futures" and manually call "resolvex()", or check the "$err" result and throw some other exception, both of which are low-value boilerplate (queries that want to do this still can, of course).
This is basically like providing a hint that the futures should be resolved with "resolvex()" instead of "resolve()".
Also, make this the default behavior of a new "$api->newFuture()" wrapper.
Test Plan: Intentionally broke a command in a HardpointQuery, got a sensible exception automatically during external future resolution.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21089
Summary: Ref T13490. There's more `array(...)` happening in this API than necessary. Sugar it slightly.
Test Plan: Grepped for "loadHardpoints()", ran a couple workflows.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21088
Summary: Ref T13490. A few older "defineHardpoint()" calls are sticking around. They no longer have callers; get rid of them.
Test Plan: Grepped for this symbol, no hits.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21087
Summary:
Ref T13490. These workflows are aliases of one another, which is a little silly, but currently all identify as "arc feature" in "arc help".
Straighten that out, at least.
Test Plan: Ran "arc help", "arc branch".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21086
Summary:
Ref T13490. Bring "arc prompts" from "wilds" and hook it into the prompt in "arc shell-complete".
See D21069. Fix an issue where the shell hook tested for a path other than the path it writes to.
Test Plan: Ran "arc shell-complete" with no hook and got a prompt. Shell completed things. Ran "arc prompts shell-complete".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21085
Summary: Ref T11968. "RefQuery" is now "HardpointEngine". "HardpointLoader" is now "HardpointQuery".
Test Plan: Grepped for affected symbols.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21083
Summary:
Ref T11968. "arc browse", "arc branch", and "arc diff" currently may execute into the RefQuery engine. Reroute them to the HardpointEngine.
This removes older-generation "Ref" objects and renames the replacement "RefPro" objects to "Ref".
Test Plan: Ran "arc branch", "arc browse <various things>", "arc diff", searched for affected symbols.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21082
Summary: Ref T11968. Merge the modern hardpoint queries and refs for the "browse" workflow as "pro" variations.
Test Plan: Ran `arc inspect browse(...)` for objects, paths, commits, and revisions.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21080
Summary:
Ref T11968. Continue bringing modern yield-based hardpoint code into "master" in the parallel "Pro" classtree.
Adds "working-copy(commit-hash)" as an inspectable ref.
Test Plan: Inspected working copy refs, saw them resolve revisions by commit hash and commit message.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21079
Summary:
Ref T11968. Inches toward the new ref/hardpoint code by introducing the modern refs as "RefPro" objects and supporting an "arc inspect <object>" to load objects and hardpoints.
This doesn't impact any existing runtime behavior.
Test Plan: Ran "arc inspect [--all] commit(...)", got hardpoint queries and yield-based data fetching.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21078
Summary:
Depends on D21074. Ref T13490. Ref T11968. Before toolsets, Arcanist has a "WorkingCopyIdentity" object. After toolsets, it has a "WorkingCopy" object.
Most workflows don't access either one, so make a slightly-breaking API change to simplify the transition.
- "getWorkingCopy()" now returns a modern object (a "WorkingCopy").
- "getWorkingCopyIdentity()" now returns an older object (a "WorkingCopyIdentity").
This isn't backward-compatible, but out-of-date code should get an explicit failure with clear resolution steps.
Test Plan: Ran "arc lint", "arc branch", and a few other commands. Grepped for "getWorkingCopy()".
Maniphest Tasks: T13490, T11968
Differential Revision: https://secure.phabricator.com/D21075
Summary:
Ref T11968. Ref T13490. These are the workflows which currently use the intermediate-level hardpoint code (which made hardpoints formal, but didn't do the yield stuff).
Move toward updating them by doing some basic bookkeeping, with a few compatibility adjustments to the parent Workflow class.
Test Plan: Ran "arc branch" and "arc browse" with various arguments.
Maniphest Tasks: T13490, T11968
Differential Revision: https://secure.phabricator.com/D21074
Summary:
This reverts commit 7e25288f49. See T13507 for discussion, shortly.
This needs to be switched to capability detection because some server configurations can not actually accept these requests.
Test Plan: Straight revert.
Differential Revision: https://secure.phabricator.com/D21076
Summary: Ref T13507. Enable compression in the body of Conduit requests if we have client support for it; we expect the server should always support it.
Test Plan: Created this revision, ran random "arc" commands that use Conduit. Added debugging code, saw payload size drop.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21073
Summary:
Depends on D21071. Ref T11968. Currently, "ConduitEngine" tries to lightly parallelize futures. This was a compromise when the initial "hardpoint" change didn't plan to pursue real request paralleization.
Now that the newer hardpoint change does, we don't need onboard resolution in ConduitEngine. Throw it away.
When the engine is supposed to resolve a future, it now just resolves that future on its own. This should be functionally identical to the previous behavior, except that it may be slower.
(In practice, because HTTP futures are backed by an internal cURL request pool, this proably has little effect anywhere. Moving to modern hardpoints will make performance no worse than it was prior to this change, in any case.)
Test Plan: Ran various modern "arc" commands.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21072
Summary:
Depends on D21070. Ref T11968. Adds "yield"-aware query classes for parallelizing service calls.
These will replace the similar (but not yield-aware) "Hardpoint" classes introduced previously. This is an API change but most of the old classes still exist and still do the same thing, just with more "yield" statements.
This just adds a bunch of new code with no callers and no API updates.
Test Plan: See future changes.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21071
Summary:
Ref T11968.
- Allow "WorkingCopy" objects to maintain an API object and update callers ("get...()" instead of "new...()").
- Always generate a WorkingCopy object and a RepositoryAPI object.
Currently, code has to look like this:
```
$working_copy = ...
if ($working_copy) {
$repository_api = ...
if ($repository_api [instanceof ... ]) {
```
This is clunky. There's also no reason some "arc" commands can't run outside a VCS working directory without special-casing how they interact with the filesystem.
Conceptually, model the filesystem as a trivial VCS (which stores exactly one commit, always amends onto it, and discards history). Provide a trivial WorkingCopy and API for it.
(This change isn't terribly interesting on its own, but chips away at landing the new Hardpoint infrastructure.)
Test Plan: Ran `arc version`, `arc upgrade`.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21070
Summary:
Depends on D21067. Ref T13492. Converting unit tests to be readable exposed this error in the grammar.
Normally, "grammar_rule" rules emit a standalone Node. In this case, the bottom-level grammar rule is a collection of trivial rules and the callers configure the Node. Wrap the bottom-level rule in a configuration rule so the node is configured correctly and consistently, in exactly one place.
Test Plan: Ran unit tests.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21068
Summary:
Depends on D21066. Ref T13492. The switch of unit test data to stable/readable output exposed this bug in parsing of variadic calls: some nodes are not given types properly.
Fix the parser and update the test.
Test Plan: Ran the test, which now works.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21067
Summary:
Depends on D21065. Ref T13492. Swap existing "expect" blocks from unstable, unreadable JSON to readable, stable trees.
(There are two "INVALID TYPE" outputs which this update effectively detects and which future changes correct.)
Test Plan: Ran "arc unit --everything", got a clean build.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21066
Summary:
Depends on D21064. Ref T13492. Earlier, see D17819. This is essentially the same change, although I inlined the token stream into the node list.
This intentionally breaks most tests since it just has the new "expect" generator; the next change will fix them by swapping the test bodies.
Test Plan: Ran "arc unit --everything" after the next change (which fixes all the tests), got a clean build. This change on its own fails all existing XHPAST tests since the block formats don't match.
Maniphest Tasks: T13492
Differential Revision: https://secure.phabricator.com/D21065
Summary: Ref T13490. The new Arcanist runtime supports workflow signal handling, but Phage isn't quite able to make use of it. Clean up the last few pieces so it can work.
Test Plan: Ran "phage", hit ^C, got status information.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21060
Summary: Ref T11968. Phage has another "sustained pool of Futures" use case, and needs some slight adjustments after Future API changes.
Test Plan: Ran `bin/phage status ...`, got a clean result instead of a JSON decoding failure.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21058
Summary:
Ref T11968. "FutureIterator" recently became non-rewindable, and starting a Future twice is now an error.
This complicates a handful of use cases where a mostly-constant pool of futures is maintained over a long period of time, notably in daemon overseers and repository pull daemons.
They previously relied on being able to do "new FutureIterator($futures)" to continue resolution of a list of futures from any state. This no longer works quite like it used to, since Futures generally may not belong to more than one iterator now (and this property is desirable).
Introduce "FuturePool", which maintains exactly one iterator but manages the small amount of glue around adding and removing Futures from it, destroying it if the pool empties, and rebuilding it if the pool fills.
Test Plan: See next change, which makes use of this.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21053
Summary:
Depends on D21051. Ref T13504. Ref T13209. This very old Mercurial code uses "X=Y hg ..." on Linux and "set X=Y & hg ..." on Windows.
The latter construct no longer works because we bypass the shell. The former construct is obsolete.
Additionaly, delete some ancient "branch merge" code which has no callers.
Test Plan: Created a diff in a Mercurial repository on Linux. I minimally vetted this on Windows since I don't have a "hg + Windows" environment at the moment.
Maniphest Tasks: T13504, T13209
Differential Revision: https://secure.phabricator.com/D21052
Summary:
Ref T13504. Ref T13209. We currently use "escapeshellarg()" under Windows, which destructively replaces "%" symbols with spaces. This is wrong, breaks general behavior with "git log --format=...", and has led to a lot of weird workarounds in `arc` code where we don't escape arguments we should be escaping to tiptoe around issues with "%".
Now that execution occurs via "bypass_shell", we can safely (probably?) escape things, although programs are still apparently free to parse the command string however they want.
Test Plan: Added unit tests, ran them under Mac and Windows, got clean results. Ran "arc version" on Mac and Windows.
Maniphest Tasks: T13504, T13209
Differential Revision: https://secure.phabricator.com/D21051
Summary:
Ref T13504. The API has changed here slightly, and if you run "arc version" without "arcanist/" being a Git working copy, it currently fatals in a misleading way.
Instead, reach the error properly.
Test Plan: Ran "arc version" after moving aside ".git/", got a helpful error message instead of a confusing "call on null" exception.
Maniphest Tasks: T13504
Differential Revision: https://secure.phabricator.com/D21050
Summary:
Fixes T13504. This fixes three issues:
# In ExecFuture, "proc_open()" on an invalid binary could fail with an unconditional exception.
# In ExecPassthru, "proc_open()" on an invalid binary could fail with an unconditional exception.
# In "arc browse", "start <url>" does not work when the shell is bypassed.
In (1) and (2), the desired behavior is to fail with an exit code which is sometimes upgraded to an exception depending on calling convention.
Issue (1) most commonly manifested as "find" failing when run via "cmd.exe".
Issue (2) most commonly manifested as "arc browse" failing.
Issue (3) was entangled with issue (2).
In cases (1) and (2), assume "proc_open()" failures under Windows are because of bad binaries and treat them like bogus commands on Linux/Mac.
In case (3), use "cmd /c start" instead of "start" as a default browser on Windows.
Test Plan:
- On Windows, did mime type detection in cmd.exe. Before patch: proc_open() exception in "find". After patch: clean (albeit not terribly useful) detection.
- On Windows, did "arc browse ...". Before patch: proc_open() exception in "start". After patch: clean browser execution.
Maniphest Tasks: T13504
Differential Revision: https://secure.phabricator.com/D21047
Summary:
Ref T11968.
- Rekeying natural lists of futures creates more problems than it solves. For now, don't try to deal with this.
- The scope of the future autokey was incorrect (per subclass, not per process).
Test Plan: Ran various future-oriented flows on new HardpointEngine code.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21046
Summary:
Depends on D21036. Ref T11968. Ref T13177. Currently, each Future integrates separately with ServiceProfiler, but much of the code is similar.
Move integration to the base class and lift up most of the implementation details.
Test Plan: Ran `arc diff --trace`, saw sensible output.
Maniphest Tasks: T13177, T11968
Differential Revision: https://secure.phabricator.com/D21038
Summary: Depends on D21035. Ref T11968. This allows a "FutureIterator" to hold futures which are blocked because of unresolved dependencies, and makes the resolution process more structured.
Test Plan: Ran unit tests, created this revision.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21036
Summary: Depends on D21033. Ref T11968. This is just an incremental step in modernizing Future and making it more robust. Currently, subclasses are expected to write directly to `$this->result`, but this isn't consistent with how modern classes generally work.
Test Plan: Ran unit tests, created this revision.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21034
Summary:
Depends on D21032. Ref T11968. Currently, "Future" and "FutureIterator" can both resolve futures.
Treat "Future->resolve()" as sugar on resolving an iterator of size 1.
Test Plan: Ran tests, created this revision.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21033
Summary:
Ref T11968. This future-level parameter has no nontrivial callers and makes the "fate of FutureGraph" changes more difficult.
Callers that are genuinely interested in this behavior can wrap the Future in a FutureIterator and use "setUpdateInterval()" to get the same behavior.
Test Plan: Grepped for "resolve()" and "resolvex()", updated callers.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21031
Summary:
One of our users ran into an issue where they were running `arc diff` in
a subdirectory and it so happened that in that directoy there wasn't any `lfs`
files so the repo was marked as `$is_lfs == false`. Lets allow `arc` to be run
from anywhere.
Test Plan: Internally this patch worked
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21045
Summary:
Ref T13499. Currently, we throw most language errors as exceptions, but this list isn't exhaustive and errors we don't specifically make more severe are allowed to slip through.
This is generally undesirable, particularly in the case of "Undefined index:" errors. See T13499 for a specific case where this caused behavior to be more difficult to understand and diagnose than it should have been.
Make this the default behavior instead, except for "E_USER" errors, which we never expect to arise from first-party code.
This may be slightly too aggressive, but future changes can selectively reduce the severity of some types of errors if problems arise.
Test Plan:
- Executed code which intentionally accessed an undefined index, got an exception.
- Poked around Phabricator and Arcanist without any further issues cropping up, but I don't have a good way to develop confidence that the "reduced severity" list should genuinely be empty.
Maniphest Tasks: T13499
Differential Revision: https://secure.phabricator.com/D21044
Summary: Fixes T13498. Currently, `arc diff` may invoke the file upload flow using an older variation of the API. Both the modern and fallback clients build an Engine, so switching to the modern API is trivial.
Test Plan:
- Created a local commit with a 1MB binary file.
- Ran `arc diff --only`.
- Before patch: fatal on old API call.
- After patch: clean upload.
Maniphest Tasks: T13498
Differential Revision: https://secure.phabricator.com/D21043
Summary:
I'm working on a fairly large monorepo using phabricator and we are
running into a `git-lfs` issue. Every single `arc diff` we have to wait
many seconds while all our lfs files are walked before the push goes through.
I chatted with the `git-lfs` folks in
https://github.com/git-lfs/git-lfs/issues/4076 and they suggested this workflow
change. I tested it on our repo and it does indeed seem to fix the issue. Three
things were change:
1. The remote url is named
2. Use branches instead of tags
3. Do a `git fetch`
(Also, hello after all this time! Hope you're all doing well over at phacility.)
Test Plan:
Before
```
$ arc diff
Linting...
LINT OKAY No lint problems.
Running unit tests...
UNIT OKAY No unit test failures.
PUSH STAGING Pushing changes to staging area...
Uploading LFS objects: 100% (8211/8211), 1.0 GB | 0 B/s, done.
Enumerating objects: 31, done.
Counting objects: 100% (31/31), done.
Delta compression using up to 12 threads
Compressing objects: 100% (21/21), done.
Writing objects: 100% (24/24), 1.77 KiB | 8.00 KiB/s, done.
Total 24 (delta 19), reused 0 (delta 0)
remote: Resolving deltas: 100% (19/19), completed with 6 local objects.
To github.com:robinhoodmarkets/web-staging.git
* [new tag] 2581578b59b6341c1023377fd8741814e89fcd0c -> phabricator/base/427095
* [new tag] 7d9be50fbca590f15742d8cc20edd4f082dfbb3b -> phabricator/diff/427095
Updated an existing Differential revision:
Revision URI: https://phabricator.robinhood.com/D135442
Included changes:
M __shared/tools/arcanist-js/src/workflow/RHArcanistDiffWorkflow.php
```
After
```
$ arc diff
Linting...
LINT OKAY No lint problems.
Running unit tests...
UNIT OKAY No unit test failures.
PUSH STAGING Pushing changes to staging area...
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 12 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 638 bytes | 2.00 KiB/s, done.
Total 8 (delta 6), reused 0 (delta 0)
remote: Resolving deltas: 100% (6/6), completed with 6 local objects.
To github.com:robinhoodmarkets/web-staging.git
* [new branch] 2581578b59b6341c1023377fd8741814e89fcd0c -> phabricator/base/427105
* [new branch] 274cc3a25eb7856291c1d4c078b28b76af8a43b2 -> phabricator/diff/427105
Updated an existing Differential revision:
Revision URI: https://phabricator.robinhood.com/D135442
Included changes:
M __shared/tools/arcanist-js/src/workflow/RHArcanistDiffWorkflow.php
```
(notice the lack of `Uploading LFS objects`)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21041
Summary:
Depends on D21031. Ref T11968. This clears some lint with the word "Future", but is just a minor cleanup patch not really related to the main goal in that task.
We currently warn on `preg_quote()` with no second parameter, but this is valid and correct:
```
preg_match('('.preg_quote($x).')', ...)
```
This is the modern recommended style for this sort of expression, so the warning is often a false positive; drop it.
The "__CLASS__" warning looks for hard-coded class names in strings, but currently looks for them as a word anywhere in the string.
This is often a false positive and sometimes makes error messages or exceptions untranslatable. For example, translators can't do anything with this:
> Filesystem path "%s" does not exist.
...if it's written as:
> %s path "%s" does not exist.
...just because it happens to appear in the class "Filesystem". Some other classes, including "Future", suffer from this.
Even when the detection is correct, it's clunky and a mistake here (failing to update the class name in an error message) is unlikely to ever do any real damage.
Test Plan: Ran unit tests and `arc lint`.
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21032
Summary:
See PHI1663. If "phabricator.uri" is not configured, we try to fall back to "default", but doesn't have a modern config specification and fails.
Instead, read "default" as a raw config value to preserve compatible behavior. My intent is to eventually remove it.
Test Plan:
In a directory with no "phabricator.uri" config available, ran `echo {} | arc call-conduit conduit.ping`. After the patch, got a reasonable error instead of a fatal.
In a directory with "default" configured but not "phabricator.uri", ran the same command. After the patch, got a ping.
Maniphest Tasks: T13497
Differential Revision: https://secure.phabricator.com/D21039
Summary: Ref T13490. This largely makes "arc upload" work, although the Future stuff is still a bit wonky. See T11968.
Test Plan: Ran "arc upload README.md", got an upload.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21030
Summary: Ref T13490. This fixes one bug in D21025 (loading relative to "arcanist/" rather than the parent directory) and improves behavior when a library fails to load (we prompt the user to continue).
Test Plan:
- Ran `arc list` with bad library specifications, got a sensible error and an option to continue.
- Ran `arc list` with a library specification that existed next to "arcanist/", got a library load instead of an error.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21029
Summary: Ref T13490. This makes the "phage" toolset work properly and updates external library behavior to match the "classic" behavior, except that "--library" is now supported.
Test Plan: Ran "phage remote" workflows.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21025
Summary:
Ref T13490. There's one simple "implode()" order issue here, and one slightly more complex one that uses "DIRECTORY_SEPARATOR" as glue.
Add test coverage for this, update the lint check to detect constants used as glue, and fix the callsites.
Test Plan:
- Added a failing test and made it pass.
- Ran `arc lint --everything` and looked for remaining implode warnings, found none.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21024
Summary: Ref T13490. Allows "arc upgrade" to run through the new Toolsets infrastructure.
Test Plan: Ran "arc upgrade", although you can't upgrade feature branches so this can't be entirely tested until it hits "master".
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21006
Summary: Depends on D21004. Ref T13490. The "wilds" branch removed a bunch of dead features but some of them were resurrected by the merge. I don't think any of these are desirable to retain, so purge them all again, even in classic mode.
Test Plan: Grepped for affected symbols.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21005
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
