1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-11 09:22:40 +01:00
Commit graph

457 commits

Author SHA1 Message Date
vrana
6472dbe168 Change fileName to filename
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.

Test Plan:
Alter database.
Open revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1437
2012-01-17 10:50:14 -08:00
epriestley
ef768f9694 Use phutil_utf8_hard_wrap() in Phabricator
Summary: See D1433.

Test Plan: Created a new diff with a line >80chars, observed it wrapping
correctly.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1438
2012-01-17 09:48:58 -08:00
epriestley
e2c75d5dc2 Improve Differential handling of disabled users
Summary:
We currently allow you to assign code review to disabled users, but
should not.

Test Plan:
  - Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
  - Looked at a disabled user handle link, was clearly informed.
  - Tried to create a new revision with a disabled reviewer, was rebuffed.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1429
2012-01-17 09:27:19 -08:00
epriestley
05ee317555 Remove "parsedHunk" property
Summary: This is never read anywhere and clearly has no effect.

Test Plan: grep

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D1434
2012-01-17 08:09:56 -08:00
epriestley
8f1c7dc663 Remove some unused nonsense
Summary: These blocks do nothing. end() produces a side effect on the internal
array pointer, but the code does not depend on it.

Test Plan: Reasoned about the code? Also viewed some diffs.

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D1432
2012-01-17 08:09:45 -08:00
epriestley
2d8b35db93 Remove getDisplayLine()
Summary: No callsites anywhere. Unclear what this method is even supposed to do.

Test Plan: grep

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D1435
2012-01-17 08:09:38 -08:00
vrana
f94c05a5bb Declare property
Test Plan: Display revision

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1430
2012-01-16 17:36:11 -08:00
vrana
8bed2f4387 Utilize phutil_render_tag()
Test Plan: Display diff with lint error.

Reviewers: pad, epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1428
2012-01-16 17:08:21 -08:00
vrana
f109342a7a Display link in Revision ToC for copied or moved files
Summary:
They are present in the document so there is not reason to omit the links to
them.
They sometimes contains changed lines so the link could be actualy useful.

Test Plan: Display ToC of revision with moved and copied files.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, nh

Differential Revision: https://secure.phabricator.com/D1412
2012-01-16 09:10:17 -08:00
vrana
c7997e0a7c Display Show Raw File links in Differential only if the file exists
Test Plan:
Open menu for added file
Open menu for deleted file
Open menu for changed file

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1410
2012-01-16 09:08:40 -08:00
vrana
49a59bd885 Fix XSS in Differential
Test Plan: Display a revision with file copied to ##<b>hack</b>##.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1411
2012-01-15 22:36:14 -08:00
epriestley
d8bbf55959 Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.

When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:

  Action Has No Effect

  You can not accept this revision because it has already been accepted.

  Do you want to post the feedback anyway, as a normal comment?

                        [Cancel] [Post as Comment]

If they have no comment text, the dialog only says "Cancel".

I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).

This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.

Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran, vrana

Maniphest Tasks: T730

Differential Revision: https://secure.phabricator.com/D1403
2012-01-15 03:44:09 -08:00
vrana
4cff02dcc0 Add BRANCH to Accepted and Needs Revision e-mails
Summary:
I always forget a branch which I used for the diff so that I must open
my browser which takes some time. This diff adds the name of the branch to the
sent e-mails. But only if the diff is in the state Accepted or Needs Revision to
not pollute other e-mails.

Test Plan:
Comment
Request changes
Accept
Look at the e-mails

Reviewers: epriestley

Reviewed By: epriestley

CC: olivier, aran, epriestley, vrana

Differential Revision: https://secure.phabricator.com/D1396
2012-01-14 11:12:28 -08:00
vrana
c6febdfc52 Add link from lint error to code
Test Plan:
Display diff with lint errors
Click on a line number in lint errors overview

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1400
2012-01-14 11:11:17 -08:00
vrana
5cf6d788ce Don't add author and reviewers to CCs
Summary:
Commenting on a diff causes adding the writer to the CCs. It doesn't make much
sense if the writer is author or reviewer who get all the copies anyway.

I've also moved the decision to DifferentialCommentEditor.

Test Plan:
Comment on a diff where I am author
Comment on a diff where I am reviewer
Comment on a diff where I am neither
Explicitely Add CCs where I am author

Reviewers: epriestley

Reviewed By: epriestley

CC: jungejason, aran, epriestley

Differential Revision: https://secure.phabricator.com/D1397
2012-01-14 11:10:40 -08:00
epriestley
04eb1f98e2 Always mark revisions as "updated" when users comment on them
Summary: See T773 and the explanatory inline comment.

Test Plan: Made no-action comments and comments that did something (reject, plan
changes) to revisions. Saw them always jump to the top of the action list.

Reviewers: jungejason, simpkins, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T773

Differential Revision: https://secure.phabricator.com/D1386
2012-01-12 20:08:26 -08:00
Jason Ge
cf35a640ec Enable filtering for committed revisions
Summary:
engineers requested to supporting filtering by 'committed'
revisions, and I think it makes sense.

Test Plan: verified that all the three options worked

Reviewers: epriestley, btrahan, nh

Reviewed By: nh

CC: nh, wolffiex, aran

Differential Revision: https://secure.phabricator.com/D1383
2012-01-12 17:02:20 -08:00
epriestley
d84c0a5be5 Validate all fields in differential.parsecommitmessage
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
2012-01-12 15:20:11 -08:00
Bob Trahan
84ea5c53e4 Kill PhabricatorFileURI
Summary:
we used to need this function for security purposes, but no longer need
it.   remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.

Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files.  not sure what
these are.  changes here are very programmatic however.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Maniphest Tasks: T672

Differential Revision: https://secure.phabricator.com/D1354
2012-01-10 15:21:39 -08:00
Bob Trahan
2a29a51080 Deploy new ArcanistManyWordsAboutDifferentialConstants class from D1328 into
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
2012-01-10 11:49:20 -08:00
epriestley
a4ceba9101 Add more information to differential.getdiff
Summary: We need some additional fields to heuristically match revisions to the
working copy in arc.

Test Plan: Executed conduit method, got correct values in fields

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: https://secure.phabricator.com/D1347
2012-01-10 10:40:57 -08:00
Jason Ge
79d6d252b7 Display base correctly for git-svn in update history
Summary:
for a git repo which is using git-svn, we should show the
revisioni number. For example, it is show "svn+ssh" if the git url
starts with "svn+ssh". Show the svn revision number instead.

Before fix:
https://secure.phabricator.com/file/view/PHID-FILE-j5rogyqjkgifoxtpwzcu/

After fix:
https://secure.phabricator.com/file/view/PHID-FILE-4vsnptcjzuzuyj4zo25x/

Test Plan:
- verified a diff with git-svn worked
- verified a pure git diff still worked

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1344
2012-01-08 12:18:22 -08:00
Nick Harper
13d930b232 Fix table layout bug for inline comments
Summary:
The filename header for inline comments used to span 2 columns - the line number
and the comment. With the addition of a column for the diff (to link to inline
comments on previous diffs), the filename header should now span 3 columns
instead of just the line number and diff, leaving the comment squished to the
right.

Test Plan:
Opened a differential revision with an inline comment from a previous diff, and
saw that the filename header continued across the comment. Also checked an
inline comment on a current diff, and saw that it looks fine.

Reviewers: epriestley, btrahan, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1340
2012-01-06 14:55:18 -08:00
epriestley
3c9551e96d Add "Reveal Entire File" option to Diffusion
Summary: Clicks all the "Show All" links for you at the touch of a button.

Test Plan:
  - Used "reveal entire file" on revealable files.
  - Opened on already-visible files, got "entire file shown".
  - Used other menu options.
  - Used normal "show more" links.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T497

Differential Revision: https://secure.phabricator.com/D1331
2012-01-06 11:51:10 -08:00
epriestley
e5c0bfc8e3 Link to undisplayed inline comments
Summary:
We currently don't link to comments which aren't visible. Link to the
appropriate diff in a new window, indicating where the comment lives.

Test Plan: Clicked visible, not-so-visible comments.

Reviewers: btrahan, jungejason, davidreuss

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T555, T449

Differential Revision: https://secure.phabricator.com/D1333
2012-01-06 11:50:59 -08:00
epriestley
1903bb80bb Add a link from Differential to Diffusion
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.

Test Plan: Tested menu in linked and unlinked diffs. Used menu item.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T309

Differential Revision: https://secure.phabricator.com/D1326
2012-01-05 18:03:08 -08:00
epriestley
da3a972400 Retry failed page anchor navigation
Summary:
When the user loads a page with an anchor on it like #thing, or clicks a link to
#thing, and #thing doesn't exist, keep trying to navigate to #thing for a few
seconds.

This allows anchors to work when the target is in content which is later ajaxed
in. In particular, this affects inline comments in Differential.

Test Plan: Opened inline comment links in a new tab, was in the right place when
I switched tabs.

Reviewers: nh, btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T492

Differential Revision: https://secure.phabricator.com/D1327
2012-01-05 18:02:02 -08:00
Bob Trahan
e478c0074c Add support for querying by commit hashes to DifferentialRevisionQuery class and
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
2012-01-05 13:51:51 -08:00
epriestley
7abdf3afe0 Add a dropdown menu for Differential view options
Summary:
There are several open Differential tasks that are basically blocked on not
having reasonable places in the UI to put things. Replace the "View Standalone /
Raw" button with a "View Options" dropdown menu so we can shove things like
"Expand All", "Fold / Unfold File", and "View in Diffusion" in there.

This doesn't change any behavior, just puts the existing options in a menu.

Test Plan:
  - Toggled menu open by clicking button.
  - Clicked menu items.
  - Toggled menu closed by clicking button.
  - Toggled menu closed by clicking document.
  - Toggled menu closed by opening another menu.
  - Toggled menu closed by selecting an item.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T497, T309

Differential Revision: https://secure.phabricator.com/D1316
2012-01-05 12:58:38 -08:00
epriestley
275472d70f Fix appearance of "Differential Revision" on edit interfaces
Summary: The recent change to the field causes us to render "http://junk.com/D"
in some cases, just null the field if there's no data.

Test Plan: Ran "arc diff --create".

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1321
2012-01-05 12:57:40 -08:00
epriestley
1cb81936e7 Minor, fix array_merge() issue. 2012-01-05 09:11:49 -08:00
vrana
460efc4489 Include added reviewers and ccs in preview
Summary: Preview of Add Reviewers looks silly without actually showing them

Test Plan:
Go to any diff
Leap into action: Add Reviewers
Add some reviewers
Write some comment
Preview including Added reviewers should be displayed
Change action to Comment
Added reviewers should disappear
Repeat with Add CCs

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, vrana

Differential Revision: https://secure.phabricator.com/D1276
2012-01-04 17:08:13 -08:00
epriestley
4077ef2555 Show lint issues inline in Differential
Summary:
Take lint information and use it to render synthetic inline comments in
Differential.

Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-2uapwmf5tqhgscbuty3b/

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley, pad

Maniphest Tasks: T24

Differential Revision: https://secure.phabricator.com/D1309
2012-01-04 14:17:33 -08:00
epriestley
28b606394b Fix undefined variable warning in unit test field
Summary:
See D1295. $unit_messages may be undefined.

I'll see if I can improve the visibility of warnings, the red dot in DarkConsole
is easy to miss right now. See T734.

Test Plan: Loaded a revision with no unit failures, didn't receive a warning.

Reviewers: nh, btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1306
2012-01-04 10:20:53 -08:00
Andrew Gallagher
cd0386bf8b Remarkup unittest result messages
Summary: This diffs adds support for marking up unittest result messages.

Test Plan: Verified that links in unittest results were markup'd.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley, zeeg

Differential Revision: https://secure.phabricator.com/D1298
2012-01-03 18:23:34 -08:00
vrana
7a9be605ae Avoid double escaping in render()
Test Plan: View any comment

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1296
2012-01-03 16:46:05 -08:00
Nick Harper
4a77906141 Don't show detailed unittest result box if all tests passed
Summary:
When all unit tests pass, a box appears between the unit test results and lint
status (for test failures to go in). This checks if there's anything to put
in that div/ul before putting it on the page.

Test Plan:
Loaded a revision with unit tests OK and saw no box. Loaded a revision with
failing unittests, and saw the same box from before.

Reviewers: tuomaspelkonen, epriestley

Reviewed By: epriestley

CC: jungejason, aran, nh, epriestley

Differential Revision: https://secure.phabricator.com/D1295
2012-01-03 14:58:22 -08:00
Nick Harper
369079dd45 Change default status to 'all' in DifferentialRevisionList
Summary: Makes it easier to discover the list of all revisions for a user.

Test Plan:
Opened up /differential/filter/revisions/, and saw that it defaulted
to status of all. Clicked between tabs, and it stayed on all. Selected
open, it only displayed open revisions, including as I switched between
tabs.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1278
2012-01-03 14:49:30 -08:00
vrana
cce212dac7 Link from Blame Revision
Summary: There can be Dxxx, rXXXxxx or even full URL in //Blame Revision// field
so just highlighting it as normal text would work probably best

Test Plan:
Go to https://secure.phabricator.com/D277
You should see a link from //Blame Revision// (if it would be displayed)

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1274
2011-12-22 15:57:53 -08:00
epriestley
9d8b5481ae Emit full URIs to identify Differential revisions
Summary:
  - Previously, used IDs like "33" to match a commit to a Differential revision.
This has a namespacing problem because we now have an arbitrarily large number
of Phabricator installs in the world, and they may want to track commits from
other installs.
  - In Differential, parse raw IDs or full URIs. Emit only full URIs.
  - In Repositories, parse only full URIs.
  - This might cause a few commits to not be picked up in rare circumstances.
Users can fix them with "arc mark-committed". This should be exceedingly rare
because of hash matching.
  - There are some caveats for reparsing older repositories, see comments
inline. I don't think there's much broad impact here.

Test Plan:
  - Created a new revision, got a full URI.
  - Updated revision, worked correctly.
  - Ran unit tests.
  - Monkeyed with "Differential Revision" field.
  - Reviewers: btrahan, jungejason

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Maniphest Tasks: T54, T692

Differential Revision: 1250
2011-12-20 19:58:22 -08:00
epriestley
a810469929 Improve performance of differential.query
Summary:
  - Instead of running (1 + N * 4) queries, run 5 queries.
  - Add 'dateModified' field.

Test Plan:
  - Ran various queries against various revisions, data seemed unchanged.
  - Checked query log.

Reviewers: nh, jonathanhester, jungejason, btrahan

Reviewed By: jonathanhester

CC: aran, epriestley, jonathanhester, jungejason

Differential Revision: 1252
2011-12-20 19:52:18 -08:00
epriestley
bd3c2355e8 Match usernames in any case in commit message object lists (CC, Reviewers, etc.)
Summary:
Allow entry of "CC: alincoln" to match user "ALincoln".

Put both variations in the map and try the exact case version first since we'll
also match email addresses and mailables, and theoretically some mailable might
have the same name as a user, as we're effectively abandoning restriction of
which characters can appear in usernames.

Test Plan: Created a local revision with a reviewer in CrAzY CaPs.

Reviewers: jungejason, btrahan

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T697

Differential Revision: 1255
2011-12-20 19:48:56 -08:00
jhester
1270b0b5bd Add properties to differential.getdiff
Summary: Add 'addLines' and 'delLines' properties to differential.getdiff return
dictionary.  These properties are aggregated from the changesets.

Test Plan: Issue a differential.getdiff query via conduit and verify that
'addLines' and 'removeLines' properties are included and accurate

Reviewers: epriestley

Reviewed By: epriestley

CC: epriestley, aran, jonathanhester

Differential Revision: 1209
2011-12-16 18:02:35 -08:00
Evan Priestley
770beb5cd5 Merge pull request #85 from dcramer/hide-empty-fields-in-maniphest
Hide auxiliary fields that have no value set
2011-12-16 16:51:58 -08:00
David Cramer
2a80bdd448 Hide auxiliary fields that have no value set
Reviewers: epriestly
2011-12-16 16:49:45 -08:00
epriestley
c97fcd12bc Share Revision/Task attaching code
Summary:
We have this code in two places; split it into an editor class so we can share
it.

This also fixes some probems with this field not //detaching// tasks properly.

Test Plan:
  - Created a revision with no attached tasks.
  - Attached it to a task.
  - Updated it.
  - Detached it.
  - Used web UI to attach/detach tasks/revisions.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Differential Revision: 1225
2011-12-16 16:13:00 -08:00
epriestley
43fc7820e9 August comes AFTER July. 2011-12-16 13:34:36 -08:00
epriestley
dfc3506240 Prevent "Maniphest Tasks" field from creating empty comment when revisions are
updated

Summary:
  - If you update a revision with a nonempty "Maniphest Tasks" field, an empty
comment is posted (see T586).
  - The transaction email currently says "Attached revision 'Unknown
Differential Revision'", move attaching to "didWriteRevision()" to make sure the
object has been written.

Test Plan:   - Attached; updated a revision.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T685

Differential Revision: 1223
2011-12-16 13:32:52 -08:00
epriestley
3cd92ab075 Minor fix to D1186 to enforce ordering. 2011-12-16 13:32:16 -08:00
epriestley
4dd87f1ad3 Drive Differential landing page with DifferentialRevisionQuery, simplify UI
Summary:
  - Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
  - Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
  - Allow views to be filtered and sorted more flexibly.
  - Allow anonymous users to use the per-user views, just don't default them
there.

NOTE: This might have performance implications! I need some help evaluating
them.

@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?

The "active revisions" view is built much differently now. Before, we issued two
queries:

  - SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
  - SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)

These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.

Now, we issue only one query:

  - SELECT (open revisions you authored or are reviewing)

Then we divide them into the two tables in PHP. That query is available in P246.

On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?

In particular:

  - Run the queries and make sure the new version doesn't take too long.
  - Run the queries with EXPLAIN and give me the output maybe?

Test Plan:
  - Looked at different filters.
  - Changed "View User" PHID.
  - Changed open/all.
  - Changed sort order.
  - Ran EXPLAIN / select against secure.phabricator.com corpus.

Reviewers: btrahan, nh, jungejason

Reviewed By: btrahan

CC: cpiro, aran, btrahan, epriestley, jungejason, nh

Maniphest Tasks: T586

Differential Revision: 1186
2011-12-16 13:21:54 -08:00
David Cramer
2677217244 Revised unit display to resemble lint output
Reviewers: epriestley

Test Plan: View a differential with unit results and compare display.

Differential Revision: 1216
2011-12-14 16:11:55 -08:00
Marek Sapota
2d232674df Allow customized patterns for marking generated files.
Test Plan:
Created a listener that adds some patterns to $matches array, reloaded
Differential, some changesets were not shown as generated.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1200
2011-12-13 17:14:25 -08:00
epriestley
682e0aa468 Add "responsible users" and "subscribers" to DifferentialRevisionQuery, plus a
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
2011-12-08 09:38:52 -08:00
epriestley
bd12a2b839 Fix a final (?) task field issue which slipped through the cracks
Summary:
Derped this one up; while my testing was successful in preventing runaway
attaching I missed the bit where it doesn't actually work.

This resolves the "Unknown Object" link seen on T661.

Test Plan:
  - Created two new revisions, each attached to a local task.
  - Verified that they attached additively, Maniphest and Differential were
linked to the right places, and nothign else bad happened.

Reviewers: btrahan, fratrik

Reviewed By: fratrik

CC: aran, fratrik, btrahan

Differential Revision: 1181
2011-12-07 08:53:19 -08:00
epriestley
d13906ff96 Add "tabindex" to Remarkup reference lists
Summary:
Prevent keyboard focus of these links so we don't disrupt tab order from
comments to "Submit".

Arguably I should make a "function" for this or something but there's nowhere to
really put it that makes any sense right now.

Test Plan: Verified Firefox skips these links in tab order.

Reviewers: fratrik, btrahan, jungejason

Reviewed By: fratrik

CC: aran, fratrik

Maniphest Tasks: T661

Differential Revision: 1180
2011-12-07 08:53:12 -08:00
epriestley
4fd81150be Remove "Updated" view from Differential
Summary:
This landed during my review drama embargo and is a generally good idea but had
some implementation issues.

@elynde reports it has been broken for some time, although it still works on
secure.phabricator.com so I'm guessing it's just taking a zillion years to run
at Facebook. It's up to more than a second for me on secure.phabricator.com:

https://secure.phabricator.com/file/view/PHID-FILE-v4ql4c66u3xnkarmrpm4/

The basic problem is that some of the data architecture around this
implementation is hard to scale. I want to pursue a similar feature eventually,
but drive it off notifications that we'll ship through real-time infrastructure
too.

I'm also trying to get rid of DifferentialRevisionListData and this simplifies
that somewhat.

Test Plan:
  - Grepped for table name, table constant, query constant, and class name; no
hits.
  - Applied SQL patch.
  - Verified that Differential no longer shows "Updated".

Reviewers: elynde, btrahan, jungejason

Reviewed By: elynde

CC: aran, elynde

Differential Revision: 1178
2011-12-07 06:55:03 -08:00
Nick Harper
74f710a437 Add sanity to DifferentialRevisionQuery
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
2011-12-06 14:47:12 -08:00
epriestley
2797903776 Fix bad query edge condition when updating a revision with no attached tasks. 2011-12-04 13:25:32 -08:00
Bob Trahan
588b959c03 phabricator_format_timestamp => phabricator_datetime
Summary: make the change, kill the function.   be sure to get a good $user or
$viewer variable

Test Plan:
for each controller or view, look at it in the ui.   change timezone, refresh ui
and note change.   i did not test the OAuthSettingsPanelController; not sure how
to get to that badboy and i got a bit lazy

Maniphest Tasks: T222

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Maniphest Tasks: T222

Differential Revision: 1166
2011-12-04 08:51:34 -08:00
Jakub Vrana
9c1697383c Add a link to Remarkup Reference below comment
Summary:
To reduce blindness, all textareas with some kind of special syntax should have
an information about this syntax and a link to its documentation. Preview
function is a nice complement but it doesn't replace this information.

I've added this information and the link below the comment field.

Please note that <a target> is a valid attribute in HTML5.

Test Plan:
Go to https://secure.phabricator.com/D1164#comment-content
There should be a link to Remarkup Reference
This link should open Remarkup Reference in a new window (to not discard the
comment)

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, vrana

Differential Revision: 1164
2011-12-03 12:28:21 -08:00
Bob Trahan
83efa6e1c5 make arc diff link maniphest tasks with revisions
Summary:
add "Maniphest Task:" or "Maniphest Tasks:" followed by text that has TX in it.
foreach TX the task will be attached to the revision and the revision will be
attached to the task.  parsing is pretty... ummm, robust such that it will pick
up any TX substring and parse that as a Maniphest Task just fine.   it errors
out if there is not an actual task for TX and otherwise churns along pretty
nicely.

Also, make sure the PhabricatorObjectHandle loads the task ID as the alternateID
since we need that here and it should be that way anyhoo.

Test Plan:
made a diff and in the commit message added Maniphest Task(s): TX combination.
Tried various combinations of TX -- single, multiple with commas, multiple many
lines, single bad, multiple bad, multiple mix of bad and good. verified that the
good tasks were attached to the diff and diff was attached to the good tasks.

Maniphest Tasks: T137

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Differential Revision: 1165
2011-12-03 11:34:55 -08:00
epriestley
10cc5f2660 Set user on auxiliary fields before validating them on template workflow
Summary: Some fields need this data in some circumstances in order to validate
-- see D1153.

Test Plan: Ran "arc diff" against local, no longer got an exception for access
of this field from the 'Reviewers' validator.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: 1160
2011-12-02 15:14:39 -08:00
Nick Harper
8f5e28bf59 Add differential.query conduit method
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
2011-12-02 13:06:43 -08:00
Bob Trahan
519a443eba kill differential tabs in favor of a create diff button
Summary:
kill the tabs and make it a create button instead.  pertinent notes:
* added a "Filter diffs" button to the form.  optional, but i thought it
necessary with the new green button
* linked to Arcanist user guide on the create diff page.  somewhat unrelated but
i think create diff will get more traffic now so linking to help seemed like a
reasonable add on here.

Test Plan:
viewed differential homepage
* clicked left hand filter elements.  noted "Create Diff" button on filters
within user revisions and no button on filters within all revisions.
* entered another user into Select User UI and viewed their diffs via button and
pressing enter

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, btrahan, epriestley

Differential Revision: 1157
2011-12-02 10:39:36 -08:00
epriestley
19f2110e74 Allow "differential.getcommitmessage" to be called without a revision ID in
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
2011-12-02 07:28:55 -08:00
epriestley
40221feed9 Validate commit message fields on the server side
Summary:
See T643. We have some hard-coded checks in Arcanist for the existence of
'testPlan' and 'title', and don't properly validate those fields on the server.
Add a validation pass in the Conduit-based edit pathway.

In particular, this means that if you disable the "Test Plan" field, Arcanist
won't block you anymore.

Test Plan: Disabled Arcanist checks and ran "arc diff"; got blocked on the
server side.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: 1153
2011-12-02 07:28:43 -08:00
Marek Sapota
e9693f25f8 Move event framework from Phabricator to libphutil
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
2011-11-16 16:34:45 -08:00
Marek Sapota
b71a55900a Allow tweaking of Differential mail by using events
Summary: Allow tweaking Differential mail before sending.

Test Plan:
Wrote a listener renaming Differential attachments and it worked without
problems.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota, davidreuss

Differential Revision: 1091
2011-11-09 10:13:53 -08:00
epriestley
fbfb263cd9 Provide a configuration flag to disable silliness in the UI
Summary: See comments. A few installs have remarked that their organizations
would prefer buttons labled "Submit" to buttons labeled "Clowncopterize".

Test Plan:
  - In "serious" mode, verified Differential and Maniphest have serious strings,
tasks can not be closed out of spite, and reset/welcome emails are extremely
serious.
  - In unserious mode, verified Differential and Maniphest have normal strings,
tasks can be closed out of spite, and reset/welcome emails are silly.
  - This does not disable the "fax these changes" message in Arcanist (no
reasonable way for it to read the config value) or the rainbow syntax
highlighter (already removable though configuration).

Reviewers: moskov, jungejason, nh, tuomaspelkonen, aran

Reviewed By: moskov

CC: aran, moskov

Differential Revision: 1081
2011-11-04 15:24:54 -07:00
Marek Sapota
f1de90d7ef Mark person that actually runs arc commit as committer instead of the author.
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
2011-11-01 15:26:56 -07:00
Marek Sapota
74d1983c68 Move Abandon Revision action to the bottom if it's an admin action.
Test Plan:
Login as admin, look at an open revision you don't own, you should be able to
choose '(Admin) Abandon Revision', the option should be on the bottom and should
abandon the revision after sending the comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1060
2011-10-28 09:20:18 -07:00
Marek Sapota
9536e5606c Allow admins to abandon Differential revisions.
Test Plan:
Login as an admin, go to a revision that you don't own - you should be able to
abandon this revision.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1048
2011-10-25 14:32:50 -07:00
Evan Priestley
9d4793b27f Merge pull request #76 from mareksapota-fb/master
Pull request for differential revision D1044
2011-10-25 10:43:42 -07:00
Marek Sapota
789dc6cb5e Allow anonymus access to Differential.
Summary:
Add possibility for not logged in users to browse and see Differential
revisions.

Test Plan:
Set 'differential.anonymous-access' config option to true, log out, you should
be able to browse Differential without logging back in.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1044
2011-10-25 10:23:08 -07:00
epriestley
88be49fd5f Allow DifferentialDiff to construct proper DifferentialChangeset objects from
diffs which add empty files

Summary:
See T507 and some others. We now parse empty git diffs correctly, but the logic
to build DifferentialDiffs out of them leaves the objects with 'null' for
$changesets, when it should be array().

Further layers later throw, believing we have not loaded the changesets, when we
actually have, there just aren't any.

Test Plan: Viewed rJX05d493e17fbbb29f29e4880be6834d1d7415374e in Diffusion,
which adds an empty README file. No exception thrown.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: nh

CC: aran, nh

Differential Revision: 1038
2011-10-24 23:39:59 -07:00
epriestley
4156cf6bd9 Add an optional configuration option to set 'Precedence: bulk' headers on
transactional mail

Summary: See T571. SES refuses to deliver mail with this header and there are
various reports of other issues on the internet so I'm defaulting it to off.

Test Plan: Set config to true, tried to send mail, SES rejected it because of
"Precedence: bulk" header.

Reviewers: bmaurer, ola, jungejason, nh, aran

Reviewed By: aran

CC: aran, epriestley, bmaurer

Differential Revision: 1032
2011-10-23 14:25:13 -07:00
epriestley
661f077bf7 Replace callsites to sha1() that use it to asciify entropy with
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
2011-10-21 11:55:28 -07:00
Emir Habul
f447e5d709 Allow custom hyperlinks; Pass differential.diff-id into remarkup engine config
Summary: This allows extensions to have more options for generating custom
hyperlinks.

Test Plan:
custom-inline rules are moved before default rules. Test existing products which
implement custom rules.
Make sure you use "$this->getEngine()->storeText()" in rules.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran, epriestley, emiraga, jungejason

Differential Revision: 1024
2011-10-20 14:39:18 -07:00
Evan Priestley
0cb9f3dcf5 Merge pull request #74 from mareksapota-fb/master
Pull request for differential revision D1019
2011-10-19 15:31:22 -07:00
tuomaspelkonen
a102c9a0fe Allow to resign from an accepted revision when you didn't accept the diff.
Summary: Girish wants to be able to do this.

Test Plan: Checked that I had the option in my sandbox on an accepted diff.

Reviewers: epriestley, jungejason

Reviewed By: jungejason

CC: aran, jungejason, tuomaspelkonen, epriestley

Differential Revision: 1020
2011-10-19 15:27:36 -07:00
Marek Sapota
a11053d0fa Add possibility to upload a diff file instead of using copy-paste.
Test Plan:
Go to /differential/diff/create and upload a diff file - result should be the
same as pasting the diff into the textarea.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1019
2011-10-19 15:25:25 -07:00
Marek Sapota
5d377e246a Send patch attachments instead of diff attachments.
Test Plan:
Turn on sending patches, create a new revision - you should get a .patch file in
your mail instead of a .diff file.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1016
2011-10-18 12:20:24 -07:00
Marek Sapota
87a2987ad6 Differential mail
Test Plan: EMPTY

Reviewers: aran, epriestley

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1004
2011-10-14 12:12:41 -07:00
epriestley
254f606e89 Tie all the pieces for symbol cross-references together
Summary:
This makes symbol cross-references work in Differential. You need to do a little
legwork but I'll document that once the change has baked for a little while.

Basically:

  - Projects are annotated with indexed languages, and "shared library" projects
(for example, symbols in Phabricator should be searched for in Arcanist and
libphutil).
  - When we render a changeset, we check if its language is an indexed one. If
it is, we invoke the decorator Javascript.
  - The Javascript takes you to a lookup page, which either gives you a list of
matching symbols (if several match) or redirects you instantly to the
definition.

Test Plan: Clicked class and function symbols in a diff, got jumped into
sensible sorts of places in Diffusion.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 980
2011-10-09 17:58:17 -07:00
epriestley
0580772805 Add a JS component for crossreferences
Summary: When the user clicks a crossreference, jump them to symbol lookup

Test Plan: Clicked some crossref symbols

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: nh

CC: aran, nh, epriestley

Differential Revision: 904
2011-10-09 17:58:01 -07:00
Jason Ge
1e3c10379a Enable typeahead's ondemand on details view page
Summary:
the details pages are using preload instead of ondemand for
typeahead, but the most common actions on the pages are commenting which
would not need the preloaded info. To improve the performance of the
pages, turn on ondemand according to the setting in the config file.

Test Plan: verify it is working with both modes, for both pages.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 995
2011-10-09 12:33:08 -07:00
epriestley
c29982acb9 Fix phid accumulation for handles
Summary: I goofed this, $phids was already being populated and I changed the
meaning. This causes a fatal if you filter the list by a user who is not an
author or first reviewer for any of the revisions (e.g., no open revisions).

Test Plan: Looked at the list of a user with no revisions.

Reviewers: codeblock, jungejason

Reviewed By: codeblock

CC: aran, codeblock, jungejason

Differential Revision: 989
2011-10-07 12:58:16 -07:00
epriestley
8ce5dd31f6 Show open Differential revisions in Diffusion browse views
Summary:
Still some rough edges, but this adds a table of open revisions to Diffusion.
See T262.

I'll make this a little better (e.g., "see all.." instead of arbitrary 10 cap,
or maybe move to top-level nav?) but I think I have to refactor some other stuff
first. This should let us root out any major issues, at least.

NOTE: You must associate Arcanist Projects with Repositories (in Repositories ->
Arcanist Projects -> Edit) for this to work!

Also made paths include all parent paths so that browse views of directories
will work.

Test Plan: Uploaded a diff which affected "/blah", it appeared when browsing "/"
and "/blah".

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 979
2011-10-06 10:27:54 -07:00
epriestley
91bf3e96c9 Provide a Differential Revision query class for affected paths
Summary:
For T262, we need to query for revisions by affected path.

We currently have a class called "DifferentialRevisionListData" but it's sort of
nasty and it would have been really cumbersome to add this query to it.

Instead, this provides a query object more in line with ManiphestTaskQuery,
which I'm pretty happy with. I'd eventually like to get rid of
DifferentialRevisionListData but it's used in a couple of places right now.

Test Plan: Used phpsh to execute queries, got back apparently-sensible result
sets.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason, epriestley

Differential Revision: 978
2011-10-06 10:27:17 -07:00
epriestley
bea4795575 Separate revision list rendering logic into a RevisionListView
Summary: I want to throw this in Diffusion as part of T262, but it's embedded in
the controller right now. Split it out.

Test Plan: Looked at various revision list views, no changes.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 977
2011-10-06 10:26:47 -07:00
Hua Wang
d41fd4a0fa T494 Image displaye issue
Summary: The display of images pairs is not corresponding to the selected two
image diffs. The fix is to use reference to get the phid for each image.

Test Plan: Create a revision with two diffs of images.
           Test the display between base and diff1/diff2.
           Test the rendering of images between diff1 and diff2.
           Test the inline comments also.

Reviewers: epriestley, jungejason

CC:

Differential Revision: 955
2011-09-30 00:25:33 -07:00
tuomaspelkonen
7b8b469da3 Changed the postponed unit tests warning message
Summary:
Postponed unit tests are not unit tests with problems. The results
just haven't arrived yet.

Test Plan: Tested accepting a diff with unit status 1, 3, 5 (ok, errors,
postponed)

Reviewers: jungejason, epriestley

Reviewed By: epriestley

CC: aran, epriestley, tuomaspelkonen

Differential Revision: 969
2011-09-27 13:00:36 -07:00
epriestley
016b060aea Add a relation table for Revisions to local commit hashes
Summary:
This allows us to performantly query for diffs related to a given local hash.
Immediate applications are:

  - Commit detection in Mercurial and Git-Immutable workflows.
  - Some async unit test stuff @mgummelt was doing.

Test Plan:
Diffed locally under SVN/Git/hg, checked the table, got sensible output.

  mysql> select * from differential_revisionhash;
  +------------+------+------------------------------------------+
  | revisionID | type | hash                                     |
  +------------+------+------------------------------------------+
  |         40 | gtcm | 8c6fb2f95598a50f7aac64a5f4cc6c12b5db42f5 |
  |         40 | gttr | 54710e361a465f4ff39565a93b2a221b6e7dd07c |
  |         41 | hgcm | c29cb69aec14                             |
  |         41 | hgcm | e7309be4eabb                             |
  |         41 | hgcm | 4e885caeff60                             |
  |         41 | hgcm | 213ee1cd30ea                             |
  |         41 | hgcm | b4050fb3490f                             |
  |         41 | hgcm | 72a76bd7ffa2                             |
  |         41 | hgcm | 06c2687e63fb                             |
  |         41 | hgcm | 2b464bde6b48                             |
  +------------+------+------------------------------------------+
  10 rows in set (0.00 sec)

NOTE: Mercurial hashes are short-form but I'll shoot out a separate Arcanist
diff to fix this.

Reviewers: Makinde, fratrik, mgummelt, jungejason, nh, tuomaspelkonen, aran

Reviewed By: Makinde

CC: aran, Makinde

Differential Revision: 961
2011-09-26 15:02:37 -07:00
epriestley
43a3f4d234 Build an "affected path" index when attaching diffs to revisions
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.

Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.

Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.

Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran

CC:

Differential Revision: 931
2011-09-15 07:45:14 -07:00
epriestley
a42f116749 Allow "!accept" to be enabled through configuration
Summary: For reasons explained in the config I've omitted this from the default
action set, but it's trivial to support it. See D916.

Test Plan: Commented on a revision, was informed I could "!accept" in the email.
Used "!accept" to accept the revision.

Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran

Reviewed By: Makinde

CC: aran, Makinde

Differential Revision: 928
2011-09-14 09:52:13 -07:00
Hua Wang
cd6eb836f6 Enable comments for image
Summary: Added line number 1 for each image and added code to display the
comments for each image.

Test Plan: Adding an image in my local directory and create a revision for it.
Click line number 1, and the comment window prompts.  Adding and save the
comment. The comment shows in the differential comment list and in the inline
comment.  Submit the comment.  Create more comments for the image and the
"Previous" and "Next" buttons all work well.

Reviewers: epriestley, jungejason

CC:

Differential Revision: 901
2011-09-06 18:11:41 -07:00
epriestley
e875c81f6d Remove blameRevision and revertPlan from the DifferentialRevision schema
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
2011-09-04 16:19:12 -07:00
epriestley
76ac8b4196 Display local commit information in Differential
Summary:
After D857, we try to attach local commit information to revisions. If this
information is available, display it on the revision.

Design on this is a little rough, I might try to combine this into the revision
update view or something like that since we're starting to take up a lot of real
estate for metadata.

Test Plan: Local diffed this and got some commit info.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Differential Revision: 872
2011-08-31 13:49:50 -07:00
epriestley
f7e136ecd2 Remove accidental double-rendering of content sources from Differential
Summary: Oops, I left this in from an earlier version and missed it since I was
mostly looking at Maniphest for testing. We already render this information in
the header, don't additionally render it under the comments.

Test Plan: derp derp, loaded any revision with sourced comments

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 871
2011-08-30 12:16:43 -07:00
epriestley
69445222f7 Track content sources (email, web, conduit, mobile) for replies
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
2011-08-30 11:08:27 -07:00
epriestley
0334a92621 Save empty fields as no row, not an empty row
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.

Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 865
2011-08-26 16:26:02 -07:00
Nicholas Harper
8c0e5e1c58 Turn off write guard when saving differential comment cache
Summary:
It is possible to view a comment that has no cache; when viewing such a comment
the request doesn't have a csrf token and there is no need for one, so we turn
off the write guard.

Test Plan:
loaded an old diff that had no cache, and the page loaded instead of throwing
an AphrontCSRFException.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 858
2011-08-25 15:45:16 -07:00
Hua Wang
e903b82fff Delete one line
Summary: Delete one line which has no effect.

Test Plan: Open revision page to make sure it still works.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 852
2011-08-23 00:45:43 -07:00
epriestley
67de714a2f Remove (most) support for 'differential.attach-task-class'
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
2011-08-19 14:10:30 -07:00
epriestley
dc39571d63 Simplify default field loadout for Differential
Summary:
Remove the blame revision, revert plan and lines fields from the default field
loadout. (After D829 this doesn't cause issues where we have bogus dictionary
entries.)

You should add these back to the Facebook configuration since Facebook wants
these fields. However, I want to keep the default stack very light and I never
saw a huge amount of value in these fields at Facebook so I don't think they
make the cut. Sorry, tomo. ;_;

Test Plan: Ran "arc diff" locally.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: aran

CC: aran, tomo, epriestley

Differential Revision: 831
2011-08-18 19:51:32 -07:00
epriestley
ebdd6d3d11 Fix a variable typo which prevents metadata from attaching to @mention
Differential comments

Summary: If you @mention several users, at least one of which is already CC'd,
we unset all the CCs and don't attach the "Added CCs: ..." block to the comment.

Test Plan: @mentioned two users, one of whom was already CC'd.

Reviewers: jungejason, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 827
2011-08-18 19:49:52 -07:00
epriestley
0be3db03ee Drive Differential commit message parsing through extensible fields
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
2011-08-18 19:49:39 -07:00
epriestley
735120b842 Fix bug where CC value is ignored on intial parse from commit message
Summary: While I thought this was complicated, there was nothing subtle or
tricky here -- I just misnamed a variable.

Test Plan: Created a revision with default CCs, got CCs instead of nothing.

Reviewers: aran, jungejason, tuomaspelkonen

Reviewed By: aran

CC: aran

Differential Revision: 834
2011-08-18 13:08:28 -07:00
Jason Ge
4693ffa82b Deprecate generateProperties
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().

Depends on D814.

Test Plan:
implemented facebook task field with custom field and
verified it worked.

Reviewers: epriestley, tuomaspelkonen

Reviewed By: epriestley

CC: aran, jungejason, epriestley

Differential Revision: 826
2011-08-18 11:33:10 -07:00
epriestley
6dc193d3d9 Fully update library map. 2011-08-18 09:52:36 -07:00
epriestley
ae7488f710 Drive commit message rendering from field specifications
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
2011-08-18 07:20:20 -07:00
epriestley
cd3a3bf759 Make Herald Rules sticky in X-Herald-Rules
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
2011-08-17 10:38:29 -07:00
epriestley
88eb6410b3 This is also an unguarded but CSRF-safe cache write. 2011-08-16 14:44:13 -07:00
epriestley
2d22226ff0 Unguard the Differential update time write on GET. 2011-08-16 13:50:47 -07:00
epriestley
39b4d20ce5 Create AphrontWriteGuard, a backup mechanism for CSRF validation
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
2011-08-16 13:29:57 -07:00
epriestley
ec0d91a3ff Drive revision update from Conduit via custom fields
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
2011-08-15 10:25:54 -07:00
epriestley
a869dbf45b Implement all field edit interfaces on the custom field schema
Summary:
Moves the revision edit controller to be completely schema-driven.

Depends on D810.

Test Plan: Edited revisions. Entered intentionally invalid values to trigger
error conditions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 810
2011-08-15 10:21:00 -07:00
epriestley
442d1dbeaa Move Differential's remaining field views to extensble field schema
Summary:
Move all the rest of the fields into the custom field schema, for revision
views.

I left a couple of stubs in here (willWriteRevision, didWriteRevision) since I'd
planned to do edits here too, but this diff is sort of big-ish already. I'll do
all the edit fields in the next revision.

Depends on D808.

Test Plan: Viewed, edited and conduit'ed some revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 809
2011-08-15 10:20:46 -07:00
epriestley
5038b26018 Move Differential's read-only fields to the extensible field schema
Summary:
Move additional fields (which rely on loading handles) to the extensible field
classes and out of hardcoding in the controller.

Depends on D807.

Test Plan: Viewed, edited, and hit conduit for revisions.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 808
2011-08-15 08:39:58 -07:00
epriestley
52ec6c02ee Move Differential's simple fields to the extensible field schema
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
2011-08-15 08:39:48 -07:00
epriestley
7aa1eff383 Expose Differential auxiliary fields in Conduit
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
2011-08-14 10:43:38 -07:00
epriestley
e196bf5b43 Provide builtin definitions for "Blame Revision" and "Revert Plan" fields
Summary:
This is just to ease transitions for any installs which use these fields (e.g.,
Facebook). I'll write some docs and a migration script once this stuff is a
little more solid, too.

Depends on D800.

Technically these are "better" than the current fields since they show up other
places than the edit screen (derp derp).

Test Plan: Created a field selector which provides these; verified they work by
typing stuff into them and saving the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 801
2011-08-14 10:04:50 -07:00
epriestley
9b3370368d Allow Differential custom fields to appear on edit and view interfaces
Summary: Depends on D798. Extends custom fields and makes the vaguely useful:
they can appear on the edit and view interfaces. This does not integrate them
with commit messages yet; that's more complicated but I plan to do it shortly.
Test Plan: Implemented a custom field per P123, it correctly appears on the edit
interface, persists, validates, and shows up when viewing the revision.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 800
2011-08-14 10:04:37 -07:00
epriestley
dd74903cae Add basic auxiliary field storage for Differential
Summary:
Precursor to building this out to solve T343. This is similar to the Maniphest
fields we landed recently, although I think they're dissimilar enough that it
isn't worth going crazy trying to make them share code, at least for now.

This doesn't really do anything yet, just adds a storage object and a couple of
selector/field indirection classes.

Test Plan: Ran SQL upgrade script, created an aux field.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 798
2011-08-14 10:04:21 -07:00
epriestley
f49e35deaf Basic task dependencies for Maniphest
Summary:
This allows you to edit dependencies. It is a better patch than it used to be.
It depends on D725.

  - If you create a cycle, it just throws an exception and aborts the workflow.
It should not do this.
  - Tasks which depend on the current task aren't shown in the UI. Need to add a
new table for this.
  - Transaction text says "attached Task" but should probably say "added a
dependency on task".

Test Plan: Created valid and invalid dependencies between tasks. Created valid
and invalid dependencies between revisions.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Commenters: codeblock
CC: aran, codeblock, tuomaspelkonen, epriestley
Differential Revision: 595
2011-08-02 11:16:31 -07:00
Edward Speyer
9e2231f6d6 Revert "Generated code> make it harder to mark code as generated"
Summary: This reverts commit e15da75687.
Test Plan: None
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 762
2011-08-02 14:47:10 +01:00
Edward Speyer
e15da75687 Generated code> make it harder to mark code as generated
Summary: It's now harder to accidentally mark code as generated.
Test Plan:
Tested with this diff in my sandbox; only file "D" was shown, the others
were marked as generated:

  diff --git a/A b/A
  index e69de29..780b46c 100644
  --- a/A
  +++ b/A
  @@ -0,0 +1,2 @@
  +@generated
  +Tue Jun  7 16:41:17 PDT 2011
  diff --git a/B b/B
  index e69de29..b55fe21 100644
  --- a/B
  +++ b/B
  @@ -0,0 +1,3 @@
  +/**
  + * @generated by ed
  + */
  diff --git a/C b/C
  index e69de29..e0f808a 100644
  --- a/C
  +++ b/C
  @@ -0,0 +1,3 @@
  +/**
  + * {@generated <<jonx>lol>}
  + */
  diff --git a/D b/D
  index e69de29..89e8829 100644
  --- a/D
  +++ b/D
  @@ -0,0 +1,2 @@
  +string = STDIN.readlines
  +string.include?('@generated')

Reviewed By: jungejason
Reviewers: jungejason
CC: aran, edward, jungejason
Differential Revision: 408
2011-08-02 12:50:37 +01:00
epriestley
9d3f33a7a6 Rough implementation of drag-and-drop file uploads
Summary:
This gets all the major pieces working. Allows you to drag-and-drop files in
Differential and Phriction, and embed files in remarkup with {Fxxx} references.
See also task.

I'm explicitly not documenting this yet since it's still pretty rough.

Test Plan: Dragged and dropped stuff into Differential and Phriction.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, tomo
Commenters: tomo
CC: aran, tomo, jungejason
Differential Revision: 674
2011-08-01 15:27:13 -07:00
epriestley
b70b9bb6d7 Be more explicit in rendering context links in Differential
Summary: See T368. The current rendering result can cause some confusion for the
first/last chunks, make their behavior more explicit.
Test Plan:
  - Clicked various "show more" links on a bunch of top/bottom/middle omitted
context blocks in a variety of diffs.
  - Located a @generated shielded file and verified the initial render is
correct when the entire file is default-hidden.

Reviewed By: avitaloliver
Reviewers: avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, avitaloliver
Differential Revision: 744
2011-07-29 17:23:36 -07:00
tuomaspelkonen
e00fae8436 Files can be set not to use 'ignore-all' by default.
Summary:
Python people don't seem to like the 'ignore-all' as default. Provide a way
to configure which file types should not use 'ignore-all'.

Test Plan:
Tested that it worked with bunch of Python of files and non-python
files. Cache was disabled during the test.

Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 713
2011-07-25 10:46:40 -07:00
epriestley
c0ae2f6289 Show change diffs in Phriction
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.

Show text changes between diffs and allow users to revert to earlier versions.

Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.

I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.

Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
2011-07-18 08:46:45 -07:00
epriestley
5e00d00cf7 Show more information on revision views
Summary:
Show line count, arcanist project and base revision.

This adds a little clutter but I think we're still okay and I can play around
with it later.

Test Plan: Looked at a couple of revisions. I'm actually not 100% sure about the
SVN logic but maybe I will test that before committing.
Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 685
2011-07-16 18:54:13 -07:00
epriestley
f761ee91d3 Publish Phriction edits to feed
Summary: Basic integration between Phriction and feed.
Test Plan: Created and edited some documents, they published to feed.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 653
2011-07-12 16:36:17 -07:00
epriestley
a49138defd Generalize the markup engine factory
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.

This will let us accommodate changes we need to make for Phriction to support
wiki linking.

Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
2011-07-11 16:36:30 -07:00
mgummelt
36e00db564 bug due to using $fields instead of $this->fields
Summary: see title

Test Plan: ran "arc amend" to ensure that task ids where being included in the commit message

Reviewers: epriestley

CC: dpepper

Differential Revision: 637
2011-07-10 20:26:56 -07:00
epriestley
f55c082e65 Publish Differential stories into feed
Summary: Basic hookup for Differential -> Feed. Also introduces "one-line"
stories for less-important stuff.
Test Plan: Interacted with some revisions, got feed stories out of it.
Reviewed By: jungejason
Reviewers: jungejason, aran, tuomaspelkonen, codeblock
CC: aran, jungejason
Differential Revision: 632
2011-07-09 17:39:17 -07:00
epriestley
51c2726a34 Add Differential parse cache to the GC daemon
Summary:
Add the differential parse cache to the GC. This is the largest object in the
system by a wide margin, I think.

This table is potentially gigantic which is why the script truncates it before
doing a schema change.

Test Plan: Ran the GC daemon, it cleaned up some parse caches.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: tuomaspelkonen
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 620
2011-07-08 17:31:25 -07:00
epriestley
c9acc5b8e9 Allow comment panel to be stuck/unstuck to the bottom of the display
Summary:
See T303. Enable comment panel haunting.

I hid the preview for the sticky panel, which I think is reasonable?

Test Plan:
https://secure.phabricator.com/file/view/PHID-FILE-64713fa8a7c2a22e5b93/
Reviewed By: broofa
Reviewers: broofa, jungejason, aran, tomo, tuomaspelkonen
CC: aran, broofa
Differential Revision: 615
2011-07-08 13:24:20 -07:00
mgummelt
81e3ec5998 Merge branch 'master' of github.com:facebook/phabricator into new 2011-07-07 15:30:20 -07:00
mgummelt
00f4c37ca2 Fixed bug resulting in duplicate field names in commit messages
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
2011-07-07 15:26:55 -07:00
epriestley
85b34c23f9 Clean up Phabricator interface to syntax highlighting
Summary: Reduce the amount of code duplication here and allow for an override
configuration on the filename.map stuff.
Test Plan: Checked paste, diffusion and differential syntax highlighting and
everything appeared reasonable.
Reviewed By: codeblock
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, codeblock, epriestley
Differential Revision: 601
2011-07-06 12:35:36 -07:00
epriestley
70cd8b1b34 Update Phabricator for split syntax highlighting API
Summary: I'll clean some of this stuff up in a followup too, but update the
callers to use the new explicit filename-based API.
Test Plan: Looked at paste, Diffusion and Differential.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 600
2011-07-06 11:56:37 -07:00
epriestley
6eed2ad2bb Deal more gracefully with unusual display changesets
Summary: These came up in dealing with the diff produced by T271. When a file is
unmodified, don't try to use the "ignore all whitespace" algorithm on it. Also,
detect "changed only by adding or removing trailing whitespace" vs "this file
was not modified" correctly.
Test Plan: Viewed the diff that came out of running 'arc diff' on my T271 mess.
Reviewed By: tuomaspelkonen
Reviewers: alex, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 548
2011-07-05 09:55:35 -07:00
Ricky Elrod
9454060c29 Add a syntax highlight dropdown, if pygments is enabled.
Summary:
- Add a default list of supported languages to default.conf.php
  and make the initial/default value customizable.
- Store a '' in the database to infer the language from the filename/title.

Test Plan:
Tested in my sandbox with pygments enabled and disabled and various
combinations of filename/extension/dropdown selection.

Reviewers:
epriestley

CC:

Differential Revision: 587
2011-07-04 12:23:43 -04:00
epriestley
a15f07cc33 Allow Phabricator to be configured to use a public Reply-To address
Summary:
We already support this (and Facebook uses it) but it is difficult to configure
and you have to write a bunch of code. Instead, provide a simple flag.

See the documentation changes for details, but when this flag is enabled we send
one email with a reply-to like "D2+public+23hf91fh19fh@phabricator.example.com".
Anyone can reply to this, and we figure out who they are based on their "From"
address instead of a unique hash. This is less secure, but a reasonable tradeoff
in many cases.

This also has the advantage over a naive implementation of at least doing object
hash validation.

@jungejason: I don't think this affects Facebook's implementation but this is an
area where we've had problems in the past, so watch out for it when you deploy.
Also note that you must set "metamta.public-replies" to true since Maniphest now
looks for that key specifically before going into public reply mode; it no
longer just tests for a public reply address being generateable (since it can
always generate one now).

Test Plan:
Swapped my local install in and out of public reply mode and commented on
objects. Got expected email behavior. Replied to public and private email
addresses.

Attacked public addresses by using them when the install was configured to
disallow them and by altering the hash and the from address. All this stuff was
rejected.

Reviewed By: jungejason
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, moskov, jungejason
Differential Revision: 563
2011-07-03 12:31:00 -07:00
epriestley
af107cff65 Explicitly list reviewers on reviewrequest email
Summary: This used to be in the subject but there was a bunch of churn and now
it's nowhere.
Test Plan: Created, updated, and added CCs to a diff.
Reviewed By: moskov
Reviewers: moskov, avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, moskov
Differential Revision: 567
2011-06-30 18:56:02 -07:00
epriestley
2b8116d7ae Fix error cues on "Edit Revision" interface
Summary: The field hints on this interface don't behave correctly. Particularly, when you add yourself as a reviewer you aren't pointed at the issue.

Test Plan: Edited a revision and tried to save invalid changes, including self-reviewership.

Reviewers: moskov, jungejason, tuomaspelkonen, aran

CC:

Differential Revision: 565
2011-06-30 13:53:22 -07:00
epriestley
553ef6f587 When sending Differential comment emails, include added CCs and reviewers
explicitly

Summary: You currently have to click through to figure out who got added.
Test Plan:
  - Made a comment which added CCs.
  - Made a comment which added reviewers.
  - Made a comment which added nothing.
  - Made a comment which added CCs and reviewers.

Reviewed By: tomo
Reviewers: tomo, jungejason, tuomaspelkonen, aran
CC: aran, tomo
Differential Revision: 562
2011-06-30 13:04:15 -07:00
epriestley
a46ae11030 Restore heavy arrow to signify linebreak. 2011-06-30 12:14:47 -07:00
mgummelt
3c785cdb5a include task ids in the commit messages returned by "arc amend"
Summary:
when "arc diff" generates a revision, it attaches a task id
if one is included.  However, "arc amend" did not return a task id,
effectively stripping it from the commit message.  This diff fixes
that.

NOTE: This is dependent on revision 549 https://secure.phabricator.com/D549

Test Plan:
0. created a custom class to append Facebook task IDs to commit messages and
attached it to the differential.append-commit-message-class config variable
1. created a new diff in the www repot
2. included Task ID: 609350 in the git commit message
3. "arc diff" to generate the revision
4. "arc amend"
5. ensure that the "Task ID:" field remained in the git commit message

Reviewed By: epriestley
Reviewers: dpepper, jungejason, epriestley
CC: aran, epriestley, mgummelt
Differential Revision: 546
2011-06-29 16:28:21 -07:00