Summary:
We show the contextual branch (always the repository default branch) when
viewing a commit. Instead, show all branches the commit appears on.
Also pull some of the duplicated DiffusionXQuery stuff into a DiffusionQuery
base class, I'll do a followup to reduce more duplication.
Test Plan: Looked at a commit in Git. My HG and SVN setups are a little borked
so I kind of faked tests in them -- I'm fixing them now.
Reviewers: btrahan, jungejason, fratrik
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T768
Differential Revision: https://secure.phabricator.com/D1458
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
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
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
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
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
Summary: We purge the cache for primary comments only right now, not inline
comments.
Test Plan: Ran "purge_cache.php --differential".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T683
Differential Revision: https://secure.phabricator.com/D1451
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
Summary: Add a conduit method to query project information.
Test Plan: Ran method from API test console.
Reviewers: bill, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1444
Summary: 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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
- Run "phd stop" before stopping apache. This is essentially a smoke test for
PHABRICATOR_ENV being set.
- Run documentation generation after everything else. Between the pull and the
restart we have some minor exposure to APC issues with deleted files and
out-of-date module definitions, and this limits that.
- Pull commands out of (x && y) stuff, this prevents "set -e" from working
correctly.
Test Plan: Ran upgrade script locally.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1419
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
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
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
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
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
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
Summary: /xhprof/ doesn't exist at all, /mail/ is only for admins
Test Plan: Display directory
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1405
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
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
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
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
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
databases
Summary: This is a stopgap patch until we fix T345/T327 properly, but you can't
add a 512-character key to a UTF-8 column because it creates an index on more
than 767 bytes, which is the key length limit.
Test Plan: Ran schmea upgrade script.
Reviewers: indiefan, btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, xela, jungejason
Maniphest Tasks: T327
Differential Revision: https://secure.phabricator.com/D1391
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
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
Summary: ..."ssh" is in quotes 'cuz this is step 1 and there's no ssh in sight
at the moment.
Test Plan:
ran api.php PHID-USER-xee4ju2teq7mflitwfcs differential.query a few times...
- tried valid input, it worked!
- tried bad input, it worked in that it failed and told me so!
ran api.php crap_user differential.query a few times...
- verified error message with respect to crap_user
ran api.php PHID-USER-xee4ju2teq7mflitwfcs crap_method a few times...
- verified error message with respect to crap_method
visited http://phabricator.dev/conduit/method/differential.query a few times...
- tried valid input, it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D1357
Summary: 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
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
