From 2ff5541fc59c4be7abd733a39e12db8358004f7a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Dec 2013 11:59:41 -0800 Subject: [PATCH] Record new commits in the push log Summary: Ref T4195. Like the previous diffs, these both create a useful log and give us an object to hand off to Herald. Surface this information in Diffusion, too, and clean things up a little bit. Test Plan: {F87565} Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4195 Differential Revision: https://secure.phabricator.com/D7718 --- .../controller/DiffusionCommitController.php | 94 ++++++++++++++----- .../engine/DiffusionCommitHookEngine.php | 63 +++++++++---- .../PhabricatorRepositoryPushLogQuery.php | 26 +++++ src/view/phui/PHUIStatusListView.php | 1 + 4 files changed, 144 insertions(+), 40 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 067fe72a37..bd69e42870 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -403,8 +403,10 @@ final class DiffusionCommitController extends DiffusionController { array $audit_requests) { assert_instances_of($parents, 'PhabricatorRepositoryCommit'); - $user = $this->getRequest()->getUser(); + $viewer = $this->getRequest()->getUser(); $commit_phid = $commit->getPHID(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); $edge_query = id(new PhabricatorEdgeQuery()) ->withSourcePHIDs(array($commit_phid)) @@ -440,6 +442,29 @@ final class DiffusionCommitController extends DiffusionController { } } + // NOTE: We should never normally have more than a single push log, but + // it can occur naturally if a commit is pushed, then the branch it was + // on is deleted, then the commit is pushed again (or through other similar + // chains of events). This should be rare, but does not indicate a bug + // or data issue. + + // NOTE: We never query push logs in SVN because the commiter is always + // the pusher and the commit time is always the push time; the push log + // is redundant and we save a query by skipping it. + + $push_logs = array(); + if ($repository->isHosted() && !$repository->isSVN()) { + $push_logs = id(new PhabricatorRepositoryPushLogQuery()) + ->setViewer($viewer) + ->withRepositoryPHIDs(array($repository->getPHID())) + ->withNewRefs(array($commit->getCommitIdentifier())) + ->withRefTypes(array(PhabricatorRepositoryPushLog::REFTYPE_COMMIT)) + ->execute(); + foreach ($push_logs as $log) { + $phids[] = $log->getPusherPHID(); + } + } + $handles = array(); if ($phids) { $handles = $this->loadViewerHandles($phids); @@ -494,13 +519,52 @@ final class DiffusionCommitController extends DiffusionController { } } - $props['Committed'] = phabricator_datetime($commit->getEpoch(), $user); + if (!$repository->isSVN()) { + $authored_info = id(new PHUIStatusItemView()); + // TODO: In Git, a distinct authorship date is available. When present, + // we should show it here. - $author_phid = $data->getCommitDetail('authorPHID'); - if ($data->getCommitDetail('authorPHID')) { - $props['Author'] = $handles[$author_phid]->renderLink(); - } else { - $props['Author'] = $data->getAuthorName(); + $author_phid = $data->getCommitDetail('authorPHID'); + $author_name = $data->getAuthorName(); + if ($author_phid) { + $authored_info->setTarget($handles[$author_phid]->renderLink()); + } else if (strlen($author_name)) { + $authored_info->setTarget($author_name); + } + + $props['Authored'] = id(new PHUIStatusListView()) + ->addItem($authored_info); + } + + $committed_info = id(new PHUIStatusItemView()) + ->setNote(phabricator_datetime($commit->getEpoch(), $viewer)); + + $committer_phid = $data->getCommitDetail('committerPHID'); + $committer_name = $data->getCommitDetail('committer'); + if ($committer_phid) { + $committed_info->setTarget($handles[$committer_phid]->renderLink()); + } else if (strlen($committer_name)) { + $committed_info->setTarget($committer_name); + } else if ($author_phid) { + $committed_info->setTarget($handles[$author_phid]->renderLink()); + } else if (strlen($author_name)) { + $committed_info->setTarget($author_name); + } + + $props['Comitted'] = id(new PHUIStatusListView()) + ->addItem($committed_info); + + if ($push_logs) { + $pushed_list = new PHUIStatusListView(); + + foreach ($push_logs as $push_log) { + $pushed_item = id(new PHUIStatusItemView()) + ->setTarget($handles[$push_log->getPusherPHID()]->renderLink()) + ->setNote(phabricator_datetime($push_log->getEpoch(), $viewer)); + $pushed_list->addItem($pushed_item); + } + + $props['Pushed'] = $pushed_list; } $reviewer_phid = $data->getCommitDetail('reviewerPHID'); @@ -508,16 +572,6 @@ final class DiffusionCommitController extends DiffusionController { $props['Reviewer'] = $handles[$reviewer_phid]->renderLink(); } - $committer = $data->getCommitDetail('committer'); - if ($committer) { - $committer_phid = $data->getCommitDetail('committerPHID'); - if ($data->getCommitDetail('committerPHID')) { - $props['Committer'] = $handles[$committer_phid]->renderLink(); - } else { - $props['Committer'] = $committer; - } - } - if ($revision_phid) { $props['Differential Revision'] = $handles[$revision_phid]->renderLink(); } @@ -530,8 +584,6 @@ final class DiffusionCommitController extends DiffusionController { $props['Parents'] = phutil_implode_html(" \xC2\xB7 ", $parent_links); } - $request = $this->getDiffusionRequest(); - $props['Branches'] = phutil_tag( 'span', array( @@ -545,7 +597,7 @@ final class DiffusionCommitController extends DiffusionController { ), pht('Unknown')); - $callsign = $request->getRepository()->getCallsign(); + $callsign = $repository->getCallsign(); $root = '/diffusion/'.$callsign.'/commit/'.$commit->getCommitIdentifier(); Javelin::initBehavior( 'diffusion-commit-branches', @@ -554,7 +606,7 @@ final class DiffusionCommitController extends DiffusionController { $root.'/tags/' => 'commit-tags', )); - $refs = $this->buildRefs($request); + $refs = $this->buildRefs($drequest); if ($refs) { $props['References'] = $refs; } diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 23a334c1c4..b76c4b8549 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -18,6 +18,7 @@ final class DiffusionCommitHookEngine extends Phobject { private $subversionRepository; private $remoteAddress; private $remoteProtocol; + private $transactionKey; public function setRemoteProtocol($remote_protocol) { $this->remoteProtocol = $remote_protocol; @@ -37,6 +38,23 @@ final class DiffusionCommitHookEngine extends Phobject { return $this->remoteAddress; } + private function getRemoteAddressForLog() { + // If whatever we have here isn't a valid IPv4 address, just store `null`. + // Older versions of PHP return `-1` on failure instead of `false`. + $remote_address = $this->getRemoteAddress(); + $remote_address = max(0, ip2long($remote_address)); + $remote_address = nonempty($remote_address, null); + return $remote_address; + } + + private function getTransactionKey() { + if (!$this->transactionKey) { + $entropy = Filesystem::readRandomBytes(64); + $this->transactionKey = PhabricatorHash::digestForIndex($entropy); + } + return $this->transactionKey; + } + public function setSubversionTransactionInfo($transaction, $repository) { $this->subversionTransaction = $transaction; $this->subversionRepository = $repository; @@ -89,6 +107,18 @@ final class DiffusionCommitHookEngine extends Phobject { return $err; } + private function newPushLog() { + return PhabricatorRepositoryPushLog::initializeNewLog($this->getViewer()) + ->setRepositoryPHID($this->getRepository()->getPHID()) + ->setEpoch(time()) + ->setRemoteAddress($this->getRemoteAddressForLog()) + ->setRemoteProtocol($this->getRemoteProtocol()) + ->setTransactionKey($this->getTransactionKey()) + ->setRejectCode(PhabricatorRepositoryPushLog::REJECT_ACCEPT) + ->setRejectDetails(null); + } + + /** * @task git */ @@ -106,34 +136,17 @@ final class DiffusionCommitHookEngine extends Phobject { // TODO: Now, do content checks. // TODO: Generalize this; just getting some data in the database for now. - $transaction_key = PhabricatorHash::digestForIndex( - Filesystem::readRandomBytes(64)); - - // If whatever we have here isn't a valid IPv4 address, just store `null`. - // Older versions of PHP return `-1` on failure instead of `false`. - $remote_address = $this->getRemoteAddress(); - $remote_address = max(0, ip2long($remote_address)); - $remote_address = nonempty($remote_address, null); - - $remote_protocol = $this->getRemoteProtocol(); $logs = array(); foreach ($updates as $update) { - $log = PhabricatorRepositoryPushLog::initializeNewLog($this->getViewer()) - ->setRepositoryPHID($this->getRepository()->getPHID()) - ->setEpoch(time()) - ->setRemoteAddress($remote_address) - ->setRemoteProtocol($remote_protocol) - ->setTransactionKey($transaction_key) + $log = $this->newPushLog() ->setRefType($update['type']) ->setRefNameHash(PhabricatorHash::digestForIndex($update['ref'])) ->setRefNameRaw($update['ref']) ->setRefNameEncoding(phutil_is_utf8($update['ref']) ? 'utf8' : null) ->setRefOld($update['old']) ->setRefNew($update['new']) - ->setMergeBase(idx($update, 'merge-base')) - ->setRejectCode(PhabricatorRepositoryPushLog::REJECT_ACCEPT) - ->setRejectDetails(null); + ->setMergeBase(idx($update, 'merge-base')); $flags = 0; if ($update['operation'] == 'create') { @@ -151,6 +164,18 @@ final class DiffusionCommitHookEngine extends Phobject { $logs[] = $log; } + // Now, build logs for all the commits. + // TODO: Generalize this, too. + $commits = array_mergev(ipull($updates, 'commits')); + $commits = array_unique($commits); + foreach ($commits as $commit) { + $log = $this->newPushLog() + ->setRefType(PhabricatorRepositoryPushLog::REFTYPE_COMMIT) + ->setRefNew($commit) + ->setChangeFlags(PhabricatorRepositoryPushLog::CHANGEFLAG_ADD); + $logs[] = $log; + } + foreach ($logs as $log) { $log->save(); } diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php index 9eae4263aa..e2e640f3cc 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php @@ -6,6 +6,8 @@ final class PhabricatorRepositoryPushLogQuery private $ids; private $repositoryPHIDs; private $pusherPHIDs; + private $refTypes; + private $newRefs; public function withIDs(array $ids) { $this->ids = $ids; @@ -22,6 +24,16 @@ final class PhabricatorRepositoryPushLogQuery return $this; } + public function withRefTypes(array $ref_types) { + $this->refTypes = $ref_types; + return $this; + } + + public function withNewRefs(array $new_refs) { + $this->newRefs = $new_refs; + return $this; + } + protected function loadPage() { $table = new PhabricatorRepositoryPushLog(); $conn_r = $table->establishConnection('r'); @@ -86,6 +98,20 @@ final class PhabricatorRepositoryPushLogQuery $this->pusherPHIDs); } + if ($this->refTypes) { + $where[] = qsprintf( + $conn_r, + 'refType IN (%Ls)', + $this->refTypes); + } + + if ($this->newRefs) { + $where[] = qsprintf( + $conn_r, + 'refNew IN (%Ls)', + $this->newRefs); + } + $where[] = $this->buildPagingClause($conn_r); return $this->formatWhereClause($where); diff --git a/src/view/phui/PHUIStatusListView.php b/src/view/phui/PHUIStatusListView.php index 64c115ecf0..9200da4531 100644 --- a/src/view/phui/PHUIStatusListView.php +++ b/src/view/phui/PHUIStatusListView.php @@ -6,6 +6,7 @@ final class PHUIStatusListView extends AphrontTagView { public function addItem(PHUIStatusItemView $item) { $this->items[] = $item; + return $this; } protected function canAppendChild() {