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
