1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-12-05 21:26:13 +01:00
Commit graph

2007 commits

Author SHA1 Message Date
Brayden Winterton
7e394dcf11 Changed checking for a closed task in arc tasks to look for the isClosed property instead of a non-empty status
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
2014-04-09 07:48:58 -07:00
Bob Trahan
2714395d98 Arcanist - allow amending revisions by other authors in the working copy
Summary: Fixes T4670. Give the user an option to abort but otherwise proceed.

Test Plan: arc patch D8685 (by epriestley); arc amend

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4670

Differential Revision: https://secure.phabricator.com/D8716
2014-04-08 11:58:50 -07:00
Peng Li
0cff627d75 Fix an issue in arc patch with git-svn
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
2014-04-08 10:46:27 -07:00
Bob Trahan
f099168aa8 Make "arc land" support landing someone else's code that got there via arc patch
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
2014-04-07 11:44:14 -07:00
Gabriel Guzman
0e5bea940c T4670 Remove the any-author flag from the which command, make any-author the default, and annotate the username of the owner of the revision.
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
2014-04-03 11:30:38 -07:00
David Cramer
b8e4261455 Add coverage reports to py.test runner
Summary: Of note this now forces the pytest-cov plugin to be installed unless they turn it off.

Test Plan:
```
../arcanist/bin/arc unit

COVERAGE REPORT
     97%     changes/models/test.py
    100%     tests/changes/api/serializer/models/test_testcase.py
    100%     changes/api/serializer/models/testcase.py
 UNIT OKAY  No unit test failures.
Updated an existing Differential revision:
        Revision URI: https://tails.corp.dropbox.com/D43387

Included changes:
  M       changes/api/serializer/models/testcase.py
  M       changes/models/test.py
  M       tests/changes/api/serializer/models/test_testcase.py
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8667
2014-04-02 11:31:38 -07:00
epriestley
11e2c1688f Fix escaping of bookmarknames properly for arc patch
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
2014-04-01 08:21:15 -07:00
epriestley
5280f3708e Strip comments during arc diff --verbatim workflow before submitting to Phabricator
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
2014-04-01 08:21:05 -07:00
epriestley
a36969e58b Allow arc to parse svnlook diffs from moves/copies
Summary: Ref T4697. This is similar to the existing stuff, but `svnlook diff ...` can also produce a "Copied" header. Currently, we choke on it in Herald when running pre-commit rules.

Test Plan: Added and ran tests. In the next diff, used this in a real system.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4697

Differential Revision: https://secure.phabricator.com/D8653
2014-04-01 08:20:55 -07:00
Aviv Eyal
d0bab7f34a fix tab complete for out-of-workdir
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
2014-03-25 17:33:23 -07:00
epriestley
46fe94db9f Support git-format-patch format in diff parser
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
2014-03-15 11:25:49 -07:00
epriestley
1e6d958b27 Use more modern detection of repositories in "arc close-revision"
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
2014-03-13 18:01:53 -07:00
epriestley
03ddc17032 Don't close revisions on "arc amend"
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
2014-03-13 18:00:20 -07:00
epriestley
67239a08a5 Use hgsprintf() when managing bookmarks in arc patch in mercurial
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
2014-03-12 19:43:48 -07:00
Peng Li
ac82dea3c9 Lint engine seems to get null configuration manager in svn precommit hook workflow
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
2014-03-11 15:35:28 -07:00
Bob Trahan
37dac61131 Arcanist - add revision data to TYPE_LAND_WILLPUSHREVISION event
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
2014-03-11 15:21:46 -07:00
Joshua Spence
a2706f6539 Ignore detached git branches.
Summary:
Fixes T4559.

It looks like the code currently (at least partially) handles this by checking for `(no branch)`. I suspect that the behaviour of `git` has changed (I am running version 1.9.0) because I haven't figured out what state to be in to cause `git` to output `(no branch)`.

Test Plan: Ran `arc branch` when on a detached branch.

Reviewers: #blessed_reviewers, epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4559

Differential Revision: https://secure.phabricator.com/D8466
2014-03-09 08:04:33 -07:00
epriestley
a90a72c648 Make assertTrue() / assertFalse() messages more descriptive
Summary: See discussion in D8460. Primarily, this prints out the failing value when a true/false assertion fails, so if it was something useful (like a function result) it's visible.

Test Plan:
Added `assertTrue("quack")`:

     FAIL  ArcanistDiffParserTestCase::testParser
  Assertion failed, expected 'true' (at ArcanistDiffParserTestCase.php:16).

  ACTUAL VALUE
  quack

Added `assertFalse("quack")`:

     FAIL  ArcanistDiffParserTestCase::testParser
  Assertion failed, expected 'false' (at ArcanistDiffParserTestCase.php:16).

  ACTUAL VALUE
  quack

Added `assertEqual("quack", "moo")`:

     FAIL  ArcanistDiffParserTestCase::testParser
  Assertion failed, expected values to be equal (at ArcanistDiffParserTestCase.php:16).
  Expected: quack
    Actual: moo

Reviewers: joshuaspence

Reviewed By: joshuaspence

CC: aran

Differential Revision: https://secure.phabricator.com/D8465
2014-03-08 19:23:23 -08:00
epriestley
06cfe0746e Utilize assertFalse and assertTrue methods.
Summary:
Ref D8460.

Use `$this->assertFalse(...)` and `$this->assertTrue(...)` instead of `$this->assertEqual(false, ...)` and `$this->assertEqual(true, ...)` respectively.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8461
2014-03-08 18:33:05 -08:00
Joshua Spence
c42bef4e25 Added some additional assertion methods.
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
2014-03-08 18:23:14 -08:00
Joshua Spence
cf52d88f8b Fix unit test coverage for NoseTestEngine.
Summary:
I noticed that code coverage wasn't showing in Differential for
some repositories that we are using with Phabricator.

`arc unit` would should unit test coverage, but the paths were messy
(for example, `.//foo/bar.py` instead of `foo/bar.py`). As a result,
the code coverage info wasn't recognised as being for the correct
module.

I'm not sure why this logic is the way that it is... perhaps this is to
do with an older version of `nose` (I am using v1.3.0).

Test Plan:
I created a diff for an internal repository that we have, and
observed that code coverage was displayed in Differential.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, seporaitis, avive, dctrwatson, zeeg

Differential Revision: https://secure.phabricator.com/D8433
2014-03-07 10:17:04 -08:00
epriestley
c6b1f3f070 Fail Arcanist tests when they make zero assertions
Summary:
Fixes T4570. When a test case doesn't make any assertions, fail it:

  - A tiny fraction of tests pass by not throwing. These tests can easily make a trivial assertion. It took about 5 minutes to fix them all (D8435, D8436).
  - In other cases, no assertions means a test construction problem, as with T4570. In these cases, failing loudly catches a severe error.
  - Fixes the no-assertion test cases in `arcanist/`
  - Makes the PHP 5.4 test pass for the moment, see discussion in T4334.

Test Plan: Ran `arc unit --everything`.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8437
2014-03-07 10:03:24 -08:00
epriestley
22f6207920 Fail lint tests if no tests are found in the test directory
Summary: Ref T4570. Testing a directory with no recognized tests currently passes, but should fail.

Test Plan:
  - Ran `arc unit`.
  - Removed tests from a test directory, ran `arc unit`, got test failure.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8434
2014-03-07 10:03:17 -08:00
Peng Li
18251a1bd8 Remove a parameter
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
2014-03-06 15:53:06 -08:00
epriestley
133d289ad1 Generate a documentation book for Arcanist
Summary: Ref T988. Not pretty or polished, but works fine.

Test Plan: Generated the docs, made sure they were all there.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8413
2014-03-05 13:00:40 -08:00
epriestley
7b6b24154b Add "Changes Planned" and "In Preparation" states for revisions
Summary:
Ref T2222. Discussion in T4481. Ref T2543.

Primarily, this will let me fix some of the rough edges that came out of ApplicationTransactions and state-based transitions.

I'm also adding a constant for T2543 while I'm in here.

Test Plan: Used `grep` to look for any weird special behavior, didn't see any.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2543, T2222

Differential Revision: https://secure.phabricator.com/D8400
2014-03-05 10:44:26 -08:00
Joshua Spence
fb826bbf9c Correctly identify @todo comments as TODOs.
Summary: Allow `@todo` comments to be linted as TODOs as well as `TODO` comments.

Test Plan: I added a new test case (`todo.lint-test`)

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8389
2014-03-04 11:03:09 -08:00
Joshua Spence
0888b6616c Use PHP type hinting in ArcanistXHPASTLinter.
Summary: Explicitly specify the types of the function parameters.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8388
2014-03-04 11:02:18 -08:00
Joshua Spence
9bca4bfda8 Simplified the logic in various functions.
Summary:
- Tidied up `ArcanistCSSLintLinter::getDefaultBinary`.
- Tidied up `CSSLintLinter::getDefaultFlags` function.
- Tidied up `ArcanistPhpcsLinter::getDefaultBinary` function.
- Tidied up `ArcanistPEP8Linter::getDefaultFlags` function
- Tidied up `ArcanistFlake8Linter::getDefaultFlags`.
- Tidied up `ArcanistCppcheckLinter::getLintOptions`.
- Tidied up `ArcanistCppcheckLinter::getLintPath`.
- Tidied up `ArcanistCpplintLinter::getLintOptions`.
- Tidied up `ArcanistCpplintLinter::getLintPath`.
- Removed child functions which are identical to the corresponding parent functions.

Test Plan: `arc lint` and `arc unit`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8385
2014-03-04 11:00:01 -08:00
Joshua Spence
9fb2a147f9 Made ArcanistNoLintTestCaseMisnamed final.
Summary: Self explanatory.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8386
2014-03-04 10:55:00 -08:00
Joshua Spence
53492fd01e Removed useless comments.
Summary: To me, it seems that these comments add no value. Personal opinion I suppose.

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8382
2014-03-04 10:53:17 -08:00
Joshua Spence
9fbf316d38 Various linter fixes
Summary:
Fixed various linter issues that were identified by running `arc lint --everything`.

- Removed trailing whitespace.
- Added newline at EOF.
- Added whitespace after `if` statement

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: aurelijus, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8337
2014-02-25 07:53:07 -08:00
epriestley
a11421ee8e Add password_* functions to extension functions list
Summary: These are technically added in PHP 5.5.0 mainline, not an extension, but the two are effectively equiavlent.

Test Plan: o.O

Reviewers: btrahan, dctrwatson

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8280
2014-02-20 17:26:03 -08:00
Joshua Spence
bd6cc6b946 Update CSSLint bindings for v0.10.0
Summary: See <https://github.com/facebook/arcanist/pull/131>

It looks like the behaviour of csslint has changed since the ArcanistCSSLintLinter was written. Consequently, ArcanistCSSLintLinter does not work with the latest version of csslint (v0.10.0).

  - `csslint` has a non-zero exit status
  - Fixed `csslint` parsing for v0.10.0

Reviewed by: epriestley
2014-02-16 15:20:29 -08:00
epriestley
13fd0a5ef1 Minor modernizations to arc browse
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
2014-02-09 12:19:57 -08:00
Ben Alpert
52d3cd1b4e Support browsing '.' and default to it
Test Plan: Ran 'arc browse', 'arc browse .', 'arc browse main.py'.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, avivey

Differential Revision: https://secure.phabricator.com/D8174
2014-02-09 09:01:52 -08:00
Ben Alpert
c3fe03db08 Detect repo for 'browse'; don't require project ID
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
2014-02-09 08:55:16 -08:00
epriestley
7c46000527 Permanently remove license linters
Summary: For eventual commit.

Test Plan: none

Reviewers: nh, btrahan

Reviewed By: btrahan

CC: aran, vrana, FacebookPOC

Maniphest Tasks: T2274

Differential Revision: https://secure.phabricator.com/D4907
2014-02-03 10:01:19 -08:00
Richard van Velzen
114bb9f25b Align usages of "\1" and "\2" in MercurialRepositoryAPI
Summary:
Especially on Windows it is hard to use "\1" type escapes in shell commands. The direct usage resulted in some undefined variables because the \1 and \2 weren't actually passed as control characters.

By passing them through the regular arguments list they get sent in the "correct way" regardless of OS

Test Plan: Executed `arc diff` in a HG repo and did not get undefined indexes back

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8129
2014-02-02 08:26:25 -08:00
epriestley
988a482ff3 Minor, fix an issue with SVN. 2014-01-29 15:05:51 -08:00
Guy Warner
14f2f7fdd2 Arc output the correct action being taken
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
2014-01-28 08:54:01 -08:00
epriestley
d1a68ccb37 Don't consider a no-op amend to be an error in Mercurial
Summary:
Unlike git, Mercurial considers an `hg commit --amend` which doesn't change the working copy to be an error.

The current behavior for this is fairly bad, since the user gets an exception wrapping the entire command ("Command Failed! ...") and it's pretty verbose and not obvious what has happened.

Some alternatives are:

  1. Detect this condition and raise a more tailored exception, like a UsageException.
  2. Detect this condition and succeed.

Although I tend to think (1) is the right approach in general (that is, `arc x` should usually behave like `git x` or `hg x`), I went with (2) here because we have a handful of amend callsites and they all assume git semantics (no-op amends are successful), and because I think Mercurial's behavior is a little silly (the working copy ends up in the correct / expected state, which seems fairly clearly like a success to me).

Test Plan:
  - Had reporting user verify patch.
  - Ran `arc amend --revision Dxxx` twice in a Mercurial working copy.

The old output looked like this:

  $ arc amend --revision 922
  Amending commit message to reflect revision D922: Improve performance of PhutilSymbolLoader::selectAndLoadSymbols().
  Exception
  Command failed with error #1!
  COMMAND
  HGPLAIN=1 hg commit --amend -l '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/71k8q057844c4g84/3539-gfkvV4'

  STDOUT
  nothing changed

  STDERR
  (empty)
  (Run with --trace for a full exception trace.)

The new output looks like this:

  $ arc amend --revision 922
  Amending commit message to reflect revision D922: Improve performance of PhutilSymbolLoader::selectAndLoadSymbols().
  Done.

Reviewers: btrahan, richardvanvelzen

Reviewed By: richardvanvelzen

CC: aran

Differential Revision: https://secure.phabricator.com/D8083
2014-01-28 03:03:30 -08:00
epriestley
7116659289 Improve some messaging in arc which for autodetection of repositories
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
2014-01-27 19:45:18 -08:00
epriestley
1daa719ace Don't choke on blame of files with whitespace-only trailing lines
Summary:
Fixes T4349. Two issues:

  - As discussed in T4349, we would trim the entire output and then require spaces when matching. This choked incorrectly if the last line of a file contained only whitespace. Use `phutil_split_lines()` instead, and regexp things more reasonably.
  - We were capturing the line text, not the commit, as "revision". This isn't actually used elsewhere, but was obviously wrong. Make this consistent with Git/SVN.

Test Plan: Rigged a call up and saw reasonable output after the patch, on a working copy which threw before the patch.

Reviewers: durham, btrahan

Reviewed By: durham

CC: aran

Maniphest Tasks: T4349

Differential Revision: https://secure.phabricator.com/D8078
2014-01-27 11:03:08 -08:00
epriestley
b1d3948d77 Default to array() when failing to look up repository PHIDs.
Auditors: btrahan
2014-01-27 10:36:40 -08:00
epriestley
2ef2b76f10 Restore "project.name" (previously "project_id") to .arcconfig for arcanist
Summary:
A couple of things actually still care about this:

  - The code that detects if we're running `arc` on `arc`, and then runs the `arc` we're running on instead of the `arc` we ran, checks for the project being "arcanist" (`reenter_if_this_is_arcanist_or_libphutil()`).
  - Some of the symbol stuff still relies on it.
  - Removing it was really more to prove it works (which it seems to) than to accomplish any concrete goals.

Test Plan: looked at the file I guess

Reviewers: btrahan, talshiri

Reviewed By: talshiri

CC: aran

Differential Revision: https://secure.phabricator.com/D8076
2014-01-26 16:11:25 -08:00
epriestley
a7376624b4 Allow arc to identify repositories without "project_id"
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
2014-01-26 15:31:30 -08:00
epriestley
e4b1e8e681 Use user.query, not user.find, in arc tasks
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
2014-01-25 14:23:23 -08:00
Sergey Sharybin
2c2c5663cb Fix exception on diff parse on FreeBSD
FreeBSD 9.2 comes with diff tool version 2.8.7 which behaves
a bit different from how it is expected to. Namely for diff
between two binary files it says:

  Files A and B are differ

This was leading to an exception when browsing revisions with
changes in binary files.

Tweaked parse patterns in order to fix this issue. Now both
older and newer diff tools are supported.

See: <https://github.com/facebook/arcanist/pull/139>

Reviewed by: epriestley
2014-01-18 11:13:28 -08:00
epriestley
a247e21093 Call willLintPath() hook from PhutilLinter
Summary:
Some callpaths end up here, which doesn't cause the internal linter's willLintPath() method. This can mean its `activePath` is set wrong, which results in us raising lint for the wrong file.

Particularly, if you apply D7979 (diff 18069) and `arc lint` it, you'll get a syntax error message in the wrong file.

Test Plan: Applied D7979 and linted it, got proper error message.

Reviewers: btrahan

Reviewed By: btrahan

CC: aarwine, aran

Differential Revision: https://secure.phabricator.com/D7988
2014-01-17 21:30:18 -08:00