Summary:
Ref T5955. This logic is a little cleaner than the previous version.
Don't require `~/.arcrc` to exist if the caller provides `--conduit-token`.
Test Plan:
- Made calls with `--conduit-token` and no `~/.arcrc`.
- Made calls with `--conduit-token` and a normal `~/.arcrc`.
- Made calls with normal `arc`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D12750
Summary: Ref T5955. This makes it easier to write scripts which call Conduit via `arc call-conduit`.
Test Plan: Used `arc --conduit-token ... call-conduit user.whoami` to make calls as various users.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D12717
Summary: Remove the `ArcanistUncommittedChangesException` class which is unused after D11843.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12515
Summary: Although these don't do any harm, they show up in my editor which is configured to highlight trailing whitespace.
Test Plan: Submitted this diff... saw no trailing whitespace.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12369
Summary:
Fixes T7521. This separates things into two prompts and doesn't try to automatically add untracked files.
This also fixes T7512, or at least I can't reproduce it after this change.
Test Plan:
- Ran `arc diff` in a `git` directory with untracked files.
- Ran `arc diff` in a `svn` directory with untracked files.
- Ran `arc diff` in a `hg` directory with untracked files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7512, T7521
Differential Revision: https://secure.phabricator.com/D12258
Summary:
Ref T7594. Currently, if a chunk upload fails, we incorrectly swallow the failure and fall back to single-file upload, which will often fail by hitting size limits. This also silences the original error.
Instead, do chunk uploads outside the block so that any exceptions escape, and we don't try to fall back to single-file upload.
Mostly just trying to get more info about what's going wrong on @joshuaspence's install.
Test Plan: Faked an exception in chunk upload, ran `arc upload` on a big file, saw the exception displayed on the console.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, joshuaspence
Maniphest Tasks: T7594
Differential Revision: https://secure.phabricator.com/D12111
Summary: Ref T7149. Make sure we sit at "Resuming: 60%" or whatever while uploading the first chunk.
Test Plan: Ran `arc upload` on a large file, cancelled it, resumed it, got sensible progress bar.
Reviewers: chad, btrahan
Reviewed By: chad, btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12082
Summary: Ref T7149. This was just for testing and is no longer required.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12077
Summary: Ref T7149. This makes the client try to use the new `file.allocate` API before falling back to the old stuff.
Test Plan: Used `arc upload` to upload files. With chunking forced, uploaded chunked files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12061
Summary:
Refs D11990. When using `arc diff` with untracked files in the working
copy, add the untracked file(s) to the commit (as they weren't stashed or
ignored). Add the untracked paths to the list of changes in the editor
template, indicating that the files were added to the commit.
This doesn't add a separate prompt to add untracked files as per the
behaviour prior to D11843.
Test Plan:
Ran `arc diff` with only untracked files, answered yes to the 'create
new commit' prompt. Saw the commit-message with the updated changes
including untracked files. Completed the arc template, and got commit
containing uncommitted, unstaged and untracked files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11995
Summary: Fixes T7465. I think I just missed this when untangling the old logic.
Test Plan: Ran `arc diff` with //only// untrakced files, saw warning.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7465
Differential Revision: https://secure.phabricator.com/D11990
Summary:
We end up with one too few newline here in some workflows, like `arc land` with unstaged changes.
Root issue here is that `phutil_console_prompt|confirm` lead with too much whitespace but that's a harder fix.
Test Plan: Saw reasonable whitespace.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11927
Summary: This is supposed to just print out the base revision, but actually prints out the repository section first.
Test Plan: Ran `arc which`, `arc which --show-base`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11888
Summary:
Fixes T7344.
Currently, we use `phutil_console_prompt()`, which isn't a very good editor. Use the real $EDITOR instead.
100% of the logic here was also a gigantic mess. Clean it up.
Test Plan: Will update in a second with console output from this run.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7344
Differential Revision: https://secure.phabricator.com/D11843
Summary: Fixes T7113. This one was a bit trickier than others as the API output changed a bit. In particular, there is no "errors" emitted so much as the result set just doesn't include the answer if things are garbage. Ergo, check the "identifier map" to either check for diff existence or to lookup the phid to grab the actual diff data from the "data" part of the result.
Test Plan: called `arc backout D11665` and got some working output...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7113
Differential Revision: https://secure.phabricator.com/D11667
Summary: Fixes T7112. Nothing too difficult here.
Test Plan:
meta - submitting this with the new arcanist code
used conduit API to verify the difference between getdiff (just the latest diff) and querydiffs (all diffs that match, with the latest diff first in the set)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7112
Differential Revision: https://secure.phabricator.com/D11665
Summary:
Some commands (like `get-config`) do not require a working copy at all. Recent changes prevented these commands from running outside a VCS working copy.
Let `null` mean "no working copy".
See also <https://github.com/phacility/phabricator/issues/800>.
Test Plan:
- Ran `arc get-config` from outside a working copy.
- Ran `arc list` from outside a working copy, got an error.
- Ran `arc commit` in a Git working copy, got an error.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11643
Summary: Explicitly declare that the `arc branch` command is only supported under `git`.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11552
Summary: This workflow only works on git + mercurial.
Test Plan: I don't even subversion.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11626
Summary: This workflow does not work on subversion.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11625
Summary: It doesn't make any sense to run this command on another VCS.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11624
Summary: It doesn't make any sense to run this command on any other VCS.
Test Plan: Ran `arc svn-hook-pre-commit` and hit the usage exception.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11623
Summary: Instead of having an `ArcanistWorkflow` subclass explicitly throw an exception when run in an unsupported VCS, consolidate this code and move it to `arcanist.php`. In doing so, we lose some specificity in some of the error messages, but this otherwise feels cleaner. We could consider adding a `getUnsupportedRevisionControlSystemMessage()` method to provide a more tailored error message. Depends on D11604.
Test Plan:
Ran `arc bookmark` in a `git` working copy:
```
Usage Exception: `arc bookmark` is only supported under hg.
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11550
Summary: Using `array('any')` to represent `array('git', 'hg', 'svn')` is a bit magical and leads to a lot of special-casing.
Test Plan: Verified that tab completion (ala `ArcanistShellCompleteWorkflow`) still worked.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11604
Summary: Fixes T2461. Similarly to `arc lint --everything`, `arc unit --everything` should work without a base commit.
Test Plan: Removed the `.git/arc/default-relative-commit` file and ran `arc unit --everything`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T2461
Differential Revision: https://secure.phabricator.com/D11459
Summary: Allow `--severity=warning` to mean the same as `--severity warning`. Longer term, we should convert this code to use `PhutilArgumentParser`, although it doesn't seem that `PhutilArgumentParser` support British spelling ;)
Test Plan: Ran `arc lint --severity=warning`. Also `var_dump`ed `$args` to make sure it looked reasonable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11464
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `ArcanistShellCompleteWorkflow` class.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11238
Summary: D7952 added namespaces to `ArcanistUnitTestResult` but these are not exported and thus don't get sent from client to server.
Test Plan: Uploaded a diff and inspected the contents of the `phabricator_differential.differential_diffproperty` table.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11208
Summary: Currently we output "Waiting for JSON parameters on stdin...", even if `stdin` is piped (for example, from `echo`).
Test Plan:
```lang=bash
> echo '{}' | arc call-conduit conduit.ping
{"error":null,"errorMessage":null,"response":"ip-10-161-81-110"}
> arc call-conduit conduit.ping
Waiting for JSON parameters on stdin...
{}
^D
{"error":null,"errorMessage":null,"response":"ip-10-161-81-110"}
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11122
Summary: The `file.uploadhash` method was added a long time ago (in D4899). It should be safe to assume that this method exists on most installs.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11118
Summary: Ref T5112. The `arc inlines` workflow no longer works because the `differential.getrevisioncomments` API method has been deprecated (see T2222). The command is basically useless right now.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5112
Differential Revision: https://secure.phabricator.com/D9464
Summary:
Ref T5955. If the server supports token-based authentication, prefer it over certificate-based authentication.
Also fixes T3117.
Test Plan:
- Used `arc install-certificate` to install credentials from both token-based and certificate-based hosts.
- Used `arc list` with a token-based host.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3117, T2878, T5955
Differential Revision: https://secure.phabricator.com/D10988
Summary: the call to setHeadCommit() was accidentally placed in a `if ($background) {}` block, which was removed in 54bea94. This adds the removed call.
Test Plan: used --head, worked as expected
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10447
Summary:
Calling `arc patch` on a diff that's dependent on a different diff tries to patch the parent first.
To patch the parent a child workflow is created, and a conduit is passed down, but the credentials are not and it is not marked as authenticated.
Then when the child tries to get the commit message for the dependency, it checks isConduitAuthenticated() https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/ArcanistPatchWorkflow.php;2c3268f03ed70d3221eb1642bIc99ebb39b12902e$800 and on failure pops up an interactive editor for the commit message.
Instead we just pass down the credentials to the childred and mark them as authenticated, so this is not a problem.
Test Plan: With two diffs where DA2 depends on DA1, run `arc patch --force --nobranch DA2` ... this no longer pops an interactive editor for the commit message for the dependency.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, seshness, epriestley, Korvin
Maniphest Tasks: T5986
Differential Revision: https://secure.phabricator.com/D10381
Summary: Ref T5971. We lose validation of the line ranges, but I don't think that's a huge issue.
Test Plan: Ran `arc browse README`, `arc browse README:3`, and `arc browse README:3,6-7`
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5971
Differential Revision: https://secure.phabricator.com/D10352
Summary: Ref T5781. This was broken in some earlier changes in T5781.
Test Plan: Ran `arc browse README`, `arc browse README:10`.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5781
Differential Revision: https://secure.phabricator.com/D10346
Summary: Fixes T3813. This error message isn't very helpful in SVN; be more helpful.
Test Plan: Ran `arc land` in a Subversion repository, got a reasonable error message.
Reviewers: btrahan, asherkin
Reviewed By: asherkin
Subscribers: aran
Maniphest Tasks: T3813
Differential Revision: https://secure.phabricator.com/D7275
Summary: Ref T5926. We only pass Phabricator an update hint from `arc land`, not from `arc commit`.
Test Plan:
- Ran `arc land` and `arc commit` with `--trace`, saw hints go over the wire.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5926
Differential Revision: https://secure.phabricator.com/D10324
Summary:
Ref T4281. A long time ago, we added a `--background` flag to let `arc lint` and `arc unit` run while you're typing a commit message, in some situations.
This code is only moderately beneficial and is way too complicated. Particularly, it has a long history of causing hangs (T4281, T2463), doesn't work on Windows, and is impossible to debug.
It's also running into a serious PHP bug with EAGAIN/EPIPE being indistinguishable that I haven't been able to find a reasonable workaround for in ~3-4 hours of trying.
All the pathways forward that I can see make this already-complex system more complex.
The major reason that this stuff is so complex is that the subprocess may need to prompt the user (notably, to apply patches from lint).
Instead, I'm going to simplify how `arc diff` interacts with `arc lint` and `arc unit`, so we can just fire-and-forget a background process, let it do as much work as it can without needing user input, and then pick up wherever it left off. This will be slightly less cool/magical, but it won't hang bizarrely and I will be able to debug it.
For now, simply remove the `--background` flag and behavior so `arc` works for everyone.
Test Plan: Ran `arc diff` to create this diff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4281
Differential Revision: https://secure.phabricator.com/D10198
Summary: Ref T5781. `git show .` works like HEAD, but that isn't what `arc browse .` means.
Test Plan: Ran `arc browse .` with a repository at a published commit.
Reviewers: chad, btrahan, avive, avivey
Reviewed By: avivey
Subscribers: epriestley, avivey
Maniphest Tasks: T5781
Differential Revision: https://secure.phabricator.com/D10197
Summary: Ref T5781. This makes things like `arc browse master` work (but they open the commit, not a revision).
Test Plan: Ran `arc browse master`.
Reviewers: csilvers, btrahan
Reviewed By: btrahan
Subscribers: epriestley, spicyj
Maniphest Tasks: T5781
Differential Revision: https://secure.phabricator.com/D10143
Summary: Ref T5781. Add a flag to optionally open a web browser after creating a diff or revision.
Test Plan: Created //this revision// with `arc diff --browse` // !!! //
Reviewers: csilvers, btrahan
Reviewed By: btrahan
Subscribers: epriestley, spicyj
Maniphest Tasks: T5781
Differential Revision: https://secure.phabricator.com/D10141
Summary: Ref T5781. This implements `arc browse T234`, for any monogrammed object.
Test Plan:
- Ran `arc browse T234` outside of a working copy (does not require repository API).
- Ran `arc browse T234` in a working copy, got task.
- Ran `arc browse PATH` in a working copy.
- Ran `arc browse nonpath` with and without `--force`.
Reviewers: csilvers, btrahan
Reviewed By: btrahan
Subscribers: epriestley, spicyj
Maniphest Tasks: T5781
Differential Revision: https://secure.phabricator.com/D10140
Summary:
See T5690.
-arc land now respects tracked branch when choosing 'onto'; '--onto' option remains as an override.
-arc land now respects tracked branch when choosing a remote; the remote is taken from 'onto's upstream unless the '--remote' option is present; when 'onto' branch has no upstream the '--remote' option must be provided as before.
Since 'arc feature' branches are (already) created as tracking branches, 'arc.land.onto.default' if present, is only used as the default when a non-tracking branch created by some other means is landed with out explicit '--onto'. This may be surprising but is probably the correct go-forward behavior and is inline with the description in T5690.
Test Plan:
-checked having no arc.land.onto.default still assumes 'master'
-checked 'arc.land.onto.default' still overrides 'master'
-checked upstream branch (of feature branch) overrides 'master' and 'arc.land.onto.default'
-checked '--onto' overrides all
-checked origin is default for non-tracking branches
-checked the land onto branch's upstream remote is used instead of 'origin'
-checked '--remote' overrides 'origin' and the tracked upstream
-tested several crazy branch names including 'something/like/this' for both the upstream and tracking branches
-tested on linux and OS X
-rinse and repeat on Windows
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5690
Differential Revision: https://secure.phabricator.com/D10058
Summary:
Ref T5655. It is superfluous to include "base" in the name of an abstract base class. Furthermore, it is not done consistently within the code base.
In order to retain compatibility with external code, I have kept the `ArcanistBaseWorkflow` class (which trivially extends from `ArcanistWorkflow`), but it is now deprecated and should output a warning message. Similarly for `ArcanistBaseUnitTestEngine`.
Test Plan: Created a workflow which extends from `ArcanistBaseWorkflow`. Executed the workflow and saw a deprecation warning.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, aurelijus
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9983
Summary: We can remove this "Stopped" code as per feedback on D7327. I think having the full text of "In Progress" and "Suspended" is much clearer than just a '*' though, so I've only removed the ongoing check.
Test Plan: Ran a combination of `arc start`, `arc stop` and `arc time` and it all worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9955
Summary:
Depends on D9906.
This adds `arc start`, `arc stop` and `arc tracking` for tracking tasks, diffs and other objects in Phrequent.
Test Plan: Tested this against a local install.
Reviewers: skyronic, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: maxhodak, hach-que, aran, epriestley, Korvin
Maniphest Tasks: T3569, T3969
Differential Revision: https://secure.phabricator.com/D7327
Summary: Fixes T4418. Allows Maniphests created through the `arc todo` workflow to have projects assigned.
Test Plan:
```
> ./bin/arc --trace --conduit-uri='http://phabricator.joshuaspence.com' todo "Test project" --project foo --project bar
libphutil loaded from '/home/joshua/workspace/github.com/phacility/libphutil/src'.
arcanist loaded from '/home/joshua/workspace/github.com/phacility/arcanist/src'.
Config: Reading user configuration file "/home/joshua/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/home/joshua/workspace/github.com/phacility/arcanist/.arcconfig".
Working Copy: Path "/home/joshua/workspace/github.com/phacility/arcanist" is part of `git` working copy "/home/joshua/workspace/github.com/phacility/arcanist".
Working Copy: Project root is at "/home/joshua/workspace/github.com/phacility/arcanist".
Config: Did not find local configuration at "/home/joshua/workspace/github.com/phacility/arcanist/.git/arc/config".
Loading phutil library from '/home/joshua/workspace/github.com/phacility/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 618>
>>> [1] <http> http://phabricator.joshuaspence.com/api/conduit.connect
<<< [1] <http> 1,050,487 us
<<< [0] <conduit> 1,051,585 us
>>> [2] <conduit> project.query() <bytes = 199>
>>> [3] <http> http://phabricator.joshuaspence.com/api/project.query
<<< [3] <http> 294,584 us
<<< [2] <conduit> 294,986 us
>>> [4] <conduit> maniphest.createtask() <bytes = 313>
>>> [5] <http> http://phabricator.joshuaspence.com/api/maniphest.createtask
<<< [5] <http> 637,693 us
<<< [4] <conduit> 638,098 us
Created task T6: 'Test project' at http://phabricator.joshuaspence.com/T6
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4418
Differential Revision: https://secure.phabricator.com/D9457
Summary: I'm pretty sure that `@group` annotations are useless now... I believe that they were originally used by Diviner?
Test Plan: Eye-balled it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9855
Summary: Fixes T5442. `arc lint --everything` currently uses a `FileFinder` to discover paths for linting. A consequence of this is that files that are ignored or otherwise excluded from version control are linted.
Test Plan: Ran `arc lint --everything` in rPHU and noticed that I wasn't prompted to add a trailing newline to `src/.phutil_module_cache`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5442
Differential Revision: https://secure.phabricator.com/D9674
Summary:
In D9595, we stopped parsing short-form "Differential Revision:" fields in commit messages, and only accept URLs.
I have one of the older style commit messages in my local `arcanist/`, so now we go down this parse failure branch in `arc branch`. This has never worked quite correctly, and if the parse fails we end up with a bad branch dictionary that is missing fields.
Test Plan: Ran `arc branch` in a working copy with an old `Differential Revision:` field at the head of a branch. Before patch: explosions; after patch: works great.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9620
Summary: The output from `arc help --full` is missing a newline character.
Test Plan:
**Before**
```
> arc help --full
--skip-arcconfig
Skip the working copy configuration file
--arcrc-file filename
Use provided file instead of ~/.arcrc.>
```
**After**
```
> arc help --full
--skip-arcconfig
Skip the working copy configuration file
--arcrc-file filename
Use provided file instead of ~/.arcrc.
>
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9608
Summary: `PhutilConsoleTable` does a better job at aligning columns. We still probably need to do some work to `PhutilConsoleTable` to set a maximum width for a specified column, or elect which columns can be truncated etc but this can probably come later.
Test Plan: {F167687}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9601
Summary:
This is useful for wrapper scripts that want to customize arcanist's behavior without affecting the global configuration.
This can be implemented with arcanist_configuration entry in .arcconfig, however it is currently limited to
per-project settings, and this feature makes writing wrapper scripts a little easier.
Test Plan: arc diff --set-config editor=vim (yeah yeah, crappy test case)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9442
Summary: I can't find an analogous flag for hg.
Test Plan: I don't have any precommit hooks set up on my machine so I can't test this easily, but I think this seems pretty harmless?
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9523
Summary:
Introducing `--head` caused us to run `git diff base..head` explicitly.
However, we can now hit this workflow:
- We resolve `HEAD` as commit `aaaa1`.
- This is cached.
- We notice dirty working copy changes and prompt the user to amend them to HEAD.
- The user accepts the amend.
- We amend, creating commit `bbbb2`.
- We dirty the commit range and reload the working copy. This //does not// dirty the cache of HEAD.
- We run `git diff`, but it uses the old cached HEAD: `git diff base..aaaa1`.
- This works fine (`aaaa1` still exists, it's just not on any branch) but produces the wrong diff (without amended changes).
To resolve this, implement the "dirty the cache when the range reloads" hook.
Also never try to amend if the user provides `--head`.
Test Plan:
Ran `arc diff --only --trace` in a working copy with a new commit and some uncommitted changes.
- Prior to this change, saw a `git diff base..aaaa1` command and the wrong diff.
- After this change, saw a `git diff base..bbbb2` command and the correct diff.
Reviewers: chad, csilvers, talshiri
Reviewed By: talshiri
Subscribers: epriestley, spicyj
Differential Revision: https://secure.phabricator.com/D9506
Summary:
This adds support for passing range of commits for arc diff. This is useful when you want to submit code reviews for past commits without mucking around with the working copy.
This will probably require changes :)
Test Plan: Tested locally, but totally need to add tests for this
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9369
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
Summary: Fixes T3929. If arc is merging it will say merging * into * vs always saying rebasing * onto *
Test Plan: Make arc do a rebase, then a merge
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3929
Differential Revision: https://secure.phabricator.com/D8085
Summary:
- The modern name for the config is "project.name".
- Missing parameter in a pht().
- When the value is set, but not valid, we gave you a misleading error message.
Test Plan: Ran `arc which`.
Reviewers: talshiri, btrahan
Reviewed By: talshiri
CC: aran
Differential Revision: https://secure.phabricator.com/D8081
Summary:
Ref T4343. Continues the process of reducing the prominence of Arcanist Projects. Primarily:
- Query Phabricator to identify the working copy based on explicit configuration, or guess based on heuristics.
- Enhance `arc which` to explain the process to the user.
- The `project_id` key is no longer required in `.arcconfig`.
Minor/cleanup changes:
- Rename `project_id` to `project.name` (consistency, clarity).
- Rename `conduit_uri` to `phabricator.uri` (consistency, clairty).
- These both need documentation updates.
- Add `repository.callsign` to explicitly bind to a repository.
- Updated `.arcconfig` for the new values.
- Fix a unit test which broke a while ago when we fixed a rare definition of "unstaged".
- Make `getRepositoryUUID()` generic so we can get rid of one `instanceof`.
Test Plan:
- Ran `arc which`.
- Ran `arc diff`.
- This doesn't really change anything, so the only real risk is version compatibility breaks. This //does// introduce such a break, but the window is very narrow: if you upgrade `arc` after this commit, and try to diff against a Phabricator which was updated after yesterday (D8068) but before D8072 lands, the lookup will work so we'll add `repositoryPHID` to the `differential.creatediff` call, but it won't exist in Phabricator yet. This window is so narrow that I'm not going to try to fix it, as I'd guess there is a significant chance that no users will be affected. I don't see a clever way to fix it that doesn't involve a lot of work, either.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4343
Differential Revision: https://secure.phabricator.com/D8073
Summary: I'm going to deprecate `user.find`, `user.query` is more modern/powerful and obsoletes it.
Test Plan: Ran `arc tasks --owner epriestley`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8070
Summary: This might not be universally desireable, but I found myself writing an additional linter (which I had called `WhitespaceTextLinter`) for the sake of these two linter tests. I figured it may be of use upstream, and so I decided to submit it as a diff. I won't be offended if it is rejected however.
Test Plan: `arc lint` and `arc unit` are both okay with it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7957
Summary: See <https://github.com/facebook/arcanist/issues/133>. Treat "!" files like "C" and "?" files and make the user deal with them.
Test Plan:
>>> orbital ~/repos/INIS $ svn st
! README
>>> orbital ~/repos/INIS $ arc diff
Usage Exception: You have missing files in this working copy. Revert or formally remove them (with `svn rm`) before proceeding.
Working copy: /INSECURE/repos/INIS/
Missing files in working copy:
README
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7886
Summary:
`arc set-config --show` only show the user config
It would be better to contain local/global/system config
Test Plan: set config by local/global/system/user/project, and check the result of `arc set-config --show`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D7851
Summary:
The current format of arc feature is not very friendly to parsing. This adds a json output
format.
Test Plan: Run the command and ensure output is valid
Reviewers: lifeihuang, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin
Differential Revision: https://secure.phabricator.com/D7647
Summary: allow specifing an replacement .arcrc file, for the odd cases.
Test Plan: `echo {} | arc call-conduit user.whoami` with and without `--arcrc-file=`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7208
Summary:
Lingers on from D7271; Rename `ArcanistWorkingCopyIdentity.getConfig()`.
Changed all linters (Except one) to use `getConfigFromAnySource()`, because it seems to make sense.
Test Plan: arc unit --everything; arc lint in github.com:epriestley/arclint-examples.git (Except for phpcs, flake8, cpplint and csslint which I don't have installed).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7382
Summary:
somewhat related to D7271 and D7377.
Not actually broken right now, but might be worth it for completeness.
Test Plan: arc unit invokes PHPUnit; Can't test Csharp/XUnit on my end.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: aurelijus, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7381
Summary:
Create a new class for them, pass instance around as need.
This looks like it's mostly working, but I'd like to replace the various `new ArcanistConfigurationManager()`
calls with something more suitable.
And maybe get a better name for ArcanistConfigurationManager ("Configuration" is already taken).
Test Plan: arc unit --everything, and then some.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7271
Summary: Nice title. Ref T479.
Test Plan:
Actually, help on that? I want to make sure I properly build up the "depends" on data. Is it as simple as
-- observe at some commit hash RAZZMATAZZ
-- git checkout -B "foo"
-- <work>
-- git commit -m "stash"
-- arc diff -> yields DX
-- git checkout -B "foo_prime"
-- <work>
-- git commit -m "stash"
-- arc diff -> yield DY
-- git checkout RAZZMATAZZ
-- arc patch DY
-- get prompted in workflow, agree
-- git log and observe DX and DY applied
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, davidressman
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D6790
Summary:
If in a subdirectory, any changes made in a different location
is missed from the patch with no errors from git. The other
option was to run some logic to compare the files being changed.
Test Plan:
Run arc patch from a subdirectory that would miss some files
previously.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3733
Differential Revision: https://secure.phabricator.com/D7167
Summary:
The existing lint configurations do not allow for linting only the lines that have
changed when paths are added. --lintall has a default behavior of true when paths are specified,
and false when paths are not specified. Because of this (and because it does not take a boolean
param) it is not possible to lint a path for only the errors on changed lines - only-new is not
working presently.
Test Plan: play around with the linter
Reviewers: lifeihuang
Reviewed By: lifeihuang
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D7055
Summary:
Fixes T3856.
Detailed code coverage iterates over all of the files with changes in the current repository, however the code coverage tool might not generate a report for all changed files in the repository, and this would result in an undefined index error.
As an additional bonus, since changes to binary files won't be reported by code coverage tools, this also prevents binary data from being outputted to the console.
Test Plan:
Change a binary file in a repository and run a code coverage tool. The binary file should be reported as 0% code coverage, but the file contents should not be rendered to the console.
Change a code file that is not covered by a code coverage tool and run code coverage. The file should be reported as 0% code coverage, and the file contents should not be displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3856
Differential Revision: https://secure.phabricator.com/D7051
Test Plan: Saw 'Do you want to add these files to the commit?'
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6991
Summary:
Make sure on failure (restoreBranch()) we call `git submodule update --init --recursive` to handle all those purdy submodules. For the pushing step, wrap the push commands in the try / catch block so everything gets cleaned up nice if there's failure. BONUS - add --recursive to arc patch workflow to so nested submodules work correctly. (Crazy git users)
Fixes T3407, T2945.
Test Plan: I wasn't sure how to simulate a good "push" failure but I think this should work.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2945, T3407
Differential Revision: https://secure.phabricator.com/D6885
Summary:
When running arc diff in a repository that supports commit ranges, it is
possible that the setting for the default relative commit hasn't been set.
If this is the case, the user will be prompted. This change makes sure that
the prompt happens (and thus the setting is set) before we run the
background lint and unit runs.
Test Plan:
```
rm .git/arc/default-relative-commit
arc diff
```
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2351
Differential Revision: https://secure.phabricator.com/D6854
Summary:
Ref T3776, Ref T479. Say you have some DN, with a submodule X@Y. Later, X@Z in your working copy / repo. If you run arc patch DN, you'd end up with a dirty working copy claiming that X@Z was wrong and it should be X@Y.
To fix, basically run 'submodule init' and 'submodule update'. This makes it so after "arc patch" if you run "git status" it looks clean.
Gross part though now is if you then "git checkout master" you'll have a dirty checkout the other way. I think this is better though.
Test Plan: made a new repository where I added libphutil @ X, did some work (DX), then made libphutil @ y. When I arc patch'd DX, things looked good!
Reviewers: epriestley
Reviewed By: epriestley
CC: csilvers, Korvin, aran
Maniphest Tasks: T479, T3776
Differential Revision: https://secure.phabricator.com/D6837
Summary:
Currently, we prompt the user to mark non-UTF8 files as binary, but don't actually attach the data to the change when they do. This means we don't upload the data, and can't patch it later.
A simple reproduction case is to build a test file (I used one with bytes from 1..255):
$ # Don't include \0, since Git treats that specially.
$ ./echo_every_byte_from_1_to_255_inclusive.erl > example.txt
Then add it:
$ git add example.txt
$ git commit -a -m derp
$ arc diff --only HEAD^
You'll be prompted to convert the file to binary:
Do you want to mark this file as binary and continue? [Y/n] y
Before this patch, that would be followed by:
Uploading 0 files...
...which is incorrect; we need to upload the new data. After this patch, this shows:
Uploading 1 files...
...which is also incorrect, but only grammatically. Diffs created after this patch apply back cleanly with `arc patch` and restore the file properly.
Test Plan: Followed instructions above, restoring a textual binary conversion by using `arc patch`.
Reviewers: zeeg, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6815
Summary: Fixes T3696. Currently, we abort. If stdin is not a TTY, we should just continue. A script which cares could conceivably run `arc lint` and `arc unit` separately, but it seems unlikely that any script would ever want to fail here.
Test Plan: Ran `echo -n '' | arc diff --create --verbatim` with a lint error and got a revision (D6720).
Reviewers: Firehed, btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3696
Differential Revision: https://secure.phabricator.com/D6721
Summary:
Ref T2039. That task has a bunch of discussion, but basically we do a poor job of serving the midrange of lint configuration right now.
If you have something simple, the default linters work.
If you have something complex, building your own engine lets you do whatever you want.
But many users want something in between, which isn't really well accommodated. The idea is to let you write a `.arclint` file, which looks something like this:
{
"linters" : {
"css" : {
"type" : "csslint",
"include" : "(\.css$)",
"exclude" : "(^externals/)",
"bin" : "/usr/local/bin/csslint"
},
"js" : {
"type" : "jshint",
"include" : "(\.js$)",
"exclude" : "(^externals/)",
"bin" : "support/bin/jshint",
"interpreter" : "/usr/local/bin/node"
}
}
}
...which will provide a bunch of common options around lint severity, interpreter and binary locaitons, included and excluded files, etc.
This implements some basics, and very rough support in the Filename linter.
Test Plan:
Generated a `.arclint` file and saw it apply filename lint correctly. Used `debug` mode and tried invalid regexps.
{
"debug" : true,
"linters" : {
"filename" : {
"type" : "filename",
"exclude" : ["@^externals/@"]
}
}
}
Next steps include:
- Provide an external linter archetype (T3186) and expose a common set of configuration here ("bin", "interpreter", "flags", "severity").
- Provide a `.arcunit` file which works similarly (it can probably be simpler).
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D6797
Summary:
arc for mercurial on windows was broken in several way.
Executing a command via passthru failed because passthru on windows
skips the shell so 'set HGPLAIN=1 & ...' was an invalid command. The
fix was to just not set HGPLAIN for passthru commands on windows.
Also removed hardcoded '' quotes in mercurial commands since windows
doesn't support single quots.
Test Plan: arc land --hold on a windows machine
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6763
Summary: Corrects relative vs absolute branch name when using 'arc commit'
Test Plan: 'arc commit' on a release branch gave an error before making the change (change was generated from 'branches/yyy' but working copy root is 'https://xxx/branches/yyy', now it does not.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6743
Summary: My recent change adding --everything to arc lint could sometimes cause a "diff is empty" error, this patch fixes it.
Test Plan: Ran "arc lint --everything" before and after patch. No longer errors out. Only appeared to originally happen when there were uncommited changes in an svn repo.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6732
Summary:
When trying to find an unused bookmark we append hyphened suffixes to the end of the bookmark name. In Mercurial, these are treated the same as the bookmark name without the suffix, here's the output from hg log -r:
```
[mehdi] HGPLAIN=1 hg log -r "arcpatch-1"
abort: unknown revision 'arcpatch'!
[mehdi] HGPLAIN=1 hg log -r "arcpatch_1"
abort: unknown revision 'arcpatch_1'!
```
Test Plan: None.
Reviewers: epriestley, dschleimer
Reviewed By: dschleimer
CC: aran, Korvin, DurhamGoode, dschleimer
Differential Revision: https://secure.phabricator.com/D6698
Summary:
If you are trying to commit someone else's diff, arc commit gives warnings about path mismatch. This changes the path comparison to be based on the repo url rather than the local working directory. E.g. if both the author and committer are working in branches/release/2013_08_07 despite being checked out in ~/dev/2013_08_07 (system user being different, of course) it no longer warns that the WC path is different
Original behavior:
eric@Eric-MBP ~/dev/2013_07_31: arc commit --revision 21
You are not the author of 'D21: WeMerge Automatic Request'. Commit this
revision anyway? [y/N] y
Revision 'D21: WeMerge Automatic Request' was generated from '', but
current working copy root is '/Users/eric/dev/2013_07_31/'. Commit this
revision anyway? [y/N] y
Committing 'D21: WeMerge Automatic Request'...
Adding test
Transmitting file data .
Committed revision 52676.
Closing revision D21 'WeMerge Automatic Request'...
Exception
ERR-CONDUIT-CORE: You can not mark a revision you don't own as closed.
(Run with --trace for a full exception trace.)
New behavior:
eric@Eric-MBP ~/dev/2013_07_31: arc commit --revision 24
You are not the author of 'D24: WeMerge Automatic Request'. Commit this
revision anyway? [y/N] y
Committing 'D24: WeMerge Automatic Request'...
Adding test
Transmitting file data .
Committed revision 52679.
Closing revision D24 'WeMerge Automatic Request'...
Exception
ERR-CONDUIT-CORE: You can not mark a revision you don't own as closed.
(Run with --trace for a full exception trace.)
Test Plan: 'arc diff' changes with one user. 'arc patch Dxx' on a different working copy by a different user to review and test changes. accept review. 'arc commit --revision xx' as reviewer to land the patch. complaint goes away.
Reviewers: epriestley, ghostwriter78
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D6665
Summary:
Support for no output from arc unit (For scripts, etc).
Also include --output param, analogous to arc lint --output.
Test Plan: run all 6 variants + `--output bad-value`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6642
Summary:
Single quotes aren't valid in the windows cmd prompt, so arc feature
didn't work in mercurial when it got to this line.
I have no idea why %C was used before. Nothing in that string should be
broken by the escaping.
Test Plan:
Ran arc feature --trace on my mac. Verified the command was escaped correctly
and the correct feature results were printed.
I don't have a windows machine to try it on, but the builtin escaping should
now account for windows machines.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D6637
Summary:
When adding arcanist support to a new project or adding a new linter,
it's helpful to be able to run new linters against the entire codebase. This
patch adds support for this with an '--everything' option, similar to 'arc unit
--everything'
Test Plan:
Run 'arc lint --everything' and check out the code. Optionally dump
the paths to test in the current lint engine's buildLinters() function to
demonstrate that it's receiving all files in the project rather than just the
changed and/or specified ones
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D6592
Summary:
- Replace `maniphest.find` with `maniphest.query`. These calls are nearly identical, it was just a rename for consistency.
- Replace `differential.find` with `differential.query`.
Test Plan:
- Ran `arc tasks`.
- Ran `arc close-revision` on valid, nonexistent, and existent-but-invalid revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6336
Summary: Makes sense now
Test Plan: Get someone else to test it
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T3342
Differential Revision: https://secure.phabricator.com/D6307
Summary: Enables the process to pass the user input for library name. Fixes T3342
Test Plan: Little help... `arc liberate` doesn't run on windows. Though I saw the flag :)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3342
Differential Revision: https://secure.phabricator.com/D6306
Summary:
Previously, updating a commit via arc diff in mercurial would
prepopulate the update message with part of the commit message. In an
amend workflow this doesn't make sense, so I disabled it. Git already
does this, so now mercurial matches git in this scenario.
We had users complain that new users would often submit diffs with the
default update message, and it wasn't useful since they were using a
amend flow.
Test Plan:
arc diff on a commit that already had a diff
Verified the editor did not have a update message
arc diff on a stack of commits where the bottom one had a diff
Verified the editor provided a default update message
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6215
Summary: We currently swallow the exception message, but this isn't useful. Fixes T3354.
Test Plan:
$ arc diff HEAD^ --conduit-uri=http://local.aphront.com:8080/ --only
Uploading 1 files...
Failed to upload new binary 'large.png'.
[HTTP/500] Internal Server Error
As received by the server, this request had a nonzero content length but no POST data.
Normally, this indicates that it exceeds the 'post_max_size' setting in the PHP configuration on the server. Increase the 'post_max_size' setting or reduce the size of the request.
Request size according to 'Content-Length' was '2093052', 'post_max_size' is set to '100K'.
Continue? [Y/n]
Reviewers: jamesr, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3354
Differential Revision: https://secure.phabricator.com/D6186
Summary: Now that we examine these in lint, this came up. There is no `ConduitException`; the class is `ConduitClientException`.
Test Plan: Lint.
Reviewers: vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6158
Summary:
Piping data around on Windows doesn't work well when it contains zany characters like "null" and "newline". Fixes T3266.
Instead of piping data into `git apply`, write to a temporary file.
Test Plan:
Ran `arc patch`, got good results.
>>> [17] <exec> $ git apply --index --reject -- '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/7z9iea6srikoo0sc/4266-ZEyvz9'
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T3266
Differential Revision: https://secure.phabricator.com/D6070
Summary:
Arc Revert does the following:
1. Git revert
2. Go to the differential of the rev you are reverting and either repoen it or set it to a reverted state
3. File a hipri task to orig author
[Preview] Creating Arc Revert workflow
Porting arc revert from FB4A to phabricator for general usage. This is my first stab,
so totally appreciate feedback and assistance. I'm currently focused
on making this work for git. However, I built out the functions through the GitAPI so this
could be easily extendable to Mercurial later on.
Stuck on the following (help):
1. Creating a task for FB internal. I tried building on top of existing arc listeners
but getting errors on failures to load the TaskCreator (and other) tasks.
2. I'm using a hacky way to grab the diff revision id from the newly created
revert diff. (see line 204) I'm looking for a way to just fetch the diff ID
from arc after the diff is created; is this possible?
Test Plan:
-
1. Ran arc revert on a www and fbcode diff
2. Confirmed that revert was run on the diffs and a proper diff filed
-
Reviewers: royw, sdwilsh, nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin, pti, keir
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5553
Summary:
During my attept to `arc land master` on `master` branch
I have discovered that error message is missing few spaces between words.
I have added them and used this ugly readonly command:
pcregrep --include="\.php$" -M -r '(?<!\\n| )("|'"'"')\.\n\s*\1(?!\\n| )' ~/arc/arcanist/src
to detect other instances of this serious bug.
Two more were found.
This time they were probably introduced in order to abide
to the draconian lint rule about number of columns.
Since I want to be a good citizen,
I have added this missing space to the begining of the next line in both cases.
It is an ugly hack, but I think user should not suffer due to missing spaces.
Another solution could be preserving no leading spaces and splitting long lines.
Or just providing excuse to lint.
Test Plan: Ran `arc land master` on `master` branch.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3008
Differential Revision: https://secure.phabricator.com/D5827
Summary:
Fixes T2266. Motivation:
- The lint cache does not always invalidate correctly. Because of the nature of the cache, this is a hard problem (right after "naming things").
- We already have a fair amount of complexity in trying to invalidate it, and are still discovering new places where it doesn't work (e.g., Windows with "/" vs "\" paths).
- One invalidation failure is when linter code changes, which seems unresolvable in the general case (e.g., changes to external linters).
- It's not obvious what's happening when the lint cache causes some kind of issue.
- Particularly while developing or debugging linters, your changes often won't be reflected in the lint output. Some of this is theoretically tractable but the external linter case probably isn't.
- When someone reports a problem with the lint cache in IRC or elsewhere, there is essentially never a way for me to fix it. The lint cache can't be debugged effectively without access to a working copy where the problem reproduces.
- The cache provides limited benefit outside of Facebook's install.
To remedy these issues:
- Introduce configuration which controls cache usage.
- Default it off.
- Print a message when the cache is in use.
(I'd tentatively support removing the cache entirely, but I don't know how @vrana and Facebook feel about that.)
Test Plan: Ran `arc set-config --show`, `arc lint --cache 0`, `arc lint --cache 1`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, mbishopim3, nh, edward, wez
Maniphest Tasks: T2266
Differential Revision: https://secure.phabricator.com/D5766
named the same as a file path.
Summary: (In fbcode and www at least, the only places I tried) if a
branch happens to be named the same as a file path (from root) (like if
a branch is called `spec` and there is a directory in `www` called
`spec`) then `arc land` will fail with git complaining that the argument
(to `git log ...`) is ambiguous as it could refer to both a path and a
revision; so this diff adds a `--` to the end so that git knows that
both are revisions and not paths.
Test Plan: Try and land something from www (I did, see D752219) where
the branch is named `spec`.
Reviewed by: epriestley
Test Plan:
$ set # on Windows
Reviewers: epriestley, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5733
Summary:
Arc lint for git is currently adding all untracked files when it
amends the commit with lint fixes. This changes the git add -A to be
git add -u. This only adds files that were already tracked. -A was adding
untracked files as well which was not the desired behavior here.
Test Plan:
Create an untracked file.
Commit a lint failure another file.
arc diff and choose to amend the lint patches.
Verify that the untracked file was not added but the tracked file was amended.
Reviewers: epriestley, wez, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5731
Summary:
arc lint was hardcoded for git for amending commits with lint
patches. This enables the same functionality for mercurial.
Test Plan:
Made some changes that would result in a lint patch.
arc diff
Verify that the patches it produces were amended into the commit.
Verified it still works in git as well.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5716
Summary:
@ender is reporting a parsing issue in SVN, but we don't build a parser with setWriteDiffOnFailure() set in this workflow right now so I can't get the raw file to fix the issue.
Use the onboard mechanism to build a parser with `setWriteDiffOnFailure()` set, so it will write the diff, so I can get a copy so I can fix the problem.
Test Plan: Ran `arc diff --preview` in an SVN repo with a linter.
Reviewers: ender, btrahan
Reviewed By: ender
CC: aran
Differential Revision: https://secure.phabricator.com/D5699
Summary:
This adds a arcconfig setting to allow specifying whether to use the merge
or rebase strategy when doing the feature branch update.
arc.land.update.default can be set to either 'rebase' or 'merge'. The command
line flags will override this setting.
We have had trouble with arc land producing merge commits (introduced
with D4080) in git. They usually appear when arc land fails, and our users
are confused by the presence of a merge commit afterwards. Today it got even
worse since a user managed to get arc land to push the merge commit to the
server. This setting will allow us to turn it off for our uses.
Test Plan:
Verified the following combinations:
update.default not set + arc land (saw git merge in the trace)
update.default = 'rebase' + arc land (saw git rebase)
update.default = 'merge' + arc land (saw git merge)
update.default = 'rebase' + arc land --update-with-merge (saw git merge)
update.default = 'merge' + arc land --update-with-rebase (saw git rebase)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5683
Summary:
The `emailuser` template is a relatively recent addition to Mercurial, and a few users have complained about it. It also doesn't actually do what I thought it did, e.g. in an address like this:
"Abraham Lincoln" <alincoln@whitehouse.gov>
^^^^^^^^^^^^^^^ ^^^^^^^^
(1) (2)
^^^^^^^^^^^^^^^^^^^^^^^
(3)
...I want (1), but `emailuser` means (2). Instead, extract (1) with `getDisplayName()` and (3) with `getAddress()` using PhutilEmailAddress.
The implementation in Mercurial is not particularly sophisticated or magical (it just looks for "@" and "<") so we aren't really missing anything by doing this ourselves, at least today.
Also fix some issues in `arc export`, which literally no one uses, but which is occasionally useful for testing (as here).
Test Plan:
- Ran `arc diff --only` in an `hg` repo, checked DB to see that name/email were correctly extracted.
- Ran `arc export --git` in an `hg` repo, didn't get a long series of fatals.
Reviewers: btrahan, DurhamGoode
Reviewed By: DurhamGoode
CC: aran
Maniphest Tasks: T2866, T2858
Differential Revision: https://secure.phabricator.com/D5539
Summary:
Arc land is a bit magical and some users have gotten bitten by
the fact that it collapses and lands every commit on the branch. To make
it explicit what is being landed, it now shows a list of the commits
that are being landed. I also added a --preview flag that will just
print the commits that would be landed, but does nothing else.
Hopefully this make arc land a little less magical for people.
Test Plan:
arc land in the following scenarios:
- Landing one change
- Landing no changes
- Landing a stack of changes
Did it with hg and git.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5460
Summary:
Previously, arc patch would create a new commit under the existing
current bookmark in mercurial. There have been two discussions about what the
right behavior should be (D3334 and D3658). One side wants no commit at all,
and one side wants a commit under a new bookmark. The current implementation is
the worst of both worlds :(
This change makes it create a new bookmark at the revision's base before commiting,
same as the --bookmark flag used to do (which is now obsolete). That way the
existing bookmark doesn't move (in mercurial >=1.8). This is the same behavior
git has, which is convienent for groups migrating between the two.
Also makes hg's getCanonicalRevision handle svn revisions just like git. This way
arc patch will try to apply the patch to the appropriate revision in the history.
Test Plan:
Ran:
arc patch - Verified it created a new bookmark and commited on top of the
revision's base commit.
arc patch --nobranch - Verified it put the new commit on top of the current
bookmark without a new bookmark.
arc patch --nocommit - Verified it left all the changes in the working copy.
Also verified arc patch still works with git.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5408
Summary:
- Added arc.autostash option to have this behaviour off by
default (but configurable on a per-project basis).
- Automatic stashing of changes now informs the user of how
to restore their working directory if Arcanist unexpectedly
terminates.
- Fixed an issue with finalizeWorkingCopy when the workflow
didn't require a clean working copy.
Test Plan:
Test `arc diff` when there are changes in the working
directory; by default it should tell you to stash or commit.
Turn on the arc.autostash option and try again; it should
automatically stash with a message on how to recover, and
it should restore the working directory automatically under
almost any circumstances (other than an unrecoverable error).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5385
Summary:
Adds error handling for several kinds of failure in arc land for
mercurial. Previously it would often leave the repo in a confusing state
if something failed.
- Aborts the land if pull brings in a diverged 'onto' branch.
- Aborts the rebase if there is a conflict. This leaves the repo exactly as
it was before, so the user is not left with a half finished rebase.
- Don't delete the original non-squashed branch until the push succeeds.
- If the push fails, strip the temporary squashed commit. This leaves the
'onto' branch back on the latest commit from the server, and leaves the users
original nonsquashed branch around.
- Always leave the user back on their original branch after an error.
Test Plan:
Ran arc land:
- with pull causing a diverged 'onto' bookmark
- with the 'onto' bookmark already diverged
- with the rebase causing conflicts
- with a push that failed due to a commit hook
- with a successful land
- with a successful collapse and land
In all failure cases the repo was left exactly as it was before arc land,
except for the push-failed case, where the only change was that the branch
was correctly on top of the destination branch due to a successful rebase.
Used bookmark name "foo bar-gah" to test that crazy bookmark names still work.
Reviewers: epriestley, dschleimer, sid0
Reviewed By: epriestley
CC: nh, wez, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5394
Summary:
Arc patch was committing with -A (--addremove) which meant any random
files that were sitting around in the repo (like conflict .orig files) were
added to the commit. The -A isn't even necessary since the hg import
adds and removes all the appropriate files for you.
Test Plan:
touch foo
arc patch --diff some-diff-id
Verified that foo was not added to the commit
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5396
Summary:
Changes arc feature to read 'arc.feature.start.default' instead
of 'arc.land.onto.default'. In our usage we actually need to fork off
a different branch than we land to, so separating these is useful.
Test Plan:
Set arc.land.onto.default = master
Set arc.feature.start.default = bar
arc feature foo
cat .git/config
Verified the foo branch tracked bar
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: wez, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5336
Summary: If user changes the file contents during linting (usually when prompted to apply a patch) then we save the old messages to the new file contents. Fix that by computing the hash before linting (or after applying patch).
Test Plan: Changed the file during linting, verified that the file hash didn't change.
Reviewers: epriestley
Reviewed By: epriestley
CC: ptarjan, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5320
Summary: I looked at the pros & cons at adding hooks in git/hg vs arc land and I prefer arc land.
Test Plan:
* added an event listener and made sure I could handle the event.
* made sure things get reverted when the event handler throws an exception.
Reviewers: vrana, epriestley, pieter
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5268
Summary: The message suggests that only one revision would land.
Test Plan: Read it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5277
Summary: Have arc land inspect the revision if it depends on some other revisions which haven't been closed yet. If yes, then warn users.
Test Plan: Will test them locally.
Reviewers: epriestley, AnhNhan
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5262
Summary:
Adds arc.feature.start.default arcconfig setting to specify
a default value for 'start' in 'arc feature name start'. This lets
users always branch from origin/master (or whatever the main branch is).
Also cleaned up the 'feature' help text a little. The stuff about sorting
and closed/abandoned revisions is explained via the options list already.
Test Plan:
Ran arc feature with/without a start and with/without the config
setting set.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: bos, sid0, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5184
Summary:
When using arc feature, it should set up the tracking branch to be
master.
Test Plan:
./bin/arc feature tracking
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5169
Summary: These are the errors I really do.
Test Plan:
$ arc diff --no-lint
(Assuming '--no-lint' is the British spelling of '--nolint'.)
$ arc diff --reviewer a
(Assuming '--reviewer' is the British spelling of '--reviewers'.)
New unit test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5185
Summary: Changes message for `arc land` that displays current branch or bookmark (if none is specified) to appropriately use the term 'bookmark' when on a bookmark in an hg repository.
Test Plan:
Ran `arc land` on new git and hg repositories, checking for correct identification of 'branch' or 'bookmark'.
~/test$ mkdir hg-test
~/test$ mkdir git-test
~/test$ cd hg-test
~/test/hg-test$ hg init
~/test/hg-test$ hg branch
default
~/test/hg-test$ hg bookmarks
no bookmarks set
~/test/hg-test$ arc land
Landing current branch 'default'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'default' onto 'default'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
~/test/hg-test$ hg bookmark testmark
~/test/hg-test$ hg bookmarks
* testmark -1:000000000000
~/test/hg-test$ hg branch
default
~/test/hg-test$ arc land
Landing current bookmark 'testmark'.
Usage Exception: Source testmark is a bookmark but destination default is not a bookmark. When landing a bookmark, the destination must also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.
Confirm still works on a git branch:
~/test/hg-test$ cd ../git-test/
~/test/git-test$ ls
~/test/git-test$ git init
Initialized empty Git repository in ~/test/git-test/.git/
~/test/git-test$ touch testfile
~/test/git-test$ git commit -am 'Test file'
~/test/git-test$ git branch
* master
~/test/git-test$ arc land
Landing current branch 'master'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'master' onto 'master'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
Reviewers: DurhamGoode, epriestley
Reviewed By: DurhamGoode
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5163
Summary:
Some tests take longer (fixtures usually around 1 second for me) and also FB runs all tests on deploy.
I want to see all results immediately.
Test Plan:
Added `usleep(200000)` to `resultTest()`, then:
$ arc unit
Saw results printed one by one.
Also didn't pass `$renderer` to `ArcanistPhutilTestCase` and saw empty output.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5141
Summary:
Previously, running arc lint on a set of changes that only
existed in your working copy threw an exception in mercurial repos.
It was trying to use the revset "...." (i.e. the range from . to .),
which didn't parse. Even if I fix that it still doesn't work because
getRawDiffText did not include the working copy changes (which it does
in git). I removed the check so the function now acts the same as in
git and arc lint works on working copy changes. I've seen this error before
in other places so hopefully this change will also fix any other areas,
that depended on getRawDiffText working the same as git.
The logic I removed was added in D1954 to support diffing against
uncommited changes. That workflow should be unchanged. arc diff will
still prompt the user if there are uncommited changes, and the user can
still choose to abort or continue.
Let me know if I missed something important which makes this a bad idea.
Test Plan:
Edited a file in the working directory of a hg repo.
arc lint
Verified lint ran successfully.
Also ran arc diff and land with and without working copy changes to make sure
they still work. I'd kill for some tests in this area...
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Maniphest Tasks: T1631
Differential Revision: https://secure.phabricator.com/D5130
Summary:
I want to run lint on background and I'm interested only in side effect of caching (and maybe exit status).
This is better than discarding stdout later because we don't do unnecessary work and error conditions are still printed.
Test Plan:
$ arc lint --output none # with error
$ echo $?
$ arc lint --output none # with no lintable paths
$ arc lint --output none # witout errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5106
Summary: If there's other revision in last commit message and I said `--create` then it is clear that I want to create a new commit.
Test Plan:
$ arc diff --create # in dirty working copy on top of my open revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5078
Summary: If I have //Differential Revision// in my commit message then `arc diff --create` updates that revision instead of creating a new one.
Test Plan:
$ arc diff --create # on top of commit message with Differential Revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5077
Summary:
The arc feature command wasn't actually creating bookmarks
on mercurial. It needs to call 'hg bookmark' instead of 'hg update'
Also removed an unnecessary hgsprintf since there were no arguments.
Test Plan:
Ran 'arc feature foo' and 'arc feature bar tip^'.
The former created a bookmark at my current location.
The latter created a bookmark at tip^ and moved me to that revision.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5023
Summary:
Mercurial 'arc land --hold' was taking 90+ seconds on our large
repository. Since most of arc land doesn't require any particular working
directory, I've changed the mercurial logic to avoid all updates except for
two: the one prior to finding the revision (only applies if the user specified
--branch), and the one at the end to leave the user in a good state.
Also got rid of a 'hg outgoing' call when phases are supported. Also changed
the hg-subversion detection to just look for .hg/svn instead of running 'hg
svn info', which was taking 4 seconds.
Now arc land takes about 50 seconds. Still much worse than git's 25 seconds.
One big hot spot is in the two 'hg rebase' calls, which account for 25 seconds
(versus 11 seconds of git).
Test Plan:
Tested arc land with mercurial and git. Tested with and without the --branch
options.
Reviewers: epriestley, bos, sid0, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5014
Summary:
Arc diff will hash file data and try to upload the file using upload by hash rather than transferring data. If it is unable, it defaults to its normal behavior
Attempts to upload file by hash, use regular upload method otherwise
Test Plan: Figure out how to arc diff to my local install and look at the behavior
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mehrapulkit
Differential Revision: https://secure.phabricator.com/D4968
Summary: Followup to D4703. When we give paths to `svn`, we need to escape them if they contain an `@`.
Test Plan:
Created a patch full of modifications to files with `@` in their names, and applied it:
$ arc patch --diff 192
A A@2xcopy2
A A@2xcopy
D A@2x
OKAY Successfully applied patch to the working copy.
Reviewers: chad, mbishopim3
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4977
Test Plan:
$ arc diff -a
$ arc diff -a # saw amend instead of a new commit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4947
Summary:
We save repository version to lint cache but ignore it when reading the cache.
Fix it.
Test Plan: Made an error for linter with repo granularity, deleted the error from the cache. Relinted, didn't see the error. Changed another file and relinted, saw the error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4841
Summary:
Mercurial 'arc land' uses the 'hg strip' command to clean up after
itself, but this command isn't available unless the mq extension is enabled.
The fix is to enable it for that particular command only.
Test Plan: Ran 'arc land' with the mq extension disabled. It worked.
Reviewers: epriestley, bos, sid0, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4955
Summary:
Previously 'arc diff X' with mercurial meant to use X as the base
to diff against. Now it means use gca(X,working directory) as the base to
diff against. This matches the git behavior.
Test Plan:
Ran 'arc diff master' on a repo where master was ahead of the feature branch.
Verified that the diff result included only the diffs in the feature branch.
Reviewers: epriestley, sid0, bos, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4865
Summary: Makes sense after D2471.
Test Plan:
Swapped two binary files, ran `arc diff --only`.
Saw time 3.797 s instead of 4.361 s.
Changed `file.upload` to `file.uploa`, saw proper error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4847
Summary: If provided, have `arc patch` use `authorName` / `authorEmail`. This simplifies handling and makes patches more portable between version control systems (previously, information was generated in the diff's VCS, regardless of which VCS it was being applied to).
Test Plan: Created a diff with author `derp <derp@derp.com>`, ran `arc patch --diff x`, got a local commit with that author.
Reviewers: btrahan, edward, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4827
Test Plan: Copied the code in a script, changed `phutil_passthru()` to `echo csprintf()` and ran it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4805
Summary:
FB currently starts Sandcastle push before starting the diff workflow.
If `arc diff` commits something then we need to restart the push.
I want to avoid this by starting the push after commit.
Test Plan: Will test after implementing the listener.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4785
Summary: Currently, we don't expose these at top level, so you can't disable coverage if your coverage is explosively broken. Expose them as passthrough arguments.
Test Plan:
- Touched a file in `arc` which triggered unit tests.
- Without `xdebug` installed:
- Ran `arc diff --preview`, `arc diff --preview --no-coverage` (both fine).
- Ran `arc diff --preview --coverage`, got exception about coverage not being available.
- Installed `xdebug`.
- Ran `arc diff --preview`, got coverage.
- Ran `arc diff --preview --coverage`, got coverage.
- Ran `arc diff --preview --no-coverage`, no coverage.
Reviewers: indiefan, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4745
Summary: `git pull` may fail in git-svn after rebasing (which is a side effect of dcommit).
Test Plan:
$ git svn rebase
$ git log trunk..master
$ git pull --ff-only; echo $?
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4716
Summary: Some people have 2GB+ untracked files in repo which significantly slows down this or even crashes it.
Test Plan: Added a debug output here and linted repo with untracked path.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, arudolph
Differential Revision: https://secure.phabricator.com/D4713
Summary: Fixes T2438. We currently escape everything with '@', but SVN rejects that for '.'
Test Plan:
Unit tests. Performed this commit:
$ svn st
M .
A x@123
$ arc commit --conduit-uri=http://local.aphront.com:8080/ --revision 53
Revision 'D53: asdf' has not been accepted. Commit this revision anyway?
[y/N] y
Committing 'D53: asdf'...
Sending .
Adding x@123
Transmitting file data .
Committed revision 37.
Done.
I grepped for more '@' adding but couldn't find any. It's a bit tricky to grep for though, so it's possible I missed some.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T2438
Differential Revision: https://secure.phabricator.com/D4703
Summary:
arc land on a hg-svn repository would fail because arc land
uses 'hg push -r' to specify which revs to push which is not supported
by hg-svn. Now we just use 'hg push' which, when used against svn, only
pushes the current branch (which happens to be the branch we're trying to land).
We can't use standard 'hg push' for non-svn repos though because when used
against a vanilla hg repo 'hg push' pushes all branches.
Also remove --new-branch from 'hg push' because it's extremely
unlikely that a person wants to create a new branch on the server via
arc land.
Test Plan:
Ran arc land on a normal hg repo, verified it used 'hg push -r'.
Ran arc land on a hg-svn repo, verified it used 'hg push' and it pushed the
correct changes.
Reviewers: epriestley, sid0, dschleimer
Reviewed By: epriestley
CC: bos, aran, Korvin
Maniphest Tasks: T2403
Differential Revision: https://secure.phabricator.com/D4653
