Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Branch: master
Commits on Feb 6, 2012
  1. @epriestley
  2. @epriestley

    Reduce visibility of "Host" and "Path" Differential fields by default

    epriestley authored
    Summary:
    See discussion in T838. These fields expose information which it isn't necessary
    or useful to expose in the general case.
    
      - Disable fields by default, allow them to be enabled in config (these fields
    were useful for me at Facebook when I had access to all the machines).
      - Remove 'sourcePath' from Conduit methods other than differential.query.
      - Condition 'sourcePath' field in Conduit on the caller being the revision
    author. This is a bit hacky but not so awful.
    
    Test Plan:
      - Verified fields are gone by default and restored by configuration.
      - Verified Conduit no longer returns these fields other than
    differential.query.
      - Verified field presence/absence according to authorship in
    differential.query.
      - Grepped around in arcanist to make sure we aren't relying on sourcePath.
    There's a workflow in "arc merge" that technically might hit it, but I think
    it's unreachable, definitely irrelvant (we never use source path as a
    distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away
    Real Soon Now anyway.
    
    Reviewers: btrahan, arice
    
    Reviewed By: arice
    
    CC: aran, epriestley
    
    Maniphest Tasks: T838
    
    Differential Revision: https://secure.phabricator.com/D1582
  3. Fix displaying of raw files in Differential

    vrana authored
    Summary:
    See D1533#5.
    Also deduplicates logic of what is stored to blob in ArcanistDiffWorkflow.
    
    Blame Rev: D1533
    
    Test Plan:
    Display raw version of text file.
    Display raw version of image.
    
    Reviewers: epriestley, nh
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1583
  4. Use constants in DifferentialRevisionUpdateHistoryView

    vrana authored
    Test Plan: Display diff with whitespace changes.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1581
  5. Reorganize escaping in DifferentialRevisionUpdateHistoryView

    vrana authored
    Summary:
    Escaped $id is compared with non-escaped $max_id.
    Escaped $id is escaped again in phutil_render_tag().
    
    Note: $id is numeric :-).
    
    Test Plan: Display diff.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1580
  6. Use hsprintf() in lint message

    vrana authored
    Test Plan: Display revision with lint problems
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1578
  7. @epriestley

    Provide software protections for HTTP response splitting

    epriestley authored
    Summary:
    This addresses a few things:
    
      - Provide a software HTTP response spliting guard as an extra layer of
    security, see http://news.php.net/php.internals/57655 and who knows what HPHP/i
    does.
      - Cleans up webroot/index.php a little bit, I want to get that file under
    control eventually.
      - Eventually I want to collect bytes in/out metrics and this allows us to do
    that easily.
      - We may eventually want to write to a socket or do something else like that,
    ala Litespawn.
    
    Test Plan:
      - Ran unit tests.
      - Browsed around, checked headers and HTTP status codes.
    
    Reviewers: btrahan, vrana
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1564
Commits on Feb 5, 2012
  1. Utilize hsprintf() in OAuth

    vrana authored
    Test Plan: /oauth/facebook/login/?error=<a>
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1577
Commits on Feb 4, 2012
  1. Add Owners search to Diffusion

    vrana authored
    Test Plan:
    /diffusion/X/browse/:/file
    Click Search Owners.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1572
  2. Search owners by repository

    vrana authored
    Summary: I've chosen passing callsign even if it is more complicated because it
    is nicer than PHID and can be written by hand.
    
    Test Plan:
    Search without repository.
    Search with repository.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1571
  3. Display compatible inline comments (usually synthetic) on the same row

    vrana authored
    Summary:
    This just looks silly:
    {F8088, size=full}
    
    It runs in O(N*N) but it's not a big deal because there are usually only few
    comments per line.
    I didn't implement it for images.
    
    Test Plan:
    View revision with compatible inline comments in two diffs.
    View revision with different inline comments on same line in two diffs.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1570
Commits on Feb 3, 2012
  1. Allow full anchors in remarkup object names

    vrana authored
    Summary: Remarkup object names require #1 for linking to comments which is not
    very intuitive.
    
    Test Plan:
      D1558#4e01328c
      D1558#1
      D1558#comment-1
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1565
  2. @epriestley

    Resolve great internal confusion about left vs right inline comments

    epriestley authored
    Summary:
    This code was just all kinds of wrong, but got all the common cases anyone cares
    about correct.
    
      - In edit-inline-comments.js, if isOnRight() is true, use data.right, not
    data.left (derp).
      - Set data.left correctly, not to the same value as data.right (derp derp).
      - Set "isNewFile" based on $is_new, not $on_right (derp derp derp).
    
    Test Plan:
     - Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
    Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
    verified output was accurate in all cases.
     - Added comments to the left-display-side of a diff-of-diffs, saved them, they
    showed up where I put them.
    
    Reviewers: btrahan, vrana
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Maniphest Tasks: T543
    
    Differential Revision: https://secure.phabricator.com/D1567
  3. @bobtrahan

    Clean up initialization of Differential Show More Behavior in Maniphest

    bobtrahan authored
    Summary:
    add a static variable to the method and use it so we don't init more
    than once!
    
    Test Plan:
    add a "phlog" and noted only init'd one time.   verified "show more"
    links worked correctly.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Maniphest Tasks: T666
    
    Differential Revision: https://secure.phabricator.com/D1553
  4. Move tail to <body>

    vrana authored
    Summary:
    Prevents invalid HTML.
    Discussion at D1561#6.
    
    Test Plan:
    http://validator.w3.org/check?uri=https%3A%2F%2Fsecure.phabricator.com
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1563
  5. Avoid sending CSRF token in GET and external forms

    vrana authored
    Summary:
    Sending CSRF token in GET forms is dangerous because if there are external links
    on the target page then the token could leak through Referer header.
    The token is not required for anything because GET forms are used only to
    display data, not to perform operations.
    Sending CSRF tokens to external URLs leaks the token immediately.
    
    Please note that <form action> defaults to GET.
    
    PhabricatorUserOAuthSettingsPanelController suffered from this problem for both
    reasons.
    
    Test Plan: Save my settings (POST form).
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1558
  6. Specify encoding in <meta>

    vrana authored
    Summary: Phabricator sends information about encoding in Content-Type header but
    when I save the HTML page then this information is lost.
    
    Test Plan: /
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1561
  7. Use GET form in Owners search

    vrana authored
    Summary:
    The form doesn't perform any action, only displays data.
    URL wouldn't exceed its maximum length.
    Bookmarking results can be useful.
    Linking from other pages can be even more useful.
    Also browsing through history is more fluent.
    
    Test Plan:
    Search by path.
    Search by owner.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1559
  8. Search by full path in Owners

    vrana authored
    Summary:
    Search tool currently allows only searching by substring which is useful.
    But searching by the full path (for packages in which the path is contained) is
    probably even more useful.
    
    NOTE: I used a trick to perform the search by superstring.
    
    Packages containing path '/' are found always which could seem strange but it is
    correct after all.
    
    Test Plan:
    Search for /src/x/a.php. Found package with path /src/x/.
    Search for /src/. Found package with path /src/x/.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1560
  9. Add link to path in Owners package list

    vrana authored
    Summary: Also changes rCALL in package detail to bold CALL.
    
    Test Plan:
    /owners/view/all/, click on link
    /owners/package/1/
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1550
  10. Escape result of PhabricatorOAuthProvider::getProviderName()

    vrana authored
    Test Plan: /settings/page/facebook/
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1556
  11. Github is actually GitHub

    vrana authored
    Test Plan: none
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1557
  12. @davidreuss @epriestley

    Add conduit feed.query method

    davidreuss authored epriestley committed
    Summary: This is not totally done yet, and i'm submitting for feedback.
    
    Test Plan: Played with various settings in local conduit console.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Maniphest Tasks: T790
    
    Differential Revision: https://secure.phabricator.com/D1555
  13. @davidreuss @epriestley

    Add remarkup.process conduit method

    davidreuss authored epriestley committed
    Summary: This exposes a few remarkup engines over conduit.
    
    Test Plan:
    Local conduit console, and playing with
    
      'cat example.json | arc call-conduit remarkup.process'
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1551
  14. @epriestley

    Add a maintenance script for reconciling repositories to disk state

    epriestley authored
    Summary:
    @rguerin ran into an issue in his install where Phabricator appears to have
    discovered commits which no longer exist, and thus is failing to proceed with
    its repository import.
    
    It's not clear how we got into this state. Previously, it was possible by, e.g.,
    parsing a different repository's working copy and then switching them back, but
    there are now safeguards against that.
    
    I'm taking a three-pronged approach to try to sort this out:
    
      - Provide a script to get out of this state (this script) and reconcile
    Phabricator's view of a repository with an authoritative copy of it. This
    basically "un-discovers" any discovered commits which don't actually exist (any
    queued tasks to parse them will fail permanently when they fail to load the
    commit object).
      - Add more logging to the discovery daemon so we can figure out where commits
    came from.
      - Improve Diffusion's UI when stuff is partially discovered (T776).
    
    (This script should also clean up some nonsense on secure.phabricator.com from a
    botched Diviner import.)
    
    Test Plan: Ran "reconcile.php" with bogus commits and bogus differential/commit
    links, had them expunged. Will work with @rguerin to see if this resolves
    things.
    
    Reviewers: btrahan, rguerin
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1552
  15. @epriestley

    Wrap basic diff/revision association in a transaction

    epriestley authored
    Summary:
    This doesn't cover every case exhaustively (see comments) but should cover like
    98% of the practical cases.
    
    This makes one workflow modification: willWriteRevision() was previously
    guaranteed to have a revisionID / revisionPHID and no longer is. I verified that
    no field implementations depend on this behavior. Fields which depend on IDs
    should be using didWriteRevision() instead.
    
    Test Plan: Inserted a "throw" into the middle of the transactions and created
    revisions; they didn't orphan. Created revisions normally, they worked
    correctly.
    
    Reviewers: btrahan, nh
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Maniphest Tasks: T605
    
    Differential Revision: https://secure.phabricator.com/D1541
  16. @epriestley

    Use 'ps <pid>' to test for process existence if posix is not available

    epriestley authored
    Summary: posix may not be loaded on the web/cgi SAPI but we call posix functions
    on this pathway, which we hit on /daemon/. Fall back to exec if we don't have
    posix.
    
    Test Plan: Added "&& false" and verified the page executed a bunch of "ps"
    tests.
    
    Reviewers: Koolvin, btrahan
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Maniphest Tasks: T821
    
    Differential Revision: https://secure.phabricator.com/D1540
Commits on Feb 2, 2012
  1. XSS in Owners

    vrana authored
    Test Plan: Display /owners/view/search/ for repository with callsign <i>hack</i>
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1549
  2. Fix conduit host check

    Nick Harper authored
    Summary:
    conduit was using getProductionURI instead of getURI for checking that the
    request was sent to the correct host, which causes problems in some dev
    environments
    
    Test Plan:
      echo '{}' | arc call-conduit --conduit-uri=mydevserver
    
    where my dev server is configured with phabricator.production-uri pointing to
    prod instead of my devserver
    
    Reviewers: epriestley, btrahan, jungejason
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1543
Commits on Feb 1, 2012
  1. Add links to empty result in Diffusion

    vrana authored
    Summary: I think that these are the only links that are useful - commit which
    deleted the path and last version of the path.
    
    Test Plan: Display deleted path, click on links.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1536
  2. @epriestley
  3. Provide better UX when trying to view binary file from differential

    Nick Harper authored
    Summary:
    When a user selects "show raw file (right)" from the dropdown of a binary file
    in differential, they should get more than a blank page.
    
    Test Plan: Loaded a raw binary file from differential
    
    Reviewers: tuomaspelkonen, epriestley, jungejason
    
    Reviewed By: epriestley
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1533
Commits on Jan 31, 2012
  1. @epriestley

    Disable personal Herald rules from invalid owners

    epriestley authored
    Summary: Since mailing list rules are now "global", don't run "personal" rules
    for disabled/invalid users.
    
    Test Plan: Added a personal rule that matches every revision for a test user.
    Created a revision, checked transcript, rule matched. Disabled user, updated
    revision, checked transcript, rule got auto-disabled.
    
    Reviewers: btrahan, jungejason, nh, xela
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1517
  2. @epriestley

    Improve Herald personal/global UI/UX

    epriestley authored
    Summary:
      - Default "personal" vs "global" choice to "personal".
      - Don't show global rules under "My Rules".
      - After editing or creating a global rule, redirect back to global rule list.
      - Use radio buttons for "personal" vs "global" and add captions explaining the
    difference.
      - For "global" rules, don't show the owner/author in the rule detail view --
    they effectively have no owner (see also D1387).
      - For "global" rules, don't show the owner/author in the rule list view, as
    above.
      - For admin views, show rule type (global vs personal).
    
    Test Plan:
      - Created and edited new global and personal rules.
      - Viewed "my", "global" and "admin" views.
    
    Reviewers: btrahan, jungejason, nh, xela
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Differential Revision: https://secure.phabricator.com/D1518
  3. @epriestley

    Document final/private policy

    epriestley authored
    Summary: Provide explicit guidance in the documentation about liberal use of
    "final".
    
    Test Plan: Generated, read documentation.
    
    Reviewers: btrahan, jungejason, aran, nh
    
    Reviewed By: btrahan
    
    CC: aran, epriestley
    
    Maniphest Tasks: T795
    
    Differential Revision: https://secure.phabricator.com/D1520
Something went wrong with that request. Please try again.