1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2025-02-23 12:09:09 +01:00
Commit graph

146 commits

Author SHA1 Message Date
epriestley
73f5afd441 Remove "very large change" warning from Arcanist
Summary: Ref T13110. We now degrade very large changes and I'm not convinced any user ever entered "n" at this prompt.

Test Plan: Ran `arc diff` to create this very revision.

Maniphest Tasks: T13110

Differential Revision: https://secure.phabricator.com/D19299
2018-04-05 06:50:57 -07:00
Stephanie Ren
165df12046 Consistently use phutil_console_confirm().
Summary:
`PhutilConsole->confirm()` is vestigial as noted in efcd70c, as
`PhutilConsole` may be deprecated sometime in the future. `PhutilConsole`
was developed as a tool for T4281 but the feature has been removed
since.

Replace existing occurences of the `PhutilConsole->confirm()` pattern with
`phutil_console_confirm()`. There should be no change in functionality
since the two functions are interchangeable.

Test Plan: Manually tested by running `bin/arc lint`, `bin/arc diff --preview`, `bin/arc land --preview`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, alexmv

Differential Revision: https://secure.phabricator.com/D18185
2017-07-06 16:27:36 -07:00
epriestley
ade25facfd Fix a bad interaction between "arc diff --reviewers" and "the first line of a message is always a title"
Summary:
Fixes T12069. We implement "arc diff --reviewers" (and "--cc") by parsing a faux message with "Reviewers: ...".

After D17122, the first line of the message is always interpreted as a title, so the text ends up in the message body.

Instead, use a placeholder title so these fields are never initial fields.

Test Plan: Ran `arc diff --reviewers dog`, got only one "Reviewers" field.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12069

Differential Revision: https://secure.phabricator.com/D17147
2017-01-05 14:35:39 -08:00
Luke081515
3ae0cc8d41 Avoid double [y/N] when updating a not owned revision
Summary:
Since 737f5c0df9 arcanist shows [y/N] two times, when arcanist asks you, if you want to proceed, if you want to update a not owned revision. Ths patch fixes
that, so that Arcanist shows [y/N] only once, like at other situations, when Arcanist asks you a question.
Ref T11489

Test Plan: Updated a not owned revision.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Tags: #arcanist

Maniphest Tasks: T11489

Differential Revision: https://secure.phabricator.com/D16415
2016-08-17 10:36:16 -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
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
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
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
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
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
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
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
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
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
epriestley
b697a3b80b Try to ship lint and unit results to Harbormaster autotargets
Summary:
Ref T8095. Before uploading lint/unit results in the old way, try to ship them to autotargets.

If we can query and upload data to autotargets, do so, and then skip the older style uploads.

Test Plan: Used `arc diff` to ship data up via Harbormaster.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

Differential Revision: https://secure.phabricator.com/D13381
2015-06-23 10:22:57 -07:00
epriestley
3ac80200e2 Push to staging areas when running "arc diff"
Summary: Ref T8238. If a staging area is configured for a repository (see D13019), push a copy of changes to it after creating a diff.

Test Plan: Ran `arc diff` with various options, saw applicable changes get pushed to staging.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: cburroughs, epriestley

Maniphest Tasks: T8238

Differential Revision: https://secure.phabricator.com/D13020
2015-05-27 10:27:03 -07:00
epriestley
5c7b22e620 Use file.allocate to upload large files from arc diff
Summary: Fixes T8259. Depends on D13016. Use modern logic to support large file uploads.

Test Plan:
  - Diffed with some images, saw them show up in the diff.
  - Diffed with a 12MB binary, saw it upload in chunks.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8259

Differential Revision: https://secure.phabricator.com/D13017
2015-05-27 10:26:12 -07:00
Nevogd
407af00ef6 Fix invalid parameter in phutil_console_wrap()
Summary:
Fixes T8314. Change the phutil_console_wrap() call to only have a single
string parameter.

Test Plan:
Ran `arc diff` added text into the title, summary and test plan fields,
but did not specify any reviewers. When prompted to continue, clicked 'No' and
saw the '(Message saved to commit message.)' string.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T8314

Differential Revision: https://secure.phabricator.com/D13015
2015-05-26 06:07:25 -07:00
Joshua Spence
410331db75 Fix some format strings
Summary: These `pht` calls use `%d` instead of `%s`.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12994
2015-05-25 21:24:40 +10:00
Joshua Spence
6f86866104 phtize a bunch more strings
Summary: I found a few strings that I had missed, using a mostly-broken-but-somewhat-okay custom linter ruler (https://secure.phabricator.com/differential/diff/30988/).

Test Plan: Intense eyeballing.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aurelijus, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12888
2015-05-22 17:09:56 +10:00
Joshua Spence
9edcaadb2a Remove an unused variable
Summary: Ref T7604. Remove an unused variable from `ArcanistDiffWorkflow`.

Test Plan: This diff, I guess.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7604

Differential Revision: https://secure.phabricator.com/D12972
2015-05-22 17:09:18 +10:00
Joshua Spence
e3311f0832 Remove "arcanist projects" from arcanist
Summary: Ref T7604. Remove "arcanist projects". Depends on D12894.

Test Plan: This diff (so meta).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7604

Differential Revision: https://secure.phabricator.com/D12901
2015-05-20 11:29:41 +10:00
Joshua Spence
d2b38cdf94 pht all the things
Summary: `pht`ize almost all strings in rARC.

Test Plan: ¯\_(ツ)_/¯

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aurelijus, Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12607
2015-05-13 21:00:53 +10:00
Joshua Spence
977baacc32 Remove unused ArcanistUncommittedChangesException class
Summary: Remove the `ArcanistUncommittedChangesException` class which is unused after D11843.

Test Plan: `grep`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12515
2015-05-03 10:07:15 +10:00
Joshua Spence
bf1b30ae26 Trim trailing whitespace from diff hints
Summary: Although these don't do any harm, they show up in my editor which is configured to highlight trailing whitespace.

Test Plan: Submitted this diff... saw no trailing whitespace.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12369
2015-04-13 07:21:07 +10:00
epriestley
8bbcbdd3b6 Prompt users to ignore or abort on untracked files
Summary:
Fixes T7521. This separates things into two prompts and doesn't try to automatically add untracked files.

This also fixes T7512, or at least I can't reproduce it after this change.

Test Plan:
  - Ran `arc diff` in a `git` directory with untracked files.
  - Ran `arc diff` in a `svn` directory with untracked files.
  - Ran `arc diff` in a `hg` directory with untracked files.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7512, T7521

Differential Revision: https://secure.phabricator.com/D12258
2015-04-02 13:40:14 -07:00
epriestley
dd59423d56 Use $EDITOR to prompt users when creating a new commit out of dirty working copy changes
Summary:
Fixes T7344.
Currently, we use `phutil_console_prompt()`, which isn't a very good editor. Use the real $EDITOR instead.

100% of the logic here was also a gigantic mess. Clean it up.

Test Plan: Will update in a second with console output from this run.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7344

Differential Revision: https://secure.phabricator.com/D11843
2015-02-21 02:37:58 -08:00
Joshua Spence
17ec3859cd Fix pht method calls
Summary: Ref T7046. This is mainly a proof-of-concept for D11661.

Test Plan: `arc lint`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7046

Differential Revision: https://secure.phabricator.com/D11683
2015-02-10 18:48:38 +11:00
Bob Trahan
d98c104991 Arcanist / Conduit - stop using deprecated differential.getdiff API
Summary: Fixes T7112. Nothing too difficult here.

Test Plan:
meta - submitting this with the new arcanist code
used conduit API to verify the difference between getdiff (just the latest diff) and querydiffs (all diffs that match, with the latest diff first in the set)

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7112

Differential Revision: https://secure.phabricator.com/D11665
2015-02-03 14:18:10 -08:00
Joshua Spence
9e6f876a68 Export namespace in ArcanistUnitTestResult dictionary
Summary: D7952 added namespaces to `ArcanistUnitTestResult` but these are not exported and thus don't get sent from client to server.

Test Plan: Uploaded a diff and inspected the contents of the `phabricator_differential.differential_diffproperty` table.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D11208
2015-01-05 06:48:32 +11:00
Joshua Spence
f86a70f411 Remove some old code handling the absence of the file.uploadhash Conduit method
Summary: The `file.uploadhash` method was added a long time ago (in D4899). It should be safe to assume that this method exists on most installs.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D11118
2015-01-02 10:10:10 +11:00
Joshua Spence
721bdf424b Use new FutureIterator instead of Futures
Summary: Ref T6829. Deprecate the `Futures()` function.

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aurelijus, Korvin, epriestley

Maniphest Tasks: T6829

Differential Revision: https://secure.phabricator.com/D11079
2014-12-30 23:16:25 +11:00
Joshua Spence
b1112e73c4 Minor linter fixes
Summary: Apply some linter autofixes.

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

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10568
2014-09-27 10:21:23 +10:00
Tal Shiri
ec35375e96 restored --head support
Summary: the call to setHeadCommit() was accidentally placed in a `if ($background) {}` block, which was removed in 54bea94. This adds the removed call.

Test Plan: used --head, worked as expected

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10447
2014-09-08 14:02:26 -07:00
Bob Trahan
c8f15136c8 phutil_utf8_shorten => PhutilUTF8StringTruncator
Summary: Ref T3307. Very easy

Test Plan: this very diff!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T3307

Differential Revision: https://secure.phabricator.com/D10391
2014-08-29 15:15:18 -07:00
epriestley
54bea946c1 Remove --background flag to prevent arc from hanging
Summary:
Ref T4281. A long time ago, we added a `--background` flag to let `arc lint` and `arc unit` run while you're typing a commit message, in some situations.

This code is only moderately beneficial and is way too complicated. Particularly, it has a long history of causing hangs (T4281, T2463), doesn't work on Windows, and is impossible to debug.

It's also running into a serious PHP bug with EAGAIN/EPIPE being indistinguishable that I haven't been able to find a reasonable workaround for in ~3-4 hours of trying.

All the pathways forward that I can see make this already-complex system more complex.

The major reason that this stuff is so complex is that the subprocess may need to prompt the user (notably, to apply patches from lint).

Instead, I'm going to simplify how `arc diff` interacts with `arc lint` and `arc unit`, so we can just fire-and-forget a background process, let it do as much work as it can without needing user input, and then pick up wherever it left off. This will be slightly less cool/magical, but it won't hang bizarrely and I will be able to debug it.

For now, simply remove the `--background` flag and behavior so `arc` works for everyone.

Test Plan: Ran `arc diff` to create this diff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4281

Differential Revision: https://secure.phabricator.com/D10198
2014-08-08 16:09:11 -07:00
epriestley
72447f649f Add a --browse flag to arc diff, to open objects in a browser after creating them
Summary: Ref T5781. Add a flag to optionally open a web browser after creating a diff or revision.

Test Plan: Created //this revision// with `arc diff --browse` // !!! //

Reviewers: csilvers, btrahan

Reviewed By: btrahan

Subscribers: epriestley, spicyj

Maniphest Tasks: T5781

Differential Revision: https://secure.phabricator.com/D10141
2014-08-04 12:03:03 -07:00
Joshua Spence
ef18ae08eb Don't explicitly name abstract base classes
Summary:
Ref T5655. It is superfluous to include "base" in the name of an abstract base class. Furthermore, it is not done consistently within the code base.

In order to retain compatibility with external code, I have kept the `ArcanistBaseWorkflow` class (which trivially extends from `ArcanistWorkflow`), but it is now deprecated and should output a warning message. Similarly for `ArcanistBaseUnitTestEngine`.

Test Plan: Created a workflow which extends from `ArcanistBaseWorkflow`. Executed the workflow and saw a deprecation warning.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, aurelijus

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D9983
2014-07-22 07:49:15 +10:00
Joshua Spence
d09beeb75c Remove @group annotations
Summary: I'm pretty sure that `@group` annotations are useless now... I believe that they were originally used by Diviner?

Test Plan: Eye-balled it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin, aurelijus

Differential Revision: https://secure.phabricator.com/D9855
2014-07-09 09:12:13 +10:00
epriestley
dfe2bde88d Dirty cache of HEAD commit after an amend/range reload
Summary:
Introducing `--head` caused us to run `git diff base..head` explicitly.

However, we can now hit this workflow:

  - We resolve `HEAD` as commit `aaaa1`.
  - This is cached.
  - We notice dirty working copy changes and prompt the user to amend them to HEAD.
  - The user accepts the amend.
  - We amend, creating commit `bbbb2`.
  - We dirty the commit range and reload the working copy. This //does not// dirty the cache of HEAD.
  - We run `git diff`, but it uses the old cached HEAD: `git diff base..aaaa1`.
  - This works fine (`aaaa1` still exists, it's just not on any branch) but produces the wrong diff (without amended changes).

To resolve this, implement the "dirty the cache when the range reloads" hook.

Also never try to amend if the user provides `--head`.

Test Plan:
Ran `arc diff --only --trace` in a working copy with a new commit and some uncommitted changes.

  - Prior to this change, saw a `git diff base..aaaa1` command and the wrong diff.
  - After this change, saw a `git diff base..bbbb2` command and the correct diff.

Reviewers: chad, csilvers, talshiri

Reviewed By: talshiri

Subscribers: epriestley, spicyj

Differential Revision: https://secure.phabricator.com/D9506
2014-06-12 15:46:15 -07:00
epriestley
9492b4ecba Tweak error and status messages for commit ranges
Summary: Improve/clarify some error messages a bit, hopefully.

Test Plan: Ran `arc which`, `arc diff`, etc., with various explicit, implicit, and `--head` flags. Read error messages, didn't catch anything too awkward.

Reviewers: talshiri

Reviewed By: talshiri

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9489
2014-06-11 16:35:50 -07:00
Tal Shiri
6b192f3178 --range support for git
Summary:
This adds support for passing range of commits for arc diff. This is useful when you want to submit code reviews for past commits without mucking around with the working copy.

This will probably require changes :)

Test Plan: Tested locally, but totally need to add tests for this

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9369
2014-06-11 14:37:01 -07:00
Joshua Spence
17820442da Change double quotes to single quotes.
Summary: Ran `arc lint --apply-patches --everything` over rARC, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.

Test Plan: Eyeballed //most// of the diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin, aurelijus

Differential Revision: https://secure.phabricator.com/D9269
2014-05-23 13:53:05 -07:00
epriestley
b52f3fafe3 Fix "arc diff --raw-command --create"
Summary: Fixes T5082. We try to access the repository API in some cases when we don't have one.

Test Plan:
  - Made revisions with "arc diff --raw-command --create".
  - Made this revision, without "--raw-command".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5082

Differential Revision: https://secure.phabricator.com/D9165
2014-05-17 13:40:05 -07:00
epriestley
75cb3cf358 Fix arc diff --raw when run outside a repository
Summary: Fixes T5071. If you run `arc diff --raw` from some arbitrary, non-repository directory we incorrectly try to read information out of the working copy. Even if this information is available, `arc diff --raw` should not assume it's accurate (there's no guarantee the diff comes from the working copy).

Test Plan: Ran `arc diff --raw` in an arbitrary directory.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5071

Differential Revision: https://secure.phabricator.com/D9142
2014-05-15 19:03:29 -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