1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2025-01-06 04:41:00 +01:00
Commit graph

766 commits

Author SHA1 Message Date
epriestley
06c641f92c Remove command spelling correction from Arcanist
Summary: Fixes T7489. Depends on D16332, which moved this code to libphutil.

Test Plan:
```
$ arc banch --bystatus
(Assuming 'banch' is the British spelling of 'branch'.)
(Assuming '--bystatus' is the British spelling of '--by-status'.)
...
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7489

Differential Revision: https://secure.phabricator.com/D16333
2016-07-27 09:35:28 -07:00
Rabah Meradi
1bedfdccd7 Fixes help message for stop command
Test Plan: arc help stop will display 'Stop tracking work...'

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: avivey, chad, epriestley

Differential Revision: https://secure.phabricator.com/D16310
2016-07-27 05:42:07 -07:00
epriestley
4d4d16f259 Validate Arcanist install-certificate URIs more carefully
Summary: Fixes T11222. This was lazy-future-proofed for Conduit SSH support, but users are boundlessly creative. Check protocols explicitly.

Test Plan:
```
$ arc install-certificate a.b:1/
Usage Exception: Server URI "a.b:1/" must include the "http" or "https" protocol. It should be in the form "https://phabricator.example.com/".
```

  - Also went through a successful workflow with a URI in the form provided in the example.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11222

Differential Revision: https://secure.phabricator.com/D16188
2016-06-28 15:20:06 -07:00
epriestley
2374403e8f Remove the "you have not specified reviewers" prompt from the arc client
Summary:
Ref T4631. Ref T10939. I don't have any good solutions here; this is perhaps the least-bad one.

  - This prompt is misleading/confusing in the presence of Herald/Owners.
  - This prompt is likely of very little value for experienced reviewers.
  - When it works, this prompt may be of some value for new reviewers, but getting it wrong is probably more confusing than getting it right is helpful, and there is a more accurate version of the warning in the web UI that new users are likely to see.
  - In the long run, this code should not live in the client.

Test Plan: Created this revision without specifying reviewers, probably didn't get prompted.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4631, T10939

Differential Revision: https://secure.phabricator.com/D16139
2016-06-17 08:04:08 -07:00
Aviv Eyal
ca33240942 Don't specify size 0 for deleted files
Summary: See D15828 - arc is reporting file size as `0` for unexisting files - make it stop.

Test Plan: `arc diff` with empty, deleted, added files - see size reported as `null` when appropriate.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: chad, Korvin

Differential Revision: https://secure.phabricator.com/D16059
2016-06-06 20:51:16 +00:00
epriestley
7891df6f25 Read arc aliases from all available configuration files
Summary: Fixes T10431. Updates the getAliases() method to read every `arc` configuration file.

Test Plan:
Added an `arc duck` alias to `/etc/arcconfig`, ran `arc duck`, saw "quack".

Added an `arc pig` alias to `~/.arcrc` via `arc alias`, ran `arc pig`, saw "oink oink".

Reviewers: nevogd, chad, #blessed_reviewers

Reviewed By: chad, #blessed_reviewers

Subscribers: eadler, epriestley

Tags: #twitter

Maniphest Tasks: T10431

Differential Revision: https://secure.phabricator.com/D15342
2016-06-06 13:15:59 -07:00
epriestley
a2ab38df78 Improve performance of arc branch in Git with many branches
Summary:
This is mostly just a personal quality-of-life fix. I run this command fairly often and having it return a little faster is nice.

This replaces a `git show` for each individual branch with a big `git for-each-ref` which we were already running anyway. This is quite a bit faster.

This command also occasionally hangs or segfaults for me while executing the huge pile of subprocesses. This is unreliable to reproduce, probably some bug in some PHP extension I have, and likely hard to narrow down, and this approach is better in every way anyway.

Test Plan:
  - Ran `arc branch` in Git, observed faster output (in my `phabricator/`, about 2000ms -> 1200ms).
  - Ran `arc feature` in Mercurial.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15735
2016-04-16 16:39:32 -07:00
Mukunda Modell
737f5c0df9 Allow amending revisions without commandeering first
Summary:
It is common practice in Wikimedia's projects to amend a contributor's
change without taking over authorship of the change. We found that
the only enforcement of commandeering before amending is in arcanist,
not validated server-side. While it would be fairly straightforward to
maintain this as a patch to arcanist, I thought I would see if upstream
is willing to support making this optional.

With this change, amending without commandeering is enabled by a flag in
`.arcconfig` and it defaults to the old behavior.

For background see [wmf T121751](https://phabricator.wikimedia.org/T121751)

Test Plan:
* ran `arc patch D146` to locally apply a revision that I did not author,
* made a trivial change and amended the commit.
* ran `arc diff --update D146 HEAD^` to send the update to differential
* Saw that https://phabricator.wikimedia.org/D146 updated as it should.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: greggrossmeier, aklapper, Luke081515.2, Korvin, dereckson

Maniphest Tasks: T10584

Differential Revision: https://secure.phabricator.com/D15468
2016-04-09 11:57:42 -05:00
epriestley
8701e6c1f3 Strip tips out of commit messages from arc backout
Summary:
Fixes T10707. Currently, `arc backout` creates a commit message which includes questionably-helpful "tips" in the message itself.

Strip these out.

Test Plan:
Used `arc backout` to revert any commit, then `git show` to see the generated message.

  - Before patch: included tips.
  - After patch: no tips.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10707

Differential Revision: https://secure.phabricator.com/D15573
2016-04-02 07:26:02 -07:00
Aviv Eyal
3d7ac867f5 Make callsigns optional in arcanist
Summary:
Remove couple of references to callsigns:
- `arc which` now prints repository name
- `getShouldAmend()` can now use new format of commit name

a quick git-grep looks like the remaining references are all about `repository.callsign` config.

Ref T4245

Test Plan:
- `arc which` on a repository with no callsign
- trigger `requireCleanWorkingCopy()`, see both "Do you want to amend this change" and "Do you want to create a new commit" prompts.
- fire this diff with new code.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D15472
2016-03-15 03:58:46 +00:00
epriestley
ccbaee585e When arc pushes to the staging area, tell Phabricator what we did
Summary:
Ref T10093. Right now, Phabricator kind of guesses that `arc` probably pushed stuff to the staging area.

This can cause confusing/misleading errors later, if it didn't actually push.

Instead, tell Phabricator that we pushed, so we can raise more tailored messages in the web UI (e.g., make "Land Revision" say "this wasn't pushed to the staging area" instead of "whoops, error!!~").

Test Plan:
Ran `arc diff` a few times, then looked in the database for properties.

{F1161655}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10093

Differential Revision: https://secure.phabricator.com/D15426
2016-03-07 07:24:16 -08:00
epriestley
4a1160e0c3 When pushing changes to staging, also push the base commit
Summary:
Fixes T10509. Pushing changes to staging can be inefficient. What happens, roughly, is:

  - Master is at commit "W" -- "W" is the most recent published commit in the main repository.
  - The local working copy has one change on top of that, "X", so its history is commits "A, B, C, D, E, F, ..., U, V, W, X".
  - The remote has some other previous changes that I or other users have made, maybe like "A, B, C, ..., S, T, U, Y" and "A, B, C, ..., T, U, V, Z", from previous pushes to staging areas.
  - "X", "Y" and "Z" will never actually make it to master, because they'll be squash-merged/amended by `arc land`.

So the local says "I want to push 'X'", and the remote says "I know about 'Y' and 'Z', are those in the history of 'W'? You only need to send me new stuff if they are".

But they aren't, so the local says "nope, so here's the whole history for you". This is slow and sends a ton of data that the remote already has over the network.

In theory, Git could use a slightly different algorithm to tell the local about more commits, but this is hard, rarely useful, and not the kind of thing I'd be excited about changing if I was the Git upstream.

Instead, when pushing "X", also push "W", to trick Git into telling future clients about it.

Now, the remote should say "I know about 'W', 'Y' and 'Z'", and the local will say "oh, great, 'W' is in history, here's just the changes since then".

Also, fail `arc diff` if the push to staging fails, and tell users to use `--skip-staging`. This code has been in production for a while and doesn't seem to have any issues, and a failed push to staging prevents builds, lands, etc.

Test Plan:
  - Ran `arc diff`, saw two changes push.
  - Ran `arc diff --base arc:empty`, saw only one change push.
  - Ran `arc diff` with an intentionally broken staging area, saw an error.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10509

Differential Revision: https://secure.phabricator.com/D15424
2016-03-07 06:53:49 -08:00
epriestley
3876d93583 Properly URL encode branches in arc browse --branch ...
Summary: Fixes T10511. If you `arc browse --branch x/y/z`, we do not encode the URI properly.

Test Plan:
Ran `arc browse --branch x/y/z/ something.c`.

Before, got an error about "x" does not exist. This is wrong; the error should be about "x/y/z".

After, got the proper error:

{F1141096}

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T10511

Differential Revision: https://secure.phabricator.com/D15397
2016-03-04 15:52:58 -08:00
epriestley
b1de04aa68 Report unit test details from Arcanist to Harbormaster
Summary: Ref T10457. Provides information for D15363.

Test Plan: See D15363.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9951, T10457

Differential Revision: https://secure.phabricator.com/D15364
2016-03-04 15:49:46 -08:00
epriestley
98d71571e4 Fix arc diff --raw with "onto" target properties
Summary:
Currently, `git show | arc diff --raw` and similar doesn't work because we try to figure out what the "Branch: feature (branched from whatever)" value is, which doesn't make sense.

```
$ git show | arc diff --raw --trace
 ARGV  '/Users/epriestley/dev/core/lib/arcanist/bin/../scripts/arcanist.php' 'diff' '--raw' '--trace'
 LOAD  Loaded "phutil" from "/Users/epriestley/dev/core/lib/libphutil/src".
 LOAD  Loaded "arcanist" from "/Users/epriestley/dev/core/lib/arcanist/src".
Config: Reading user configuration file "/Users/epriestley/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/Users/epriestley/dev/core/lib/arcanist/.arcconfig".
Working Copy: Path "/Users/epriestley/dev/core/lib/arcanist" is part of `git` working copy "/Users/epriestley/dev/core/lib/arcanist".
Working Copy: Project root is at "/Users/epriestley/dev/core/lib/arcanist".
Config: Reading local configuration file "/Users/epriestley/dev/core/lib/arcanist/.git/arc/config"...
Loading phutil library from '/Users/epriestley/dev/core/lib/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 489>
>>> [1] <http> https://secure.phabricator.com/api/conduit.connect
<<< [1] <http> 211,217 us
<<< [0] <conduit> 212,001 us
>>> [2] <event> diff.didCollectChanges <listeners = 0>
<<< [2] <event> 140 us
>>> [3] <event> diff.didBuildMessage <listeners = 0>
<<< [3] <event> 46 us
Reading diff from stdin...
>>> [4] <conduit> differential.creatediff() <bytes = 10,542>
>>> [5] <http> https://secure.phabricator.com/api/differential.creatediff
<<< [5] <http> 120,215 us
<<< [4] <conduit> 120,411 us
>>> [6] <event> diff.wasCreated <listeners = 0>
<<< [6] <event> 41 us
 SKIP STAGING  Raw changes can not be pushed to a staging area.
>>> [7] <conduit> harbormaster.queryautotargets() <bytes = 290>
>>> [8] <http> https://secure.phabricator.com/api/harbormaster.queryautotargets
<<< [8] <http> 217,717 us
<<< [7] <conduit> 217,944 us
>>> [9] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [10] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
>>> [11] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [12] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
<<< [10] <http> 123,821 us
<<< [9] <conduit> 134,329 us
<<< [12] <http> 227,580 us
<<< [11] <conduit> 227,787 us

[2016-01-05 10:08:58] EXCEPTION: (Exception) This workflow ('ArcanistDiffWorkflow') requires a Repository API, override requiresRepositoryAPI() to return true. at [<arcanist>/src/workflow/ArcanistWorkflow.php:804]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=stable, ref.master=adb8a9c074ba, ref.stable=7b8d38cd2d4e)
  #0 ArcanistWorkflow::getRepositoryAPI() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2421]
  #1 ArcanistDiffWorkflow::getDiffOntoTargets() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2411]
  #2 ArcanistDiffWorkflow::updateOntoDiffProperty() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:534]
  #3 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:392]
```

Test Plan: Ran `arc diff --raw` in `phabricator/`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14946
2016-01-05 10:16:39 -08:00
epriestley
8762e3f367 Use "--whitespace nowarn" in arc patch to respect trailing whitespace
Summary: Fixes T10008. Git tries to fix some issues by default (apparently? empirically; not consistent with documentation, I think?), but patches from `arc patch` are "always" accurate (disregarding other bugs we might have -- basically, they haven't been emailed or copy/pasted or anything like that) so we can just tell it to apply the patch exactly as-is.

Test Plan: {F1029182}

Reviewers: chad, joshuaspence

Reviewed By: chad, joshuaspence

Maniphest Tasks: T10008

Differential Revision: https://secure.phabricator.com/D14816
2015-12-17 17:36:26 -08:00
epriestley
74c7495b1a Clarify that "arc land" means it is merging changes, not branch refences
Summary: Ref T9973. Make this language unambiguously clear about the underlying operations.

Test Plan: Ran `arc help land`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9973

Differential Revision: https://secure.phabricator.com/D14754
2015-12-12 10:25:12 -08:00
epriestley
dae2f0073f In arc diff, try to guess where a change should land
Summary:
Ref T9952. Ref T3462. My primary goal is to improve prefilling of the "Onto Branch:" field in the "Land Revision" dialog.

When uploading a diff with `arc diff`, add a property with some information about which branch to target. In particular:

  - If the local branch tracks an upstream branch (or tracks something which tracks something which tracks the upstream), target that.
  - If not, but "arc.land.onto.default" is set, target that.

This doesn't try to guess in other cases, since they're more involved. I'll add some context about this in T3462.

I don't //love// using "diff properties" for this, but it doesn't make cleaning them up any harder since we already use it for other stuff which isn't going away (lint/unit excuses).

Test Plan:
  - Added some `var_dump()` and used `arc diff --only` to generate diffs.
  - Saw upstream tracking and config-based rules generate reasonable values and submit them.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T3462, T9952

Differential Revision: https://secure.phabricator.com/D14736
2015-12-10 15:24:38 -08:00
Joshua Spence
b52e9dc702 Linter fixes
Summary: Minor linter fixes

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14633
2015-12-07 21:35:34 +11:00
epriestley
4a680c762b Tailor CLI feedback about "arc alias" to describe shell command aliases
Summary:
Fixes T9873. Instead of printing something like this:

> Aliased "arc ls" to "arc !ls".

...print this:

> Aliased "arc ls" to shell command "ls".

Test Plan:
  - Added shell commands and internal aliases.
  - Removed aliases.
  - Listed aliases.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9873

Differential Revision: https://secure.phabricator.com/D14651
2015-12-03 18:31:48 -08:00
Joshua Spence
5218ec357c Add a table showing all XHPAST linter rules to the output of arc linters xhpast
Summary: Now that we have 91 subclasses of `ArcanistXHPASTLinterRule`, it is becoming difficult to manage the IDs. This diff adds a table to `arc linters xhpast` workflow to facilitate this.

Test Plan:
```
> arc xhpast-linter-rules

+=====+=======================================================+===================================================================+
| ID  | Class                                                 | Name                                                              |
+=====+=======================================================+===================================================================+
| 1   | ArcanistSyntaxErrorXHPASTLinterRule                   | PHP Syntax Error!                                                 |
| 2   | ArcanistUnableToParseXHPASTLinterRule                 | Unable to Parse                                                   |
| 3   | ArcanistVariableVariableXHPASTLinterRule              | Use of Variable Variable                                          |
| 4   | ArcanistExtractUseXHPASTLinterRule                    | Use of `extract`                                                  |
| 5   | ArcanistUndeclaredVariableXHPASTLinterRule            | Use of Undeclared Variable                                        |
| 6   | ArcanistPHPShortTagXHPASTLinterRule                   | Use of Short Tag `<?`                                             |
| 7   | ArcanistPHPEchoTagXHPASTLinterRule                    | Use of Echo Tag `<?=`                                             |
| 8   | ArcanistPHPCloseTagXHPASTLinterRule                   | Use of Close Tag `?>`                                             |
| 9   | ArcanistNamingConventionsXHPASTLinterRule             | Naming Conventions                                                |
| 10  | ArcanistImplicitConstructorXHPASTLinterRule           | Implicit Constructor                                              |
| 12  | ArcanistDynamicDefineXHPASTLinterRule                 | Dynamic `define`                                                  |
| 13  | ArcanistStaticThisXHPASTLinterRule                    | Use of `$this` in Static Context                                  |
| 14  | ArcanistPregQuoteMisuseXHPASTLinterRule               | Misuse of `preg_quote`                                            |
| 15  | ArcanistPHPOpenTagXHPASTLinterRule                    | Expected Open Tag                                                 |
| 16  | ArcanistTodoCommentXHPASTLinterRule                   | TODO Comment                                                      |
| 17  | ArcanistExitExpressionXHPASTLinterRule                | `exit` Used as Expression                                         |
| 18  | ArcanistCommentStyleXHPASTLinterRule                  | Comment Style                                                     |
| 19  | ArcanistClassFilenameMismatchXHPASTLinterRule         | Class-Filename Mismatch                                           |
| 20  | ArcanistTautologicalExpressionXHPASTLinterRule        | Tautological Expression                                           |
| 21  | ArcanistPlusOperatorOnStringsXHPASTLinterRule         | Not String Concatenation                                          |
| 22  | ArcanistDuplicateKeysInArrayXHPASTLinterRule          | Duplicate Keys in Array                                           |
| 23  | ArcanistReusedIteratorXHPASTLinterRule                | Reuse of Iterator Variable                                        |
| 24  | ArcanistBraceFormattingXHPASTLinterRule               | Brace Placement                                                   |
| 25  | ArcanistParenthesesSpacingXHPASTLinterRule            | Spaces Inside Parentheses                                         |
| 26  | ArcanistControlStatementSpacingXHPASTLinterRule       | Space After Control Statement                                     |
| 27  | ArcanistBinaryExpressionSpacingXHPASTLinterRule       | Space Around Binary Operator                                      |
| 28  | ArcanistArrayIndexSpacingXHPASTLinterRule             | Spacing Before Array Index                                        |
| 30  | ArcanistImplicitFallthroughXHPASTLinterRule           | Implicit Fallthrough                                              |
| 32  | ArcanistReusedAsIteratorXHPASTLinterRule              | Variable Reused As Iterator                                       |
| 34  | ArcanistCommentSpacingXHPASTLinterRule                | Comment Spaces                                                    |
| 36  | ArcanistSlownessXHPASTLinterRule                      | Slow Construct                                                    |
| 37  | ArcanistCallParenthesesXHPASTLinterRule               | Call Formatting                                                   |
| 38  | ArcanistDeclarationParenthesesXHPASTLinterRule        | Declaration Formatting                                            |
| 39  | ArcanistReusedIteratorReferenceXHPASTLinterRule       | Reuse of Iterator References                                      |
| 40  | ArcanistKeywordCasingXHPASTLinterRule                 | Keyword Conventions                                               |
| 41  | ArcanistDoubleQuoteXHPASTLinterRule                   | Unnecessary Double Quotes                                         |
| 42  | ArcanistElseIfUsageXHPASTLinterRule                   | `elseif` Usage                                                    |
| 43  | ArcanistSemicolonSpacingXHPASTLinterRule              | Semicolon Spacing                                                 |
| 44  | ArcanistConcatenationOperatorXHPASTLinterRule         | Concatenation Spacing                                             |
| 45  | ArcanistPHPCompatibilityXHPASTLinterRule              | PHP Compatibility                                                 |
| 46  | ArcanistLanguageConstructParenthesesXHPASTLinterRule  | Language Construct Parentheses                                    |
| 47  | ArcanistEmptyStatementXHPASTLinterRule                | Empty Block Statement                                             |
| 48  | ArcanistArraySeparatorXHPASTLinterRule                | Array Separator                                                   |
| 49  | ArcanistConstructorParenthesesXHPASTLinterRule        | Constructor Parentheses                                           |
| 50  | ArcanistDuplicateSwitchCaseXHPASTLinterRule           | Duplicate Case Statements                                         |
| 51  | ArcanistBlacklistedFunctionXHPASTLinterRule           | Use of Blacklisted Function                                       |
| 52  | ArcanistImplicitVisibilityXHPASTLinterRule            | Implicit Method Visibility                                        |
| 53  | ArcanistCallTimePassByReferenceXHPASTLinterRule       | Call-Time Pass-By-Reference                                       |
| 54  | ArcanistFormattedStringXHPASTLinterRule               | Formatted String                                                  |
| 55  | ArcanistUnnecessaryFinalModifierXHPASTLinterRule      | Unnecessary Final Modifier                                        |
| 56  | ArcanistUnnecessarySemicolonXHPASTLinterRule          | Unnecessary Semicolon                                             |
| 57  | ArcanistSelfMemberReferenceXHPASTLinterRule           | Self Member Reference                                             |
| 58  | ArcanistLogicalOperatorsXHPASTLinterRule              | Logical Operators                                                 |
| 59  | ArcanistInnerFunctionXHPASTLinterRule                 | Inner Functions                                                   |
| 60  | ArcanistDefaultParametersXHPASTLinterRule             | Default Parameters                                                |
| 61  | ArcanistLowercaseFunctionsXHPASTLinterRule            | Lowercase Functions                                               |
| 62  | ArcanistClassNameLiteralXHPASTLinterRule              | Class Name Literal                                                |
| 63  | ArcanistUselessOverridingMethodXHPASTLinterRule       | Useless Overriding Method                                         |
| 64  | ArcanistNoParentScopeXHPASTLinterRule                 | No Parent Scope                                                   |
| 65  | ArcanistAliasFunctionXHPASTLinterRule                 | Alias Functions                                                   |
| 66  | ArcanistCastSpacingXHPASTLinterRule                   | Cast Spacing                                                      |
| 67  | ArcanistToStringExceptionXHPASTLinterRule             | Throwing Exception in `__toString` Method                         |
| 68  | ArcanistLambdaFuncFunctionXHPASTLinterRule            | `__lambda_func` Function                                          |
| 69  | ArcanistInstanceOfOperatorXHPASTLinterRule            | `instanceof` Operator                                             |
| 70  | ArcanistInvalidDefaultParameterXHPASTLinterRule       | Invalid Default Parameter                                         |
| 71  | ArcanistModifierOrderingXHPASTLinterRule              | Modifier Ordering                                                 |
| 72  | ArcanistInvalidModifiersXHPASTLinterRule              | Invalid Modifiers                                                 |
| 73  | ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule  | Space After Unary Prefix Operator                                 |
| 74  | ArcanistObjectOperatorSpacingXHPASTLinterRule         | Object Operator Spacing                                           |
| 75  | ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule | Space Before Unary Postfix Operator                               |
| 76  | ArcanistArrayValueXHPASTLinterRule                    | Array Element                                                     |
| 77  | ArcanistListAssignmentXHPASTLinterRule                | List Assignment                                                   |
| 78  | ArcanistInlineHTMLXHPASTLinterRule                    | Inline HTML                                                       |
| 79  | ArcanistGlobalVariableXHPASTLinterRule                | Global Variables                                                  |
| 80  | ArcanistParseStrUseXHPASTLinterRule                   | Questionable Use of `parse_str`                                   |
| 81  | ArcanistNewlineAfterOpenTagXHPASTLinterRule           | Newline After PHP Open Tag                                        |
| 82  | ArcanistEmptyFileXHPASTLinterRule                     | Empty File                                                        |
| 83  | ArcanistParentMemberReferenceXHPASTLinterRule         | Parent Member Reference                                           |
| 84  | ArcanistArrayCombineXHPASTLinterRule                  | `array_combine()` Unreliable                                      |
| 85  | ArcanistDeprecationXHPASTLinterRule                   | Use of Deprecated Function                                        |
| 86  | ArcanistUnsafeDynamicStringXHPASTLinterRule           | Unsafe Usage of Dynamic String                                    |
| 87  | ArcanistRaggedClassTreeEdgeXHPASTLinterRule           | Class Not `abstract` Or `final`                                   |
| 88  | ArcanistClassExtendsObjectXHPASTLinterRule            | Class Not Extending `Phobject`                                    |
| 90  | ArcanistNestedNamespacesXHPASTLinterRule              | Nested `namespace` Statements                                     |
| 91  | ArcanistThisReassignmentXHPASTLinterRule              | `$this` Reassignment                                              |
| 92  | ArcanistUnexpectedReturnValueXHPASTLinterRule         | Unexpected `return` Value                                         |
| 97  | ArcanistUseStatementNamespacePrefixXHPASTLinterRule   | `use` Statement Namespace Prefix                                  |
| 99  | ArcanistUnnecessarySymbolAliasXHPASTLinterRule        | Unnecessary Symbol Alias                                          |
| 107 | ArcanistAbstractPrivateMethodXHPASTLinterRule         | `abstract` Method Cannot Be Declared `private`                    |
| 108 | ArcanistAbstractMethodBodyXHPASTLinterRule            | `abstract` Method Cannot Contain Body                             |
| 113 | ArcanistClassMustBeDeclaredAbstractXHPASTLinterRule   | `class` Containing `abstract` Methods Must Be Declared `abstract` |
+=====+=======================================================+===================================================================+
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14563
2015-12-02 06:27:33 +11:00
epriestley
4f1141d0c5 Improve error handling in arc install-certificate
Summary: Fixes T9858. Reasonable typos and misunderstandings currently produce very confusing error messages.

Test Plan:
```
$ arc install certificate
Usage Exception: Server URI "certificate" must include a protocol and domain. It should be in the form "https://phabricator.example.com/".
```

  - Also used a good URI.
  - Also used no URI.

Reviewers: joshuaspence, chad

Reviewed By: chad

Maniphest Tasks: T9858

Differential Revision: https://secure.phabricator.com/D14577
2015-11-27 09:05:50 -08:00
epriestley
b32149495b Don't give Mercurial empty string as a remote name
Summary:
Fixes T9807. We currently run commands like this in some cases:

  hg push -r master ''

From T9807, it seems that older Mercurial treated `''` in the same way it would treat no argument, while newer Mercurial does not.

Passing `''` is unusual and not intended.

Test Plan:
From T9807, @cspeckmim confirmed that running this command without the `''` works, and @jgelgens tested the patch itself.

I didn't actually run this code myself, since I don't have Mercurial 3.6.1 installed and the fix seems straightfoward.

Reviewers: chad

Reviewed By: chad

Subscribers: cspeckmim

Maniphest Tasks: T9807

Differential Revision: https://secure.phabricator.com/D14531
2015-11-21 09:54:05 -08:00
Joshua Spence
66ab1c955d Remove arguments from unit test engines
Summary: Ref T9131. This doesn't seem to be used... it seems like it is a relic of postponed test results.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T9131

Differential Revision: https://secure.phabricator.com/D14487
2015-11-15 20:04:59 +00:00
Joshua Spence
2db40f9953 Various translation improvements
Summary: Depends on D14070.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14072
2015-11-02 21:31:04 +11:00
epriestley
2a2fd6e338 Pull git upstream-path logic into a separate class
Summary:
Ref T9661. I need to reuse this to fix the complex workflow described in T9661 where we need to follow multiple paths to the upstream and cascade updates across them.

Pull the logic into a separate class to make this easier and less copy/pastey.

This shouldn't change any behavior.

Test Plan: Ran `arc land --preview` from detached head, remote-tracking branch, non-tracking branch, local-tracking branch. Selection of target/remote seemed correct in all cases.

Reviewers: chad

Reviewed By: chad

Subscribers: edibiase

Maniphest Tasks: T9661

Differential Revision: https://secure.phabricator.com/D14360
2015-10-28 13:19:30 -07:00
epriestley
aa5c023fe8 Make rules for guessing onto/remote more powerful and more explicit in arc land
Summary:
Fixes T9543. Fixes T9658. Ref T3855.

Major functional change is that you can have a sequence of branches like:

  origin/master -> notmaster -> feature1

...where they track each other, but you named your local master something else. Currently, we resolve only one level of upstreams, so we try to land onto "notmaster" in this case, which is wrong.

Instead, keep resolving upstreams until we either hit a cycle, don't have another upstream to look at, or find someting in a remote. In this case we'll eventually find "origin/master" and select "origin" as the remote and "master" as the target.

Other minor changes:

  - Make this selection process explicit.
  - Make the help 3000x longer.

Also fix a bug where we could incorrectly try to tell Differential to update awith `--preview`.

Test Plan:
  - Landed from a tag.
  - Landed from a tracking branch.
  - Landed from an nth-degree tracking branch.
  - Tried to land from a local branch with a cycle in upstreams.
  - Landed with --remote and --onto.
  - Read `arc help land`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9658, T3855, T9543

Differential Revision: https://secure.phabricator.com/D14357
2015-10-28 09:37:17 -07:00
epriestley
a03c6079bb Make "arc land" great again
Summary:
Ref T3855. Fixes T9537. Fixes T8620. Fixes T4333.

This declares bankruptcy and replaces the entire `arc land` workflow under Git. These are the notable changes:

  - (T3855) You can now land from a branch to itself.
  - (T3855) We now try to restore the original state very aggressively after any failure, instead of dumping you into the middle of a mess.
  - (T9537) You can now land without a local branch.
  - ([not actually] T9543) We'll now ignore the local branch if it just happens to be named the same thing as the remote branch but doesn't actually track it.
  - (T8620) You can now land from a detached HEAD.
  - (T4333) We now preserve the author and author date of whatever you land.

This may need some followup work. In particular:

  - The signal handler (that tries to put you in a better place if you ^C in the middle of things) causes ^C to work awkwardly in prompts. This might not be worth it.
  - Errors/instructions on push/merge issues might need work.
  - I dropped support for `--delete-remote` and `--update-with-blah-blah` because I think these flags aren't worth their complexity.
  - I've simplified the update/merge algorithm a bit. It may need some complexity added back in.
  - I probably missed a few things because this covers like 200 unique, creative workflows.
  - Users might need more guidance on the workflows that drop them in the middle of nowhere if they manage to reach them more often than I think.

Test Plan:
  - Used `arc land` to land like at least 15,000 different kinds of changes.
  - Landed normally.
  - Landed from a branch onto itself.
  - Landed from a detached head.
  - Landed nothing.
  - Landed with no local branch.
  - Landed onto made-up branches.
  - Landed with bad targets.
  - ^C'd things in the middle.

Reviewers: chad

Reviewed By: chad

Subscribers: tycho.tatitscheff

Maniphest Tasks: T3855, T4333, T8620, T9537, T9543

Differential Revision: https://secure.phabricator.com/D14356
2015-10-28 07:04:57 -07:00
epriestley
dfde57ff81 Improve two error handling behaviors in arc upgrade
Summary:
Fixes T9222. Two issues here:

  - First, we currently continue on error. Throw instead. I just swapped us from "phutil_passthru()" to "execx()" since I don't think printing out the "pulling from remote..." status messages is very important, and this makes it easier to raise a useful exception.
  - Second, if you have a dirty working copy we currently may try to do some sort of silly stuff which won't work, like prompt you to amend changes. Instead, do a slightly lower-level check and just bail.

Test Plan:
  - Ran `arc upgrade` with a dirty working copy and got a tailored, useful error.
  - Ran `arc upgrade` with an artificially bad `git pull` command, got a failure with a specific error message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9222

Differential Revision: https://secure.phabricator.com/D14317
2015-10-22 19:55:16 +00:00
Aviv Eyal
6c7def560d Remove ArcanistWorkflow::setWorkingCopy
Summary: Fixes T9583. setWorkingCopy doesn't actually work, so eliminate it.

Test Plan: arc unit --everything

Reviewers: chad, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T9583

Differential Revision: https://secure.phabricator.com/D14293
2015-10-16 09:53:01 -07:00
epriestley
d3abb65572 Add a --target flag to arc unit for uploading results
Summary: Ref T5821. Basic idea here is that Harbormaster can run `arc unit --everything --target ...` to get a build target updated.

Test Plan: Ran `arc unit --target ...`, saw web UI update with test results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5821

Differential Revision: https://secure.phabricator.com/D14190
2015-09-29 09:51:51 -07:00
epriestley
9c056c5cc8 Improve arc's handling of dirty submodules in Git
Summary:
Fixes T9455. Depends on D14136. When you have a dirty submodule:

  $ nano submodule/file.c # save changes

...we currently ask you to make a commit when you run `arc diff`, which is meaningless and misleading.

Instead, prompt the user separately.

This behavior isn't perfect but I think it's about the best we can do within reason.

Test Plan:
  - Ran `arc diff` in a working copy with uncommitted submodule changes only, got new prompt.
  - Ran `arc diff` in a working copy with submodule base commit changes only, got old (correct) prompt.
  - Ran `arc diff` in a working copy with both, got only old prompt (which is incomplete, but reasonable/meaningful).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9455

Differential Revision: https://secure.phabricator.com/D14137
2015-09-21 12:40:06 -07:00
Aviv Eyal
bdab422440 arc linters: switch name and short
Summary: I think they were wrong.

Test Plan: `arc linters nolint`. It would fail without it.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, #lint

Differential Revision: https://secure.phabricator.com/D14084
2015-09-08 10:39:26 -07:00
Aviv Eyal
8f3a002cdb Allow upgrading in branch stable
Summary: close T9043. This still allows for one dir to be in `master` and the other in `stable`, but hopefully that's not going to be a problem.

Test Plan: Clone arc/libph, checkout `stable`, run arc upgrade.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: cburroughs, epriestley

Maniphest Tasks: T9043

Differential Revision: https://secure.phabricator.com/D14076
2015-09-07 13:44:48 -07:00
epriestley
55d9cc7013 Improve temporary file upload API and add viewPolicy support
Summary: Ref T7148. In D14056, I let `arc upload` upload temporary files, but this is a better way to do some of the internals. Also add support for setting a `viewPolicy`.

Test Plan: Used `arc upload`, `arc upload --temporary`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7148

Differential Revision: https://secure.phabricator.com/D14075
2015-09-07 12:44:59 -07:00
epriestley
9896908685 Add temporary file support to ArcanistFileUploader
Summary: Ref T7148. Depends on D14055. Allows files to be marked as temporary when uploaded.

Test Plan: Used `arc upload --temporary` to upload temporary files.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7148

Differential Revision: https://secure.phabricator.com/D14056
2015-09-04 10:34:14 -07:00
Alex Vandiver
7e677c27ec Fix callsites which called libphutil_console_wrap like it were _format
Summary:
7e2df9a attempted to pht() some strings; unfortunately, it assumed
that some things that were calls to phutil_console_wrap() were
actually calls to phutil_console_format().  This produces errors of
the form:

    [2015-07-17 21:17:28] ERROR 2: str_repeat() expects parameter 2 to be long, string given at [/usr/local/libphutil/src/console/format.php:162]
    #0 str_repeat(string, string) called at [<phutil>/src/console/format.php:162]
    #1 phutil_console_wrap(string, string, string) called at [<arcanist>/scripts/arcanist.php:620]
    #2 arcanist_load_libraries(array, boolean, string, ArcanistWorkingCopyIdentity) called at [<arcanist>/scripts/arcanist.php:154]
    %s: %s

Provide an additional call to phutil_console_format() when necessary,
or simply append the relevant characters if possible.

Test Plan: Caused a library load error

Reviewers: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14053
2015-09-03 12:01:01 -07:00
cburroughs
029e5a7c29 staging repo compatibility for older git versions
Summary:
The `--no-verify` flag was not added until git 1.8.2.  This
flag is used to avoid running local pre-push hooks.  This is likely a
rare configuration and is safe to omit the flag on older versions.
Users with local pre-push hooks **and** older git version may need to
adjust their workfow.

fixes T9310

Test Plan:
 * Ran `arc diff` with my real git 1.7.10.4 and succeeded with `STAGING
   PUSHED`.
 * Edited `getGitVersion` to be > 1.8.2 and pushed again.  Got
   `STAGING FAILED` because `error: unknown option`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T9310

Differential Revision: https://secure.phabricator.com/D14033
2015-09-02 06:58:42 -07:00
Joshua Spence
10f9c460fa Remove leftover code for "postponed" lint and unit status
Summary: Ref T9134. It looks like this functionality was removed in D13848. Depends on D13869.

Test Plan: Ran `arc diff`, `arc lint` and `arc unit`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T9134

Differential Revision: https://secure.phabricator.com/D13868
2015-08-30 21:56:12 +10:00
Aviv Eyal
1ed98937c4 arc linters: Filter by name, make display one-line
Summary: allow searching/filtering linters displayed by name.

Test Plan:
`$ arc linters --verbose --search JSon --search ruby` (Lots of text about many linters)
`$ arc linters --search JSon ruby` (error message)
`$ arc linters python` (no results, "try --search").
`$ arc linters ruby` (Verbose text about the "ruby" linter).

Reviewers: joshuaspence, #blessed_reviewers, epriestley

Reviewed By: joshuaspence, #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D10706
2015-08-29 05:10:03 -07:00
Javier Arteaga
f47d15387b Fix wrong plural of an arc land message
Summary:
"Branch" was pluralized as "branchs".

Fixes T9225.

Test Plan:
* Created test repo with two revisions on a feature branch.
* Saw old message, frowned a little.
* Applied patch.
* No longer frowning.

Reviewers: chad, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: avivey, Korvin

Maniphest Tasks: T9225

Differential Revision: https://secure.phabricator.com/D13944
2015-08-20 04:19:22 -07:00
Aiden Scandella
807057087d Restore default "yes" behavior for lint patch
Summary:
Fixes T9029

See T9029 for more details, but basically at some point phutil_console_confirm
(which takes a `$default_no` parameter) was refactored to `$console->confirm()`
which takes a `$default` parameter with semantics negated..

Test Plan:
Run `arc lint` on a repository where patch is suggested. Default
option should be "[Y/n]" to accept the patch.

Reviewers: joshuaspence, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T9029

Differential Revision: https://secure.phabricator.com/D13873
2015-08-11 14:49:56 -07:00
Joshua Spence
e4caf1a7d9 Automatically use the configuration driven unit test engine
Summary: Ref T5568. Use the `ArcanistConfigurationDrivenUnitTestEngine` automatically, if an `.arcunit` file exists. This behavior mimics the auto-detection for the configuration driven lint engine.

Test Plan:
Ran through the following scenarios:

  - Ran `arc unit` and saw unit tests execute.
  - Ran `arc unit --engine PhutilUnitTestEngine`
  - Remove the `.arcunit` file and ran `arc unit`... got an exception.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T5568

Differential Revision: https://secure.phabricator.com/D13855
2015-08-11 07:55:57 +10:00
Joshua Spence
0e09578720 Use PhutilClassMapQuery instead of PhutilSymbolLoader
Summary: Use `PhutilClassMapQuery` instead of `PhutilSymbolLoader`, mainly for consistency. Depends on D13572.

Test Plan: This hasn't been tested very comphrehensively as of yet.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13573
2015-08-11 06:50:47 +10:00
Joshua Spence
c304c4e045 Minor linter fix
Summary: Self-explanatory.

Test Plan: Eyeball it.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D13807
2015-08-08 10:18:59 +10:00
Yomi
b7e87e959c Fix typo. Closes T8915.
Summary:
From 'or' to 'are'.
Closes T8915.

Test Plan: Wait 30 seconds.

Reviewers: joshuaspence, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T8915

Differential Revision: https://secure.phabricator.com/D13664
2015-07-21 11:14:39 -07:00
Joshua Spence
407954dddd Output lint XML results to a file
Summary:
Ref T8332. I find it really odd that I need to run `arc lint --everything --output xml > checkstyle.xml` and feel that it would be much more intuitive to just run `arc lint --everything --output xml` and have the output written to `checkstyle.xml`.

To provide context, we are running `arc lint --everything` in a CI job and parsing the results.

Test Plan: Ran `arc lint --everything --output xml` and saw the output written to file.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T8332

Differential Revision: https://secure.phabricator.com/D13570
2015-07-15 06:58:41 +10:00
epriestley
3d16595c07 Update references to "www.phabricator.com" in Arcanist
Summary: This host is no longer in service.

Test Plan: `git grep -i www.phabricator.com`

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D13586
2015-07-07 13:34:56 -07:00
epriestley
d54cb072fa Raise a more tailored error message when a third-party test engine returns bad results
Summary: Fixes T8714. When a test engine isn't returning the correct result type, shift suspicion onto it.

Test Plan: Faked error, got exception blaming test engine.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8714

Differential Revision: https://secure.phabricator.com/D13486
2015-07-01 04:48:52 -07:00
epriestley
29839e8c72 Don't fail lint builds for "advice"; make lint/unit upload failures louder
Summary:
Ref T8670. Ref T8657.

  - When lint only has advice (no warnings/errors), consider it a "passing" build.
  - Be a little louder about `sendmessage` calls failing because this stuff is not totally broken and that makes T8670-related things easier to catch/fix.

Test Plan: Created this revision.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8657, T8670

Differential Revision: https://secure.phabricator.com/D13436
2015-06-25 10:05:54 -07:00