Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:
- Tons and tons of duplicated code.
- Bugs with handling unusual branch and file names.
- An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
- Other tools were doing hacky things like passing ":" branch names.
This diff attempts to fix these issues.
- Make the base class abstract (it was concrete ONLY for "/diffusion/").
- Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
- Delete the 300 copies of URI generation code throughout Diffusion.
- Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
- Add an appropriate static initializer for other callers.
- Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
- Refactor static initializers to be sensibly-sized.
- Refactor derived DiffusionRequest classes to remove duplicated code.
- Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
- Properly encode path names (fixes issues in D1742).
- Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
- Fix a couple warnings.
- Fix a couple lint issues.
- Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
- Fix a bug where Git change queries would fail unnecessarily.
- Provide or improve some documentation.
This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.
This supplants D1742.
Test Plan:
- Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
- Used Owners typeaheads and search.
- Used diffusion.getrecentcommitsbypath method.
- Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.
{F9185}
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1921
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary: No big surprises here, delted the unused "DarkConsole" class.
Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1876
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary: We now support an optional coverage parameter.
Test Plan: ??? iiam
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1850
Summary:
Adds a macro handler that spams your channel with macros. Config is:
- macro.size: scale macros to this size before rasterizing
- macro.sleep: sleep this many seconds between lines (evade flood protection)
Test Plan: derpderp
Reviewers: kdeggelman, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1838
Summary: Conduit access for open audits.
Test Plan: Used test console to run some queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1792
Summary:
I use a smart editor which wraps words by itself so that I don't need them to be
wrapped by actual newlines.
Curent state disallows me adding or removing words later without uglying the
formatting.
Also the wordwrapped message looks ugly in Phabricator.
I am not sure how the commit message would look like on other places (such as
GitHub) but all reasonable tools should be able to wrap the text by itself.
Test Plan: arc amend --show # on a diff with long lines
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1755
Summary:
Adds softer parse modes with less validation for doing partial parses
during the "arc diff --create" flow.
Test Plan:
Ran "arc diff --create" and got sensible results for inputs like bad
reviewers but a good title/summary.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1720
for scope
Summary:
this patch makes the access token response "complete" relative to spec by
returning when it expires AND that the token_type is in fact 'Bearer'.
This patch also lays the groundwork for scope by fixing the underlying data
model and adding the first scope checks for "offline_access" relative to expires
and the "whoami" method. Further, conduit is augmented to open up individual
methods for access via OAuth generally to enable "whoami" access. There's also
a tidy little scope class to keep track of all the various scopes we plan to
have as well as strings for display (T849 - work undone)
Somewhat of a hack but Conduit methods by default have SCOPE_NOT_ACCESSIBLE. We
then don't even bother with the OAuth stuff within conduit if we're not supposed
to be accessing the method via Conduit. Felt relatively clean to me in terms
of additional code complexity, etc.
Next up ends up being T848 (scope in OAuth) and T849 (let user's authorize
clients for specific scopes which kinds of needs T850). There's also a bunch of
work that needs to be done to return the appropriate, well-formatted error
codes. All in due time...!
Test Plan:
verified that an access_token with no scope doesn't let me see
anything anymore. :( verified that access_tokens made awhile ago expire. :(
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T888, T848
Differential Revision: https://secure.phabricator.com/D1657
Summary: This makes the oauth server a bunch more useful.
Test Plan:
- used /oauth/phabricator/diagnose/ and it actually passed!
- played around with conduit via hacking URL to include access_token on a logged
out browser
- linked my account to itself by going to /settings/page/phabricator/, clicking
"link" account, then cutting and pasting the pertinent ?code=X into
/oauth/phabricator/login/.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T852
Differential Revision: https://secure.phabricator.com/D1644
Summary:
adds a Phabricator OAuth server, which has three big commands:
- auth - allows $user to authorize a given client or application. if $user has already authorized, it hands an authoization code back to $redirect_uri
- token - given a valid authorization code, this command returns an authorization token
- whoami - Conduit.whoami, all nice and purdy relative to the oauth server.
Also has a "test" handler, which I used to create some test data. T850 will
delete this as it adds the ability to create this data in the Phabricator
product.
This diff also adds the corresponding client in Phabricator for the Phabricator
OAuth Server. (Note that clients are known as "providers" in the Phabricator
codebase but client makes more sense relative to the server nomenclature)
Also, related to make this work well
- clean up the diagnostics page by variabilizing the provider-specific
information and extending the provider classes as appropriate.
- augment Conduit.whoami for more full-featured OAuth support, at least where
the Phabricator client is concerned
What's missing here... See T844, T848, T849, T850, and T852.
Test Plan:
- created a dummy client via the test handler. setup development.conf to have
have proper variables for this dummy client. went through authorization and
de-authorization flows
- viewed the diagnostics page for all known oauth providers and saw
provider-specific debugging information
Reviewers: epriestley
CC: aran, epriestley
Maniphest Tasks: T44, T797
Differential Revision: https://secure.phabricator.com/D1595
Summary:
This is pretty simple and unpolished, but it's getting pretty big and it seems
like a reasonable starting point.
- Log chat in various "channels".
- Conduit record and query methods.
- IRCBot integration for IRC logging
Major TODO:
- Web UI is really unpolished and has no search, paging, anchor-linking, etc.
Basically all presentation stuff, though.
- I think the bot should have a map of channels to log with channel aliases?
- The "channels" should probably be in a separate table.
- The "authors" should probably be correlated to Phabricator accounts somehow,
where possible.
Test Plan: Used phabotlocal to log #phabricator.
Reviewers: kdeggelman, btrahan, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T837
Differential Revision: https://secure.phabricator.com/D1625
Summary:
Provide a phid.query method that returns the same information as phid.info,
but allows querying for multiple phids at once.
Test Plan: Called the method from the web conduit console.
Reviewers: btrahan, epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1617
Summary:
Some browsers will still sniff content types even with "Content-Type" and
"X-Content-Type-Options: nosniff". Encode "<" and ">" to prevent them from
sniffing the content as HTML.
See T865.
Also unified some of the code on this pathway.
Test Plan: Verified Opera no longer sniffs the Conduit response into HTML for
the test case in T865. Unit tests pass.
Reviewers: cbg, btrahan
Reviewed By: cbg
CC: aran, epriestley
Maniphest Tasks: T139, T865
Differential Revision: https://secure.phabricator.com/D1606
Summary:
This was a sort of speculative feature added by a contributor some time ago and
just serves as a label; for now, simplify it into "active" and "archived" and
remove "archived" projects from the "active" list.
- Fix a bug where we'd publish a "renamed from X to X" transaction that had no
effect.
- Publish stories about status changes.
- Remove the "edit affiliation" controller, which has no links in the UI
(effectively replaced by join/leave links).
- Add query/conduit support.
Test Plan: Edited the status of several projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1573
Summary:
See discussion in T838. These fields expose information which it isn't necessary
or useful to expose in the general case.
- Disable fields by default, allow them to be enabled in config (these fields
were useful for me at Facebook when I had access to all the machines).
- Remove 'sourcePath' from Conduit methods other than differential.query.
- Condition 'sourcePath' field in Conduit on the caller being the revision
author. This is a bit hacky but not so awful.
Test Plan:
- Verified fields are gone by default and restored by configuration.
- Verified Conduit no longer returns these fields other than
differential.query.
- Verified field presence/absence according to authorship in
differential.query.
- Grepped around in arcanist to make sure we aren't relying on sourcePath.
There's a workflow in "arc merge" that technically might hit it, but I think
it's unreachable, definitely irrelvant (we never use source path as a
distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away
Real Soon Now anyway.
Reviewers: btrahan, arice
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T838
Differential Revision: https://secure.phabricator.com/D1582
Summary: This is not totally done yet, and i'm submitting for feedback.
Test Plan: Played with various settings in local conduit console.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T790
Differential Revision: https://secure.phabricator.com/D1555
Summary: This exposes a few remarkup engines over conduit.
Test Plan:
Local conduit console, and playing with
'cat example.json | arc call-conduit remarkup.process'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1551
Summary:
conduit was using getProductionURI instead of getURI for checking that the
request was sent to the correct host, which causes problems in some dev
environments
Test Plan:
echo '{}' | arc call-conduit --conduit-uri=mydevserver
where my dev server is configured with phabricator.production-uri pointing to
prod instead of my devserver
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1543
Summary:
this has a single side nav now. added a Utilites section below the methods
which houses Logs and Token.
On logs I ended up deleting this whole concept of "view" and the existing side
nav -- I think there were plans to add a way to filter down to subset of the
conduit calls. For logs, I envision that being a separate first class tool when
/ if we think we need additional complexity.
On token I made the form FULL so it was like the rest of the views in this page.
Test Plan:
looks good! clicked on a few methods and it worked! clicked on the
logs and they were there! clicked on the pager within the logs and it worked!
checked out the token page and it looked good too.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D1499
Summary:
- Expose existing 'committed' filter.
- Add an 'accepted' filter.
- Fix a fatal where $repository may not be defined (for diffs not linked to a
repository).
Test Plan: Ran accepted / committed queries. Viewed a previously fataling diff.
Reviewers: btrahan, vrana, Makinde
Reviewed By: Makinde
CC: Koolvin, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1490
Summary: We currently allow you to launch abstract daemons; use
setConcreteOnly() to only list/launch concrete daemons.
Test Plan: Ran "phd list" (no abstract daemons listed), "phd launch
PhabricatorRepositoryCommitDiscoveryDaemon" (reasonable error message).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T801
Differential Revision: https://secure.phabricator.com/D1487
Summary: This enables some improvements in D1478. Allow revisons to be queried
by the branch which they appear on.
Test Plan: Queried revisions by branch. Ran "arc which" branch queries in SVN
and Mercurial.
Reviewers: btrahan, cpiro, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T787
Differential Revision: https://secure.phabricator.com/D1479
Summary: It was broken by D!352
Test Plan: Praying that it works.
Reviewers: nh, epriestley, andrewjcg
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1453
Summary: Add a conduit method to query project information.
Test Plan: Ran method from API test console.
Reviewers: bill, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1444
Summary: ..."ssh" is in quotes 'cuz this is step 1 and there's no ssh in sight
at the moment.
Test Plan:
ran api.php PHID-USER-xee4ju2teq7mflitwfcs differential.query a few times...
- tried valid input, it worked!
- tried bad input, it worked in that it failed and told me so!
ran api.php crap_user differential.query a few times...
- verified error message with respect to crap_user
ran api.php PHID-USER-xee4ju2teq7mflitwfcs crap_method a few times...
- verified error message with respect to crap_method
visited http://phabricator.dev/conduit/method/differential.query a few times...
- tried valid input, it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D1357
Summary:
- We currently run ##parseValueFromCommitMessage()## on all fields present in
the message, but not ##validateField()##.
- This detects value errors (e.g., an invalid reviewer) but not higher-level
errors (e.g., a missing field).
- This can break the stacked-commits Git mutable history workflow by
recognizing too many commit messages as valid ("multiple valid commit messages,
this is ambiguous").
- This also gives you some errors ("Missing test plan") too late in "arc diff
--create" (after the diff has been built).
Test Plan:
- Grepped for validateField() calls, removed a couple of calls that had the
same implementation as the base class.
- Grepped for other calls to this to make sure I'm not stumbling into
unintended side effects, but it only runs from the diff workflow.
- Ran "arc diff --create" with an invalid test plan, got a good error early in
the process.
- Ran "arc diff master" with stacked local commits, got a correct selection of
the intended message.
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, cpiro
Differential Revision: https://secure.phabricator.com/D1373
Summary:
Added a Conduit API method to return all transactions for a
given set of task_ids. This will be used to comments and other important
information about the tasks.
Test Plan:
Use Conduit to execute ##maniphest.gettasktransactions## and
visually verify that transaction information is returned.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1361
Summary:
Just talked to @tuomaspelkonen, and turns out there is a case where
postponed tests results use the filepath for both the name and file
parameters. Then, after the tests have completed, the unittest
results are updated with the class name as the test name. To handle
this, this diff matches the stored unittest results name against
either the name or file component of the updated unittest info.
Not sure of great way to generally handle these situations. Perhaps,
long term, we can just use a placeholder unittest result, mark that
as passed (or delete it?) then add a new test result with the correct
name.
Test Plan: updated unittest result with new name (but file was the same).
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, epriestley, andrewjcg
Differential Revision: https://secure.phabricator.com/D1356
Summary:
When using postponed unittests to make 'arc diff' faster, there
are some situations where it is difficult to know exactly how
many unittests will be run. This is the case for many of our
C++ unittests, which we can't really know until we compile the
tests (which is slow, and probably isn't reasonable to be done
before posting the diff). I suppose we could make sure we
explicitly which tests a C++ unittest will run in some way, but
this would require a lot of change to our backend test infra.
Also, it seems that this is a pretty general issue of not knowing
how many unittests will be run until they actually run.
This diff adds an optional "create" parameter to updateunitresults
which wil create a new unit tests result rather than updating an
existing one. I am not sure if this really fits here or should
be its own method, but there is a lot of code re-use between them
so I consolidated.
Test Plan: updated a diff with a new unit test result
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, andrewjcg, tuomaspelkonen
Differential Revision: https://secure.phabricator.com/D1352
Phabricator
Summary: ...this breaks without D1328. Used good ole "codemod" to do this
work, with lots of manual edits around 80 chars.
Test Plan: clicked around phabricator tool suite, particular differential, a
bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1351
corresponding ConduitAPI
Summary: reasonable title... also made this new functionality used by the
repository worker for parsing diffs
Test Plan:
- looked at the conduit console and queried for various types of hashes,
including hashes with no match. got correct results.
- identified a reasonable diff from a local git repo. set the revision status
to 2 (ACCEPTED) in the database. augmented the worker parser code to var_dump
and die after finding revision id. ran scripts/repository/reparse.php
--message rX and verified my var_dumps. removed var_dumps and die and ran
reparse.php again with same paramters. verified revision looked good in
diffusion and there were no errors.
- repeated the above reparse.php jonx for a mercurial repo. note svn isn't in
this hash game so that test was particularly exciting no-op'dness i did not
bother with
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1315
Summary: I want to add a command like "where is ArcanistUnitTestEngine" to
phabot. I also want to add a symbol typeahead to Diffusion and generally finish
up that feature since it's useful but only half-implemented. Consolidate the
query logic and expose the data over Conduit.
Test Plan: Used /symbol/ and Conduit to lookup symbols.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1260
Summary: See D1257. Also make the error message more friendly, and remove a very
very old Facebook-specific error.
Test Plan:
- Tried to diff with an older arc.
- Tried to diff with a newer arc.
- Diffed with the right arc.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1258
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary: Add the author PHID to the differential.getrevisionfeedback conduit api
method
Test Plan: issue differential.getrevisionfeedback query via conduit against a
valid revision and verify author phid is included in results
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, jungejason, tuomaspelkonen, jonathanhester
Differential Revision: 1190
couple bug fixes
Summary:
- Add the ability to query for "responsible users" (author or reviewer).
- Add the ability to query for "subscribers" (reviewer or CC).
- Fix an issue where CC and Reviewer used the same join table alias and were
incompatible.
- Remove support for 'paths' for the moment, since each path needs a
repository ID. (There are no clients for this.)
- Remove single withX() methods that have no callsites -- withPath() is
singular because it accepts two arguments and I didn't want to have an ad-hoc
type format, but I think we can get away without these for other conditions.
- Include GROUP BY in more cases where may need it. This doesn't actually
change program behavior since we uniquify in loadFromArray(), it just means less
data over the wire.
These new query classes are to support rewriting the Differential list view on
top of DifferentialRevisionQuery.
Test Plan:
- Issued queries via conduit for "responsible users".
- Issued queries via conduit for "subscribers".
- Issued queries via conduit for "cc" with "reviewer" at the same time.
- Issued queries via conduit for "cc", "reviewer", "responsible users" and
"subscribers" at the same time.
- Issued a "subscribers" and "reviewers" query which returned duplicates;
verified GROUP BY took effect.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, nh
Differential Revision: 1182
Summary:
Changed cc/reviewer search to be a union/or instead of intersection/and within
each list. Also added support to search for multiple authors (same behavior as
cc/reviewer), and updated conduit call to match. (See discussion on D1158.)
Test Plan:
Used the conduit call to search for revisions with one of 2 people on the cc
list, and checked the results to see that it wasn't constraining to requiring
both be on the cc list.
Reviewers: epriestley, jungejason, btrahan
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1179
Summary:
Created a differential.query conduit method that is built on top of
DifferentialRevisionQuery. I also added support for querying by author, ccs, and
reviewers to DifferentialRevisionQuery, so feature parity can be brought up to
match differential.find and its backing class DifferentialRevisionListData.
Test Plan:
Tried a few calls to the conduit call using the web interface, and got back
reasonable looking data.
Reviewers: epriestley, jungejason, btrahan
Reviewed By: epriestley
CC: aran, nh, epriestley
Differential Revision: 1158
order to generate a template
Summary: See T614. This allows us to generate an empty template by calling
Conduit, so we can build command-line editing workflows for SVN, Mercurial, and
conservative-Git.
Test Plan: Used web console to invoke Conduit method; got a reasonable empty
template out of it.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1156
Summary: As of D1154, we don't need this anymore. See that change for context.
Test Plan: See D1154.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1155
Summary: This is sort of silly but maybe useful? The real problem is that there
are like 500k conduit call logs and the real solution to that is better
filtering options, but this seems sort of okay.
Test Plan: Used "[" and "]" to switch between pages on the conduit call log.
Reviewers: btrahan, jungejason, nh, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 1145
Summary:
The conduit access to Differential kind of sucks and we want to break
back-compat in order to fix it (see D1114).
To make it easier to pull this off, I want to build out the Conduit logging a
bit so administrators can identify which users are making deprecated calls.
We should probably build a little more infrastructure around this too (API
versions?), but this is at least a reasonable step forward which gives us more
insight into the use of Conduit and more tools to smooth the deprecation
process.
This initial commit is super basic but the interface currently says "stuff",
I'll build this out a little more in a bit.
Test Plan: Looked at call logs.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1144
Summary: some ground work for T479
Test Plan:
called up a diff via the conduit api console
it had the right project name and did not error
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1139
Summary:
Added a new method differential.createcomment
Task ID: #752014
Test Plan:
I created a test diff and called this method via the conduit
from a client PHP script to add comments. I confirmed that
1) the comment appeared on the revision, 2) URLs within the
comment were turned into hyperlinks, and 3) Phabricator
sent a notification email to the people watching the test
diff.
Reviewers: nh, jungejason, epriestley
Reviewed By: nh
CC: aran, nh
Differential Revision: 1128
Summary:
Move event framework from Phabricator to libphutil so it can be used in other
phutil projects, such as Arcanist.
Test plan:
Use along with path to libphutil, events should work as expected.
Reviewers: epriestley
Differential Revision: 1098
Summary: createrevision creates the revision as the user which certificate is
used. Add a meta parameter to API calls named actAsUser so one user can create
revisions for someone else. Right now there is no authentication.
Test Plan: Called createrevision with one users cert and set actAsUser to
someone else. The revision was created as the actAsUser user.
Reviewers: epriestley, nh, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1087
Summary:
the code tries to access 'encoding' property even when the
repository is empty. The fix is to set it to null in that case.
Test Plan: run the conduit method on my sandbox and it works now.
Reviewers: grglr, epriestley, nh
Reviewed By: grglr
CC: aran, grglr
Differential Revision: 1075
Summary:
`arc commit` and `arc mark-committed` would only add comments <author> committed
this revision, since now everyone can run this commands it makes more sense to
show the actual committer instead of the author.
Test Plan:
Commit (or mark committed) not your revision, Phabricator should add <you>
committed this revision comment instead of <author> committed this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1067
Summary:
Allow anonymous access to conduit getdiff method, which is needed for anonymous
`arc patch`.
Test Plan: Running getdiff with an unauthenticated conduit should work.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1068
Summary:
This adds an encoding detail to the repository, so we can attempt to
convert hunks previously detected as binary.
We also add the encoding information to the arcanist projectinfo
API so we can pull the information if we have it when uploading changes
via arc.
Test Plan:
Changed encoding through the edit UI, and saw "This is binary file", and
changed it back and saw the correct output from the diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1009
Filesystem::readRandomCharacters()
Summary: See T547. To improve auditability of use of crypto-sensitive hash
functions, use Filesystem::readRandomCharacters() in place of
sha1(Filesystem::readRandomBytes()) when we're just generating random ASCII
strings.
Test Plan:
- Generated a new PHID.
- Logged out and logged back in (to test sessions).
- Regenerated Conduit certificate.
- Created a new task, verified mail key generated sensibly.
- Created a new revision, verified mail key generated sensibly.
- Ran "arc list", got blocked, installed new certificate, ran "arc list"
again.
Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 1000
without quotes around it (and similar)
Summary: See D1010. The API uniformly requires JSON, which is good for
strictness and predictablity but can be bad for UEX, especially considering that
we silently continue after failing to decode things. Toss the user a lifeline
when they make this common mistake.
Test Plan: Ran API calls with invalid and valid inputs. Invalid inputs gave me a
reasonable error message.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1012
Summary:
This is an attempt to satisfy a lot of the one-off requests a little more
generally, by providing a relatively generic piece of event architecture.
Allow the registation of event listeners which can react to various application
events (currently, task editing).
I'll doc this a bit better but I wanted to see if anyone had massive objections
to doing this or the broad approach. The specific problem I want to address is
that one client wants to do a bunch of routing for tasks via email, so it's
either build a hook, or have them override most of ManiphestReplyHandler, or
something slightly more general like this.
Test Plan: Wrote a silly listener that adds "Quack!" to a task every time it is
edited and edited some tasks. I was justly rewarded.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley
Differential Revision: 881
Summary:
We currently rely on "remote_hooks_enabled" in .arcconfig to determine whether
commands like "arc amend" and "arc merge" should imply "arc mark-committed".
However, this is a historical artifact that is now bad for a bunch of reasons:
- The option name is confusing, it really means 'repository is tracked'.
- The option is hard to discover and generally sucks.
- We can empirically determine the right answer since we now know if a project
is in a tracked repository.
Add a call which arcanist can make on these workflows to figure out if it is
interacting with a project in a tracked repository or not.
Also added an "isTracked()" convenience method to reduce the number of magic
strings all over the place.
Test Plan: Ran "arcanist.projectinfo" for nonexistent, untracked and tracked
projects.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 945
Summary:
We need to query the unit status in order to determine if
there are postponed unit tests to update after running "arc unit"
Test Plan:
1) set my conduit uri to a server running the new code
2) ensured unitStatus existed when retrieving a diff
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 918
Summary:
These fields use auxiliary storage now. Migrate the data and get rid of the
columns in the main table.
- This might take a little while to run, although there are <500k rows so
probably not too long.
- Maybe grab a backup of the table first, if I screwed something up this will
delete the data in these fields.
Test Plan:
- Ran migration locally.
- Browsed Differential.
- Grepped for "revertPlan" and "blameRevision".
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 832
Summary:
I didn't realize createDiffDict was a public method when I
modified it, and I broke the API call in getrevision. This moves the
modification inside the method and reverts the method header back to
it's original form.
Test Plan: none
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, epriestley
Differential Revision: 883
Summary:
Provide three Phriction methods for programmatic access to Phriction:
- phriction.info: get information about a document
- phriction.history: get change history for a document
- phriction.edit: create or update a document
I moved all the editing/creation logic into a new PhrictionDocumentEditor to
share code between the Conduit and Web edit pathways.
Test Plan: Got info and history via conduit. Edited and created new pages via
conduit and web.
Reviewers: hsb, jungejason, tuomaspelkonen, aran, hunterbridges
Reviewed By: hunterbridges
CC: skrul, aran, hunterbridges
Differential Revision: 866
Summary:
We need createlintresults because we are doing extended
static analysis offline, and thus we need to be able to update the
lint results associated with a diff. This is similar to
updateunitresults, but "create" is more accurate than "update" since
we never need to change existing lint results.
getdiffproperty is used by the client to ensure it isn't creating any
duplicates lint results. It's the symmetric operation to
setdiffproperty, which already exists.
Test Plan:
We have a new offline linter that I used to test. This
linter calls getdiffproperty on every run.
1. Tested updating an existing set of lint results by first running
"arc diff" with lint errors caught by the local linter, then later
running offline analysis which catches one other error and updates via
createlintresults. Ensured the differential lint results were as
expected.
2. Tested the creation of an entirely new diff property through
createlintresults. I first ran "arc diff --nolint" to skip all lint
results, then ran offline analysis which caught an error and updated
through createlintresults. Ensured differential lint results were as
expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, mgummelt, jungejason, epriestley
Differential Revision: 868
Summary: Execute searches like the primary Maniphest task list. Let me know what
else you guys need from this API.
Test Plan: Executed "maniphest.info" and "maniphest.find"
Reviewers: jungejason, tuomaspelkonen, aran, nh
Reviewed By: nh
CC: blair, skrul, aran, jungejason, epriestley, nh, tuomaspelkonen
Differential Revision: 867
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: This could be a lot fancier but let's see what else we need. Also fixed
some bugs with maniphest.info.
Test Plan: Used the Conduit web console to create some tasks with different
values.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 824
Summary:
After D814 and D829, you should be able to implement this logic in the
didWriteRevision() method of the field.
Note that the attacher is still referenced in
ConduitAPI_differential_updatetaskrevisionassoc_Method. This method should
probably be moved to facebook/ since it's pretty Facebook-specific.
No rush on any of this, it's not hurting anything.
Test Plan:
- Hit differential.getcommitmessage
- Ran 'arc diff'
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 830
Summary:
I think this is the last major step -- use the fields to parse commit messages,
not a hard-coded list of stuff. This adds two primary methods to fields, one to
get all the labels they'll parse (so we can do "CC" and "CCs" and treat them as
the same field) and one to parse the string into a canonical representation
(e.g., lookup reviewers and such).
You'll need to impelement the one block of task-specific stuff I removed in
Facebook's task field:
list($pre_comment) = split(' -- ', $data);
$data = array_filter(preg_split('/[^\d]+/', $pre_comment));
foreach ($data as $k => $v) {
$data[$k] = (int)$v;
}
$data = array_unique($data);
break;
Otherwise I think this is clean.
Test Plan:
- Called the conduit method with various commit messages, parsed fields/errors
seemed correct.
- "arc diff"'d this diff onto localhost, then updated it.
- "arc amend"'d this diff.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 829
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
Summary:
Provide a catchall mechanism to find unprotected writes.
- Depends on D758.
- Similar to WriteOnHTTPGet stuff from Facebook's stack.
- Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
- Never allow writes without CSRF checks.
- This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
- **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**
Test Plan:
- Ran some scripts that perform writes (scripts/search indexers), no issues.
- Performed normal CSRF submits.
- Added writes to an un-CSRF'd page, got an exception.
- Executed conduit methods.
- Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
- Did OAuth login.
- Did OAuth registration.
Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
Summary:
When we create or update a revision, we use a parsed commit message dictionary
to edit its fields. Drive consumption of the dictionary through custom fields
instead of hardcoding.
This requires adding some fields which don't really do anything right now to
cover fields which appear only in the commit message.
Test Plan: "arc diff"'d this revision against localhost, "arc diff"'d again to
update.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 811
Summary:
Differential has a bunch of display-only fields, implement them all as field
specifications instead of hard-coded fields.
Also add some more documentation and fix redundant string constants in blame
rev/revert plan fields.
Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 807
Summary:
- Fix a bug where 'caption' didn't do anything.
- Provide an abstract base implementation for extensions.
- Add some documentation.
- Expose aux fields via conduit.
Test Plan: Added some fields like "Dinosaur", "Kilograms" and "derp" on my local
install. Read documentation.
Reviewed By: jungejason
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, philc, jungejason
Differential Revision: 785
Summary: Similar to D785 for Maniphest, expose auxiliary field values via
Conduit.
Test Plan: Ran revision.getinfo on a revision with aux fields, got them in the
response.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 802
Summary:
we need to know the revision id in order to generate
differential links. It would be amazing if there existed some library
for object <-> network call mapping, and we could get all the
information about an object given some unique ID rather than having to
create a dictionary manually. One can dream...
Test Plan:
1. setup on test phabricator instance
2. called differential.getdiff from client code with a valid diffid
3. verified that the correct revisionID was included
Reviewed By: epriestley
Reviewers: epriestley
CC: dpepper, aran, epriestley, mgummelt
Differential Revision: 795
Summary:
- Allow the console to handle abstract classes correctly.
- Move paste dictionary generation to an abstract base class.
- Add paste.create.
- Add 'objectName', 'parentPHID', and 'content' to Paste info dictionaries
(you can use filePHID with file.download to get the content but I think just
always sending it back is reasonable).
Test Plan:
- Use paste.create to create new pastes.
- Used paste.info to get existing pastes.
- Checked console UI to make sure "paste." didn't show up or anything
silly/dumb like that.
- Tried to call the method "paste" and got the right exception.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 747
Summary:
Need this for 'arc upload' and 'arc download'.
Given a file PHID or ID, provides information about it.
Test Plan:
- Implemented 'arc upload' and 'arc download' based on 'file.info'.
- Used Conduit API console to test method.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 741
Summary:
Provides a slowvote.poll.info method.
Test Plan:
Web console - seemed to work fine.
Reviewers:
epriestley, phuzion
CC:
Differential Revision: 659
Summary:
Now that we store language with pastes, we can include this in Conduit.
Test Plan:
Tried it on a paste with a blank language, and one with a specified language.
16:14:50 <@CodeBlock> P1
16:14:51 <@codeblock-phabot> P1: http://phabricator.local/P1 - test.php
16:15:05 <@CodeBlock> P43
16:15:06 <@codeblock-phabot> P43: http://phabricator.local/P43 - sadoijfoisaf (php)
Reviewers:
epriestley, Ttech
CC:
Differential Revision: 616
Summary:
- have files be uploaded by drag+drop instead of browse.
- Files are named by their uploaded filename, the user isn't given a chance to enter a file name. Is this bad?
- Store author PHID now with files
- Allow an ?author=<username> to limit the /files/ list by author.
- If one file is uploaded, the user is taken to its info page.
- If several are uploaded, they are taken to a list of their files.
Test Plan:
- Quickly tested everything and it still worked, I'd recommend some people try this out before it gets committed though. It's a rather huge revision.
Reviewers:
epriestley, Ttech
CC:
Differential Revision: 612
Summary:
My earlier diff refactored some code without completely
respecting the semantics, sometimes resulting in duplicate field names
returned from differential.getcommitmessage. This fixes that.
Test Plan:
ran "arc diff" with diff causing the bug (commit message
had an empty Revert Plan: field) and verified no duplicate fields
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, dpepper, epriestley
Differential Revision: 610
