Summary: This function has been deprecated for a long time (see rARC8150fdf044818c503a588c1b3ef0ddbb93cfa1be). It should be safe to remove it now.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9465
Summary: This workflow has been deprecated for a long time now. At some stage, it should be removed.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9463
Summary: Applied various linter fixes. Also make the `.editorconfig` file a bit more specific. Unfortunately, `arc lint --apply-patches` currently modifies some test data that it shouldn't, but this should be fixed after T5105.
Test Plan: Ran `arc unit` to make sure things weren't broken.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9440
Summary: A `version` workflow would be useful, especially for less technical users. Additionally, whenever I am faced with a new command I reasonably expect `$CMD [--help|help]` and `$CMD [--version|version]` to work.
Test Plan: Ran `arc version`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9265
Summary: Ran `arc lint --apply-patches --everything` over rARC, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed //most// of the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9269
Summary:
This is all derped up, and we ignore the error so old installs don't break.
Underp it.
Test Plan: Did `arc land`, verified that the call actually went through successfully.
Reviewers: btrahan, davedash
Reviewed By: davedash
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9271
Summary: `$engine` will be undefined here. This was just copy/paste derp, combined with me accidentally having an `.arclint` file in a place I didn't expect to have one when I tried to test this.
Test Plan: Ran `arc linters` in a totally bare, non-.arclint-having working copy
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9255
Summary: Handles an issue in windows with large code bases not squash merging correctly, this only catches the issue it doesn't really do any recovery.
Test Plan: We have been running this change in our work environment, it's a hard bug to replicate but when it has reared it's head this has caught it.
Reviewers: waynea, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T4884
Differential Revision: https://secure.phabricator.com/D8729
Summary:
Fixes T4952. Several issues:
- You review configuration values with `arc set-config --show`. This makes no sense and never has, I think it just predated `arc get-config` or was easier or something.
- Instead, review values with `arc get-config` and review details with `arc get-config --verbose`.
- Show better and more detailed information about all config sources.
- Establish and show default values from a new "default" source.
- With `--trace` include more information about attempts to read configuration files.
Test Plan:
Ran `arc get-config --trace --verbose` in various working directories and received sensible-looking output.
{F156247}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4952
Differential Revision: https://secure.phabricator.com/D9172
Summary: This string is quite silly!
Test Plan: `arc land --hold` in an affected working copy.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9212
Summary: To me, it seems that these methods should never be overwritten in subclasses.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: LegNeato, aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7959
Conflicts:
src/workflow/ArcanistBaseWorkflow.php
Summary: Fixes T5082. We try to access the repository API in some cases when we don't have one.
Test Plan:
- Made revisions with "arc diff --raw-command --create".
- Made this revision, without "--raw-command".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5082
Differential Revision: https://secure.phabricator.com/D9165
Summary: Fixes T5071. If you run `arc diff --raw` from some arbitrary, non-repository directory we incorrectly try to read information out of the working copy. Even if this information is available, `arc diff --raw` should not assume it's accurate (there's no guarantee the diff comes from the working copy).
Test Plan: Ran `arc diff --raw` in an arbitrary directory.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5071
Differential Revision: https://secure.phabricator.com/D9142
Summary: Ref T4948. Move the `startDocument` code to a `renderPreamble` function so that, at least theoretically, the renderer can be reused. Otherwise, the only way to reuse the renderer would be to construct a new instance.
Test Plan: Ran `arc lint --output xml` and verified that the output looked reasonable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4948
Differential Revision: https://secure.phabricator.com/D9108
Summary: The Checkstyle XML output format is not intended to be an interactive workflow.
Test Plan: Introduced linter issues and ran `arc lint`. Verified that no interactive prompt was shown.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9107
Summary: Fixes T4948. Add a lint renderer which supports outputting the lint results in the Checkstyle XML format.
Test Plan: Ran `arc lint --xml` and inspected the output.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4948
Differential Revision: https://secure.phabricator.com/D9083
Summary: It looks like this command is just hanging if you skim the documentation and miss that you have to echo parameters into it. Print out a hint.
Test Plan: Ran `arc call-conduit` and got a hint.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9050
Summary:
Ref T2039. I'll update the corresponding documentation.
It feels a little awkward that this is disconnected from `getLinterConfigurationOptions()`, but I dislike returning weird ad-hoc structures more than I dislike having two methods. Most linters don't implement either of these anyway.
Test Plan: Ran `arc linters` and `arc linters --verbose`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9062
Summary: Ref T2039. We're starting to get kind of a lot of linters; provide `arc linters` to help users review and understand them and construct `.arclint` files.
Test Plan: {F152205}
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9041
Summary: Contine is not a word methinks.
Test Plan: looks better
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8997
Summary:
Fixes T4938. Now that we have customs statuses, this workflow needs to know about 'em.
One fun caveat is that Conduit isn't up and running to generate the help method, so I had to add a parameter to have the statuses list out. This could maybe be omitted since entering erroneous status options does indeed tell you the correct options.
Test Plan:
`arc close --list-status` -- got a list of statuses
`arc close T1` -- closed T1
`arc close T1` -- error T1 closed already
`arc close T1 -s foobar` -- T1 status set to foobar
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1, T4938
Differential Revision: https://secure.phabricator.com/D8938
Summary: Like git, it is also valid to have empty net change under hg. Should not throw if that is the case.
Test Plan: Tried on a hg repo.
Reviewers: lifeihuang, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8888
Summary:
Fixes T4809. When landing a revision, check for a (non-manual) buildable of the current diff. If we find one, check its status:
- If it passed, print out a message to inform the user that we checked.
- If it failed or is still building, print out details about the issue and require a confirmation to continue.
- Just ignore other cases.
Test Plan:
- Ran `arc land` on a revision with no buildable, a passing buildable, a failed buildable, and a building buildable for the current diff.
- Got sensible output / prompts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8801
Summary: Fixes T4605. Smart waits (see D8782) are presumably good on the balance, but may cause some delays when changes are made to rarely updated repositories. To help mitigate this, have `arc land` hint that a repository has changed.
Test Plan: Will run `arc land`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8783
Summary: make it a real member variable. See rARC77a9c1814063 and D8753#33914.
Test Plan: sending up this diff!
Reviewers: shadowhand, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8761
Summary:
Fixes T4163. Re-word the question so its the same as in "arc diff". Draw the line though and don't automagically do anything if the user says "Y", 'cuz amending / adding files last minute in 'arc land' and other workflows is batshit insane. Assuming infinite growth in the future, I think its best to get this language consistent now.
I changed the shouldAmend member variable to a shouldAmend() function with a static inside. Previously shouldAmend was getting set as a side effect and it was kind of weird. I thought maybe it was written this way because the calls to the vcs are a little slow or something. As such, I figured caching it in the static was a good idea? Didn't seem awful but maybe a premature optimization with whatever the performance reality turns out to be.
Also a modest amount of bonus pht.
Test Plan: this very diff. i'm going to arc land it laters too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4163
Differential Revision: https://secure.phabricator.com/D8753
Summary: Fixes T4291. Also pht user-facing strings.
Test Plan:
made a branch `foo` off master. made a commit and a diff in `foo`. switched backed to master and cowboy committed some thing. went back to branch`foo`, did an arc land, and saw the error message. went back to master, did a git resert --hard HEAD^1, went back to branch `foo`, and then successfully arc landed.
also ran arc help land and things looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4291
Differential Revision: https://secure.phabricator.com/D8738
Summary:
I changed the rendering of the bar color for the `priority` column when running `arc tasks` to match the `priorityColor` property.
If the `priorityColor` property is one of the basic colors already supported by ansi, then the bar is set to that color, otherwise it is set to white.
This will allow the user to customize maniphest priorities and then set their own colors and have those colors display correctly when running `arc tasks`
Fixed some linting errors
Test Plan:
Run `arc tasks` and ensure that:
* the priority column is displayed with a colored bar
* the priority column bar is the correct color (or white if it is an unsupported color)
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8735
Summary: I changed the way `arc tasks` was checking to see if a maniphest task was closed from looking for a non-empty status to looking for the isClosed property submitted in D8731.
Test Plan: Run `arc tasks` and check that the tasks show open/closed correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8732
Summary:
We recently moved our HEAD and it caused some issues on `arc patch` with git-svn repos. The base revision is incorrect and patch will fail. Add the check in such case to make it work.
The check was there before but removed in change b202158. The reason wasn't mentioned there though.
Test Plan: Tried it on svn.
Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8717
Summary: Ref T4670.
Test Plan: arc patch D8685; arc land --hold; verified i got a nice message asking me to be sure i wanted to land epriestley's code
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4670
Differential Revision: https://secure.phabricator.com/D8709
Summary: Remove any-author flag from 'arc which' make any-author be the default behavior. Annotate revision with the owners username.
Test Plan:
Apply a patch that you don't own (arc patch Dxxx) run 'arc which' verify that:
1. You see the revision
2. You see the original authors username next to the revision (owned by sjobs) etc.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8690
Summary:
Fixes T4596. I misunderstood this issue and D8512 was not correct. Specifically:
- The `hg log` needs to be escaped, since otherwise "arcpatch-x" is interpreted as a revset.
- The `hg update` does not need to be escaped, since updating to a revset doesn't make sense and the command never treats its argument as a revset.
- The `hg bookmark` does not need to be escaped, for similar reasons.
Test Plan:
- Ran these commands in isolation and got sensible, consistent results.
- Ran `arc patch` several times in a row and got proper bookmark names.
Reviewers: btrahan, durham, rvanvelzen
Reviewed By: rvanvelzen
Subscribers: epriestley
Maniphest Tasks: T4596
Differential Revision: https://secure.phabricator.com/D8661
Summary: Fixes T4649. The issue in that task is caused because we're submitting a block of text including comments. We've probably been doing this for a long time, but maybe were more liberal in parsing before. Instead, strip them.
Test Plan: Ran `arc diff --verbatim` and got "Dxxx" prefilled correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4649
Differential Revision: https://secure.phabricator.com/D8658
Summary:
see https://github.com/facebook/phabricator/issues/546 - arc complete blows up when not
in a workdir.
There's no "is ArcanistWorkingCopyIdentity object valid" method, but getVCSType() looks like the
closest match.
git grep for `getProjectRoot` didn't reveal any more problmatic call sites.
Test Plan: `arc [tab] [tab]`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8578
Summary:
Fixes T4063. The `git format-patch` command produces a special header
and footer which we need to detect, strip, and parse.
Test Plan:
- Added and ran unit tests.
- Submitted a diff with `git format-patch HEAD^ --stdout | arc diff --raw`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4063
Differential Revision: https://secure.phabricator.com/D8547
Summary:
Fixes T4603. We fire `arc close-revision --finalize` implicitly from `arc land`, which may close a corresponding Differential revision.
We want to close if the repository is not present in Phabricator (i.e., we'll never be able to close in response to the commit message, since we'll never see it). Historically, we used Arcanist Project -> "Tracked" to make this determination. Instead, just check if the working copy is associated with a repository. This is simpler, easier, and works better.
Test Plan: Ran `arc close-revision --finalize`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4603
Differential Revision: https://secure.phabricator.com/D8523
Summary:
Ref T4603. This workflow predates `arc land` and doesn't make much sense in modern Phabricator/Arcanist. It is surprising that `arc amend` will sometimes close accepted revisions, and we're better at detecting that repositories are tracked, and tracking repositories is easier.
Also fix some inaccuracies and old claims in the documenation and help.
Test Plan: Ran `arc amend`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4603
Differential Revision: https://secure.phabricator.com/D8522
Summary: Fixes T4596. I couldn't immediately reproduce this, but the `hgsprintf()` version is clearly more correct.
Test Plan: Used `arc patch --trace` to examine hg commands.
Reviewers: btrahan, durham
Reviewed By: durham
Subscribers: aran, epriestley
Maniphest Tasks: T4596
Differential Revision: https://secure.phabricator.com/D8512
Summary:
We recently tried to advance the arcanist HEAD in our release branch but failed, due to an exception in SVN pre-commit hook like this:
abort: Commit blocked by pre-commit hook (exit code 1) with output:
LINT 1.3s FacebookWebJSLintLinter (1 file)
Exception
Some linters failed:
- FacebookWebCopyrightLinter: BadMethodCallException: Call to a member function getConfigFromAnySource() on a non-object
(Run with --trace for a full exception trace.)
However `arc lint` works just fine. By searching the change history, it looks related to a few commits D7271, D7377, D7382, especially D7377, where configuration manager is added to the lint engine. Add it to the SVN precommit hook workflow too.
Test Plan: I am not quite sure how to test it out easily. Any suggestions?
Reviewers: lifeihuang, JoelB, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aran, epriestley, mikemag, Korvin
Differential Revision: https://secure.phabricator.com/D8492
Summary:
pretty straight-forward stuff here. Note that in other events we use "fields" or "specification" rather than "revision"; I think "revision" is best particularly in this context where it is in fact the revision being landed.
Fixes T4565.
Test Plan: php -l
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Maniphest Tasks: T4565
Differential Revision: https://secure.phabricator.com/D8484
Summary: D7271 added this line but the signature of function ArcanistWorkingCopyIdentity::newFromRootAndConfigFile(...) was never changed. I have no idea why it was added here and it's causing failures in facebook. I am just removing it now.
Test Plan: No idea how to test
Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8432
Summary:
Do a little cleanup:
- Remove copyright header (we removed all of these a long time ago, this one just snuck through somehow).
- Remove `@group` comment (obsolete with new Diviner).
- Note support for all VCSes.
- Add pht() for translation.
- Hint `arc browse .`.
- Fail on no paths sooner.
- Raise a useful error if we can't figure out which repository we're heading to.
- Clarify "open" comment.
- Use `Filesystem::binaryExists()`.
- Some minor wordsmithing.
Test Plan: `arc browse`, `arc browse .`, `arc browse README`, `arc browse README src`, ran `arc browse` in valid working copy with no associated repo.
Reviewers: btrahan, spicyj
Reviewed By: spicyj
CC: aran
Differential Revision: https://secure.phabricator.com/D8176
Test Plan: Ran 'arc browse' in a repo that contains a .arcconfig and one without.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8173
