1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 18:52:42 +01:00
Commit graph

1296 commits

Author SHA1 Message Date
epriestley
008461a395 Use getBestURI() to set file handle URIs
Summary: getBestURI() = best URI

Test Plan:
It says "best" in the name so it must be the best!

Also in Maniphest emails we'll link you to /view/ even for binaries and other
non-viewable content.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: anjali, aran

Differential Revision: https://secure.phabricator.com/D1461
2012-01-19 17:16:30 -08:00
awyler
6080d74112 Created personal vs. global herald rule distingtion
Summary:
A personal rule only has actions targeting the owner.  Likewise, only they can
edit the rule. OTOH, a global may affect any target and is editable by anyone.

There are no new action types.  Instead, type of the rule modifies the available
targets and the messaging in the ui.  This is beneficial because herald rule
adapters don't need to be aware of the difference between emailing the owner of
a personal rule and emailing an arbitrary user.

This diff sets up the logic and ui for creating personal/global rules.  All
existing rules have been defaulted to global.

TODO: Filter all existing rules into personal/global
TODO: Create a UI for surfacing (relevant?) global rules.

Test Plan:
1. Created a personal rule to email myself.  Created a dumby revision satisfying
the conditions of that rule.  Verified that I recieved a herald email.
2. Removed my adminship, change the owner of a personal rule. verified that I
couldn't edit the rule.
3.Changed rule type to global. verified that I could edit the rule.
4. Verified that admins can edit both global and personal rules.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, zizzy

Differential Revision: https://secure.phabricator.com/D1449
2012-01-19 11:21:49 -08:00
Evan Priestley
c4804aa019 Merge pull request #89 from Koolvin/f6a78452f36454101ab5c8add297657680557091
Added Fn directive to IRCbot
2012-01-19 10:56:12 -08:00
root
f6a78452f3 Added Fn directive to IRCbot D1456 2012-01-19 10:50:53 -08:00
Dave Ingram
3edf60627d Add support for marking files as "generated" by regexp against path
Summary:
Not all auto-generated files can include the magical
"generated" annotation for one reason or another, but they may follow
path rules. This patch allows files to be marked as automatically
generated by matching the path with a regular expression.

Test Plan:
Alter 'differential.generated-paths' setting in config.
Create a new diff that affects a file matching one of those regular
expressions. Verify that Differential marks it as automatically
generated and therefore probably not worth reviewing (in the same way as
the magical "generated" annotation.

Reviewers: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1455
2012-01-19 18:30:19 +00:00
tuomaspelkonen
7f9fb3597d Make updating postponed tests work again for Facebook.
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
2012-01-18 20:46:16 -08:00
epriestley
ad36865e50 Add optional "Re:" prefix to all threaded mail and allow disabling mail about
your own actions

Summary:
  - Mail.app on Lion has cumbersome threading rules, see T782. Add an option to
stick "Re: " in front of all threaded mail so it behaves. This is horrible, but
apparently the least-horrible option.
  - While I was in there, I added an option for T228.

Test Plan:
  - Sent a bunch of threaded and unthreaded mail with varous "Re:" settings,
seemed to get "Re:" in the right places.
  - Disabled email about my stuff, created a task with just me, got voided mail,
added a CC, got mail to just the CC.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, mkjones

Maniphest Tasks: T228, T782

Differential Revision: https://secure.phabricator.com/D1448
2012-01-18 15:20:50 -08:00
awyler
14d16eab17 Enable Phabricator admin to change the owner of a herald rule
Summary:
Added a typeahead in the edit herald rule page that allows an admin or
owner to change the current owner of a rule.  If the typeahead is emptied, the
current owner will remain owner.

Test Plan:
Created a test rule.  Changed the owner.  Deleted the owner in the
typahead. Verified expected behavior.

Reviewers: jungejason, epriestley

Reviewed By: epriestley

CC: aran, jungejason, epriestley, xela

Differential Revision: https://secure.phabricator.com/D1322
2012-01-18 11:59:35 -08:00
epriestley
42ddad1bc8 Add project.query to Conduit
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
2012-01-18 09:57:26 -08:00
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
vrana
ae0d9770a5 Fill <a href> in PhabricatorMenuItem
Summary: The <a href> attribute is useful because user knows where the link goes
before opening it plus he can copy it to the clipboard plus he can add it to the
bookmarks.

Test Plan:
Display revision.
View Options.
Click.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1436
2012-01-17 10:49:39 -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
jungejason
4faab06c3c Enable herald rule for commits that need auditing
Summary:
enable herald commit rules to have access to auditing info.

Note that the new herald condition I added contains info for the
packages. I thought about using a simpler herald condition like
"Requires audit is true or false" and let it work together with the
existing "Affected package contains any of the package". It doesn't work
because we need the info about the package to decide if the commit
requires audit, but the herald conditions work separately.

Test Plan:
- A commit requiring auditing was detected by a herald rule that checks
  the auditing status
- A commit not requiring auditing was not detected by a herald rule
  which checks auditing status, but was detected by a rule which doesn't
  check the auditing status

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1399
2012-01-17 09:13:07 -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
epriestley
96c08a4f37 Put file name in view URI, so downloading it with option-return creates a
usefully-named file

Summary:
If you Command-L + Option-Return to download stuff off, e.g., Paste,
you get "PHID-FILE-ad98abg9bsd9ashbs.txt" in your download folder. Put the file
name in the URI instead, so you get a reasonably named file.

Test Plan: Downloaded some files, got reasonable results.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1427
2012-01-16 17:38:10 -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
epriestley
5f1438354b Document nginx, s3 storage in Phabricator
Summary: Add nginx documentation and s3 documentation and some other doc tweaks.

Test Plan: Generated, read documentation.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, wnemay

Maniphest Tasks: T638

Differential Revision: https://secure.phabricator.com/D1426
2012-01-16 16:15:27 -08:00
epriestley
b35ea500cc Allow files to be deleted
Summary:
A couple of people mentioned that they've had users accidentally upload
sensitive files. Allow files to be deleted.

(At some point it might be nice to keep the file handle around and log who
deleted it, but this addresses the immediate problem without needing too much
work.)

Test Plan: Deleted some files.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T780

Differential Revision: https://secure.phabricator.com/D1423
2012-01-16 16:15:18 -08:00
vrana
c0592b55d7 Use generic URIs
Summary:
/diffusion/X/history/?copies=0 is same as /diffusion/X/history/
/countdown/1/?chrome=1 is same as /countdown/1/

Test Plan:
Visit /diffusion/X/history/, click on Show/Hide Copies/Branches twice.
Visit /countdown/1/, click on Disable/Enable Chrome twice.

Reviewers: epriestley, tuomaspelkonen

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D1424
2012-01-16 14:53:05 -08:00
epriestley
1651be91ec Remove daemon PID files for missing daemons when running "phd stop"
Summary: When we try to kill a daemon but discover it isn't running, we should
remove the PID file. We can also simplify the logic here.

Test Plan: Ran "phd stop" a couple of times, subsequent runs did not try to stop
a legion of dead daemons.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T781

Differential Revision: https://secure.phabricator.com/D1421
2012-01-16 12:59:41 -08:00
epriestley
56447ed2cc Add more options to Remarkup
Summary:
See D1416. Add options to file-embed syntax, and document new code and
embed options.

Test Plan: Used new options in markup blocks.

Reviewers: davidreuss, btrahan, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T336

Differential Revision: https://secure.phabricator.com/D1417
2012-01-16 11:53:16 -08:00
epriestley
82c0795e54 Unify logic for username validation
Summary: Revisit of D1254. Don't require lowercase, just standardize the logic.
The current implementation has nonuniform logic -- PeopleEditController forbids
uppercase.

Test Plan: Ran unit tests, see also D1254.

Reviewers: btrahan, jungejason, aran

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1415
2012-01-16 11:52:59 -08:00
epriestley
5333b16d7f Clarify header block documentation
Summary: Make it more explicit that headers are block formatters, see T778.

Test Plan: Read docs.

Reviewers: davidreuss, btrahan, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T778

Differential Revision: https://secure.phabricator.com/D1420
2012-01-16 11:51:54 -08:00
epriestley
80643d63a8 Detect empty $PATH environmental var
Summary:
By default, PHP-FMP (an alternate PHP FCGI SAPI) cleans the entire environment
for child processes. This means we have no $PATH.

This causes some confusing failures for reasons I don't fully understand. If you
do these things:

  exec_manual('env');
  exec_manual('export');

...they show no $PATH, as expected. If you do this:

  exec_manual('echo $PATH');

...it shows a path. And this works (i.e., it finds the executable):

  exec_manual('ls');

...but this fails (it says "no ls in ((null))"):

  exec_manual('which ls');

So, basically, the sh -c process itself gets a default PATH somehow, but its
children don't. I don't realllly get why this happens, but clearly an empty
$PATH is a misconfiguration, and can easily be remedied.

See discussion here: https://github.com/facebook/libphutil/issues/7

Test Plan: Applied patch to Centos6 + nginx + PHP-FPM machine, ran setup, the
configuration issue was detected and I was given information on resolving it.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1413
2012-01-16 11:49:19 -08:00
epriestley
f81021fa7f Improve error message for Conduit path problems
Summary:
A few people in IRC have been having issues here recently. If you misconfigure
the IRC bot, e.g., you get a 200 response back with a bunch of login HTML in it.
This is unhelpful.

Try to detect that a conduit request is going to the wrong path and raise a
concise, explicit error which is comprehensible from the CLI.

Also created a "PlainText" response and moved the IE nosniff header to the base
response object.

Test Plan: As a logged-out user, hit various nonsense with "?__conduit__=true"
in the URI. Got good error messages. Hit nonsense without it, got login screens.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T775

Differential Revision: https://secure.phabricator.com/D1407
2012-01-16 11:48: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
epriestley
025cc1376e Fix a fatal on the file info screen
Summary: D1354 added a query for a possibly-empty list -- only show the table if
there are transformations.

Test Plan: Reloaded a previously-fataling page, no fatals. Viewed a file with
transformations, got a list.

Reviewers: davidreuss, btrahan, jungejason

Reviewed By: davidreuss

CC: aran, davidreuss

Differential Revision: https://secure.phabricator.com/D1414
2012-01-16 07:01:23 -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
vrana
9ba4f24e93 Send 403 for admin pages without being admin
Summary: I've also moved the response generation for 404 from
##AphrontDefaultApplicationConfiguration## to ##buildResponseString()##

Test Plan:
Visit /
Visit /mail/
Visit /x/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, vrana

Differential Revision: https://secure.phabricator.com/D1406
2012-01-15 17:30:23 -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
cedb0c045a Lock down accepted next URI values for redirect after login
Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).

Test Plan:
  - Ran tests.
  - Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: arice, aran, epriestley, btrahan

Differential Revision: https://secure.phabricator.com/D1369
2012-01-13 11:58:45 -08:00
epriestley
b71e1c15ef Detect which PHP SAPI the CLI binary uses during setup
Summary:
  - PHP uses a SAPI ("server API") to determine how it interacts with the caller
(e.g., how to read the environment, how to read flags, what code to execute).
  - There are several different SAPIs: cli, cgi, cgi-fcgi, apache, etc.
  - Each SAPI has different behavior -- for instance, the "cgi" SAPI emits some
CGI headers unless told not to, so a script like 'echo "x"' actually echoes some
headers and then 'x' as an HTTP body.
  - In some setups, "php" may be php-cgi.
  - If you run php-cgi as "php scriptname.php" and your ENV has an existing CGI
request in it, it runs that CGI request instead of the script. This causes an
infinite loop.
  - Add checks to verify that "php" is the "cli" SAPI binary, not some other
SAPI.
  - In particular, cPanel uses suphp and is affected by this configuration
issue. See this thread:
https://lists.marsching.com/pipermail/suphp/2008-September/002036.html

Test Plan:
  - On a cPanel + suphp machine, ran setup and was stopped for having the
"cgi-fcgi" SAPI instead of throw into an infinite loop.
  - Applied the suggested remedy, setup now runs fine.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Differential Revision: https://secure.phabricator.com/D1390
2012-01-13 11:54:22 -08:00
Bob Trahan
cf61f0e32d Adding an "ssh" client for conduit
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
2012-01-13 11:54:13 -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
epriestley
a88e132179 Minor documentation improvements
Summary:
  - Link to "importing a repository" from Config next steps, since it's not
obvious (and the article isn't obviously named).
  - Some minor doc tweaks.
  - Remove "Roadmap" document since it's super out of date and not very useful.

Test Plan: Regenerated and read documentation.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T743

Differential Revision: https://secure.phabricator.com/D1384
2012-01-12 20:08:19 -08:00
epriestley
f5e1e3377c Add basic draft support to Phriction
Summary:
  - When a user is creating a Phriction document, save a draft as
"phriction:<slug>".
  - When a user is editing a Phriction document, save a draft as "<document
phid>:<document version>".
  - If a user has an available draft, use that instead of the native content.
  - If using a draft, tell the user and give them an option to discard it.
  - If a page is updated, your draft is lost (we show new page content
unconditionally) but this should be rare and is the simplest way to resolve this
issue in a realtively consistent way.

Test Plan:
  - Recovered drafts for new and edited pages.
  - Used "nodraft" to discard drafts.

Reviewers: davidreuss, btrahan, jungejason

Reviewed By: davidreuss

CC: aran, davidreuss

Maniphest Tasks: T769

Differential Revision: https://secure.phabricator.com/D1378
2012-01-12 18:04:20 -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
bfbe6ec594 Prevent login brute forcing with captchas
Summary: If a remote address has too many recent login failures, require they
fill out a captcha before they can attempt to login.

Test Plan: Tried to login a bunch of times, then submitted the CAPTHCA form with
various combinations of valid/invalid passwords and valid/invalid captchas.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Maniphest Tasks: T765

Differential Revision: https://secure.phabricator.com/D1379
2012-01-12 15:22:05 -08:00
epriestley
7f7710a24d Add @phutil-external-symbol declarations to Phabricator
Summary: See D1381.

Test Plan: Ran "arc liberate src/ --all" and got a clean rebuild.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T762

Differential Revision: https://secure.phabricator.com/D1382
2012-01-12 15:20:18 -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
epriestley
02fb5fea89 Allow configuration of a minimum password length, unify password reset
interfaces

Summary:
  - We have a hard-coded minimum length of 3 right now (and 1 in the other
interface), which is sort of silly.
  - Provide a more reasonable default, and allow it to be configured.
  - We have two password reset interfaces, one of which no longer actually
requires you to verify you own the account. This is more than a bit derp.
  - Merge the interfaces into one, using either an email token or the account's
current password to let you change the password.

Test Plan:
  - Reset password on an account.
  - Changed password on an account.
  - Created a new account, logged in, set the password.
  - Tried to set a too-short password, got an error.

Reviewers: btrahan, jungejason, nh

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T766

Differential Revision: https://secure.phabricator.com/D1374
2012-01-12 07:39:13 -08:00
epriestley
8b3ab97d64 Fix fatal on project list
Summary:
Until T605 gets fixed, you might end up with a Project without a Profile if the
Profile insert failed. This fatals the list view; instead, don't fatal if a
profile is missing.

(At some point we should probably just merge this field into the Project object,
I was just mimicking the user/profile separation but we have partial-field
object support now and Projects aren't super heavily used or very big.)

Test Plan:
  - Viewed list view including a project with a missing profile.
  - Edited the project, creating its profile.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: arice, aran, btrahan

Differential Revision: https://secure.phabricator.com/D1368
2012-01-12 06:52:24 -08:00
jungejason
12d1379dee Add instructions about how to support localhost
Summary:
With T764, http://localhost doesn't work anymore. So add instructions
about how to support it by modifying the hosts file.

Test Plan:
- turned on setup mode and the error message did show up
- turned off the setup mode and the error message also showed up

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T764

Differential Revision: https://secure.phabricator.com/D1370
2012-01-11 18:09:14 -08:00
epriestley
65a56c6ce0 Improve mailing list edit form
Summary:
  - Add some captions to make it more clear what these fields mean.
  - Require "name", since tokenizers use it exclusively.
  - Limit URI to allowed protocols, since admins can currently XSS users by
entering a "javascript:" URI and then tricking the user into clicking the
mailing list name. This exploit is dumb, but technically privilege escallation.

Test Plan:
  - Created a new mailing list.
  - Edited a mailing list.
  - Tested URI: valid, invalid, omitted.
  - Tested name: valid, omitted.

Reviewers: btrahan, jungejason, davidreuss

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1365
2012-01-11 15:48:21 -08:00
kevin
beaee478d3 Added conduit method to get maniphest transactions
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
2012-01-11 09:13:59 -08:00
epriestley
b10fa8023f Support Git implicit file:// URIs
Summary: @s reported an issue with implicit file:// URIs in Git, see P270.
Recognize and handle URIs in this format. For URIs we don't understand, raise an
exception.

Test Plan:
  - Added failing tests.
  - Fixed code.
  - Tests pass.

Reviewers: btrahan, jungejason, s

Reviewed By: s

CC: aran, epriestley, s

Differential Revision: https://secure.phabricator.com/D1362
2012-01-11 09:00:18 -08:00
epriestley
d75007cf42 Validate logins, and simplify email password resets
Summary:
  - There are some recent reports of login issues, see T755 and T754. I'm not
really sure what's going on, but this is an attempt at getting some more
information.
  - When we login a user by setting 'phusr' and 'phsid', send them to
/login/validate/ to validate that the cookies actually got set.
  - Do email password resets in two steps: first, log the user in. Redirect them
through validate, then give them the option to reset their password.
  - Don't CSRF logged-out users. It technically sort of works most of the time
right now, but is silly. If we need logged-out CSRF we should generate it in
some more reliable way.

Test Plan:
  - Logged in with username/password.
  - Logged in with OAuth.
  - Logged in with email password reset.
  - Sent bad values to /login/validate/, got appropriate errors.
  - Reset password.
  - Verified next_uri still works.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, j3kuntz

Maniphest Tasks: T754, T755

Differential Revision: https://secure.phabricator.com/D1353
2012-01-11 08:25:55 -08:00
epriestley
af37b637f5 Detect un-cookieable domain confiugration and explode
Summary:
Chrome/Chromium won't set cookies on these domains, at least under
Ubuntu. See T754. Detect brokenness and explode.

Test Plan:
Logged into phabricator as "http://derps/" (failed) and
"http://derps.com/" (worked) in Chromium. Set config to "http://derps/" (config
exploded) and "http://local.aphront.com/" (config OK).

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T754

Differential Revision: https://secure.phabricator.com/D1355
2012-01-11 08:12:50 -08:00
Andrew Gallagher
840eb46d03 Match unittest results by name or file
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
2012-01-10 16:51:14 -08:00
Andrew Gallagher
48f53ba095 Allow updating diff with results for new unit tests
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
2012-01-10 16:18:50 -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
epriestley
b9cac3bcd1 Improve phabot handling of private messages
Summary: When private messaged, the bot responds via private message to the
sender, instead of sending a private message to itself.

Test Plan: Mentioned tasks in public channels and private messages.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T274

Differential Revision: https://secure.phabricator.com/D1350
2012-01-10 15:11:45 -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
epriestley
5f8711ebf8 Minor, fix CSRF error caused by D1329. 2012-01-09 13:47:33 -08:00
epriestley
a2349e82ba Remove dead link from install documentation
Summary: XHProf install documentation went missing a month or two ago (see T725)
and doesn't work in the widely deployed versions of PEAR/PECL. Provide
build-from-source instructions inline.

Test Plan: Generated, read documentation.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T725

Differential Revision: https://secure.phabricator.com/D1345
2012-01-09 11:43:43 -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
epriestley
79205481e6 Minor, fix \n in MetaMTA headers. 2012-01-08 10:26:16 -08:00
Jason Ge
3a142c8788 Fix undefined index header.generate-toc in Differential
Summary:
getMarkupEngineDefaultConfiguration() is not setting
header.generate-toc. This is causing "undefined index" in Differential
page.

Test Plan:
  - Differential reviion page renders correctly
  - Phriction page renders correctly

Reviewers: epriestley, tuomaspelkonen, nh

Reviewed By: nh

CC: aran, nh

Differential Revision: https://secure.phabricator.com/D1342
2012-01-06 23:52:39 -08:00
epriestley
684d12d5db Add an example notification handler to the IRC bot
Summary: Simple notificaiton handler that reads the difx event timeline and
posts notifications to IRC.

Test Plan: Ran it in #phabricator.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1337
2012-01-06 15:09:55 -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
5e486db59d Enable Table of Contents in Phriction
Summary: Use the TOC stuff introduced in D1334.

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

Reviewers: btrahan, jungejason, davidreuss

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1335
2012-01-06 11:52:50 -08:00
epriestley
4579f23f63 Add a "maniphest.update" Conduit method
Summary:
  - Add maniphest.update
  - Add support for auxiliary fields to maniphest.createtask

Test Plan:
  - Created tasks with maniphest.createtask
  - Updated tasks with maniphest.update

Reviewers: btrahan, jungejason, zeeg

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1330
2012-01-06 11:52:00 -08:00
epriestley
b5fbf8eaa8 Use a simpler footer style to accommodate overly wide pages
Summary: Not really thrilled about my fix for T684 in D1224. This makes some
design tweaks to solve it without the awkward horizontal scrollbar in the page
content div.

Test Plan: Looked at diffs overflowing the window. Looked at footer on several
pages.

Reviewers: btrahan, jungejason, Makinde

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T684

Differential Revision: https://secure.phabricator.com/D1332
2012-01-06 11:51:49 -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
58f2cb2509 Provide a script for batch creating user accounts
Summary: Make it a little easier to create a bunch of accounts if your company
has more than like 5 employees.

Test Plan: Ran "add_user.php" to create new users. Created new users from the
web console.

Reviewers: btrahan, jungejason, rguerin

Reviewed By: btrahan

CC: aran, btrahan, rguerin

Differential Revision: https://secure.phabricator.com/D1336
2012-01-06 11:50:51 -08:00
epriestley
d16454d45d Improve a race condition in session establishment code
Summary:
If you try to establish several sessions quickly (e.g., by running several
copies of "arc" at once, as in "arc x | arc y"), the current logic has a high
chance of making them all pick the same conduit session to refresh (since it's
the oldest one when each process selects the current sessions). This means they
all issue updates against "conduit-3" (or whatever) and one ends up with a bogus
session.

Instead, do an update against the table with the session key we read, so only
one process wins the race. If we don't win the race, try again until we do or
have tried every session slot.

Test Plan:
  - Wiped conduit sessions, ran arc commands to verify the fresh session case.
  - Ran a bunch of arc piped to itself, e.g. "arc list | arc list | arc list |
...". It succeeds up to the session limit, and above that gets failures as
expected.
  - Manually checked the session table to make sure things seemed reasonable
there.
  - Generally ran a bunch of arc commands.
  - Logged out and logged in on the web interface.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T687

Differential Revision: https://secure.phabricator.com/D1329
2012-01-06 11:33:03 -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
epriestley
9c5a6601d6 Remove some Mercurial and Google caveats
Summary: These seem to work relatively reasonably and don't have any known
deal-breaking failures.

Test Plan: shrug~

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1324
2012-01-05 14:14:06 -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
1c1fe96bec Add more keyboard navigation options for inline comments
Summary:
  - Use n/p to jump between comments.
  - Use r to reply to the selected comment.
  - Use e to edit the selected comment.

Test Plan: Verified n, p, r, e, j, k, J, K, "click edit" and "click reply"
behavior in as many weird cases as I could come up with.

Reviewers: btrahan, jungejason, nh, cpiro, jl

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Maniphest Tasks: T583

Differential Revision: https://secure.phabricator.com/D1308
2012-01-05 12:58:05 -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
epriestley
2b3d7e757e Minor, fix number_format() warning. 2012-01-05 09:09:36 -08:00
vrana
015a6d2989 Undelete celerity map
Summary: Deleted by rP460efc448932279727928f46b892f433fd277928

Test Plan: View any page

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1317
2012-01-04 17:15:43 -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
c289ec304a Document why "accept" is sticky
Summary: This is a fairly common question but I think it's the right product
behavior, document it so I can reference the docs next time it comes up.

Test Plan: Generated and read documentation.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T735

Differential Revision: https://secure.phabricator.com/D1310
2012-01-04 14:15:03 -08:00
Nick Harper
468d2eaa10 Make aphront-side-nav more clear re what's a link
Summary:
Create a visual hierarchy with the <span>s and <a>s in the aphront-side-nav
so people don't try to click on a span thinking it's a link. This is to
help specifically with the case of the "All Revisions" header on the
differential revision list page - I've had a few people ask about that
broken link.

Test Plan:
Loaded the differential revision list view and the maniphest task list
view to check that their left-hand navs look ok (did this in ff8 on ubuntu
11.10).

Reviewers: epriestley, jungejason, btrahan

Reviewed By: jungejason

CC: aran, jungejason, epriestley

Differential Revision: https://secure.phabricator.com/D1303
2012-01-04 11:40:02 -08:00
epriestley
d43dec1d12 Make it harder to miss errors and warnings while developing Phabricator
Summary:
If a page generates warnings or errors, you only get a little red dot in
DarkConsole which is hard to see. DarkConsole is also fairly big and there are
plenty of reasons not to leave it open all the time.

Instead, unconditionally show a big message to developers if there are errors or
warnings.

We could make this more sophisticated eventually, but the value is just that you
see it.

Test Plan: Browsed pages with and without warnings, got the right banner state.

Reviewers: nh, btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T734

Differential Revision: https://secure.phabricator.com/D1307
2012-01-04 10:21:00 -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
epriestley
2e9bb62fe7 Show entire page weight in DarkConsole
Summary:
This provides an easier way to get a quick handle on page costs without
installing XHProf, which can be a bit complicated.

  - We currently show an "All" line, but it means "All Services".
  - Rename "All" to "All Services".
  - Add "Entire Page".

Test Plan: Looked at the services tab, saw "All Services" and "Entire Page".

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1305
2012-01-04 10:20:41 -08:00
epriestley
99231ebba4 Allow Git repositories to track only some branches
Summary:
Some installs use Git as the backbone of a CI framework or use a Git remote to
share patches. The tracker scripts currently recognize associated revisions as
"Committed" when they appear in any branch, even if that branch is
"alincoln-personal-development_test_hack" or whatever.

To address the broadest need here, allow Git repositories to be configured to
track only certain branches instead of all branches.

This doesn't allow you to import a branch into Diffusion but ignore it in
Differential. Supporting that is somewhat technically complicated because the
parser currently goes like this:

  - Look at HEAD of all branches.
  - For any commits we haven't seen before, follow them back to something we
have seen (or the root).
  - "Discover" everything new.

Since this doesn't track <branch, commit> pairs, we currently don't have enough
information to tell when a commit appears in a branch for the first time, so we
don't have anywhere we can put a test for whether that branch is tracked and do
the Differential hook only if it is.

However, I think this cruder patch satisfies most of the need and is simple and
obvious in its implementation.

See also D1263.

Test Plan:
  - Updated a Git repository with various filters: "", "master, remote", "derp",
"  ,,, master   ,,,,,"
  - Edited SVN and Mercurial repositories to verify they didn't get caught in
the crossfire.
  - Ran daemon in debug mode on libphutil with filter "derp", got exception
about no tracked branches. Ran with filter "master", got tracking. Ran with no
filter, got tracking.
  - Looked at Diffusion with "derp" and "master", saw no branches and "master"
respectively.
  - Added unit tests to cover filtering logic.

Reviewers: btrahan, jungejason, nh, fratrik

Reviewed By: fratrik

CC: aran, fratrik, epriestley

Maniphest Tasks: T270

Differential Revision: https://secure.phabricator.com/D1290
2012-01-04 10:20:34 -08:00
epriestley
ec1df21bef Add getStrList() to AphrontRequest
Summary:
  - We have a few places where we do some kind of ad-hoc comma list tokenizing,
and I'm adding another one in D1290. Add a helper to the request object.
  - Add some unit tests.

Test Plan:
  - Ran unit tests.
  - Used PHID manager, Maniphest custom view, and Repository project editor.

Reviewers: btrahan, fratrik, jungejason

Reviewed By: btrahan

CC: aran, btrahan, epriestley

Differential Revision: https://secure.phabricator.com/D1302
2012-01-04 10:18:46 -08:00
epriestley
7831b92427 Improve LiskDAO::__call() performance
Summary:
This is kind of expensive and can be significant on, e.g., the
Maniphest task list view. Do a little more caching and some clever nonsense to
improve performance.

Test Plan:
Local cost on Maniphest "all tasks" view for this method dropped from
##82,856us## to ##24,607us## on 9,061 calls.

I wrote some unit test / microbenchmark things:

  public function testGetIDCost() {
    $u = new PhabricatorUser();
    $n = 100000;
    while ($n--) {
      $u->getID();
    }
    $this->assertEqual(1, 1);
  }

  public function testGetCost() {
    $u = new PhabricatorUser();
    $n = 100000;
    while ($n--) {
      $u->getUsername();
    }
    $this->assertEqual(1, 1);
  }

  public function testSetCost() {
    $u = new PhabricatorUser();
    $n = 100000;
    while ($n--) {
      $u->setID(1);
    }
    $this->assertEqual(1, 1);
  }

Before:

   PASS  598ms   testSetCost
   PASS  584ms   testGetCost
   PASS  272ms   testGetIDCost

After:

   PASS  170ms   testSetCost
   PASS  207ms   testGetCost
   PASS   29ms   testGetIDCost

Also, ran unit tests.

Reviewers: nh, btrahan, jungejason

Reviewed By: nh

CC: aran, epriestley, nh

Differential Revision: https://secure.phabricator.com/D1291
2012-01-03 22:03:34 -08:00
Andrew Gallagher
8f289e6687 Add documentaion about literal block
Test Plan: none, not sure how to test this

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1299
2012-01-03 19:01:13 -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
Andrew Gallagher
48f72b57b2 Enable use of remarkup literal block
Summary:
D1293 adds support for a literal block in remarkup.  This diff enables
it in phabricator with a few basic rules (for line breaks, escaping HTML,
and linkifying URLs).

Test Plan: Tested in sandbox

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1297
2012-01-03 18:00:45 -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