diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -43,7 +43,10 @@ public function getRoutes() { return array( - '/D(?P[1-9]\d*)' => 'DifferentialRevisionViewController', + '/D(?P[1-9]\d*)' => array( + '' => 'DifferentialRevisionViewController', + '/(?Pnew)/' => 'DifferentialRevisionViewController', + ), '/differential/' => array( '(?:query/(?P[^/]+)/)?' => 'DifferentialRevisionListController', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -6,6 +6,7 @@ private $revisionID; private $changesetCount; private $hiddenChangesets; + private $warnings = array(); public function shouldAllowPublic() { return true; @@ -68,9 +69,17 @@ $revision->attachActiveDiff(last($diffs)); - $diff_vs = $request->getInt('vs'); - $target_id = $request->getInt('id'); - $target = idx($diffs, $target_id, end($diffs)); + $diff_vs = $this->getOldDiffID($revision, $diffs); + if ($diff_vs instanceof AphrontResponse) { + return $diff_vs; + } + + $target_id = $this->getNewDiffID($revision, $diffs); + if ($target_id instanceof AphrontResponse) { + return $target_id; + } + + $target = $diffs[$target_id]; $target_manual = $target; if (!$target_id) { @@ -81,10 +90,6 @@ } } - if (empty($diffs[$diff_vs])) { - $diff_vs = null; - } - $repository = null; $repository_phid = $target->getRepositoryPHID(); if ($repository_phid) { @@ -170,7 +175,7 @@ } $handles = $this->loadViewerHandles($object_phids); - $warnings = array(); + $warnings = $this->warnings; $request_uri = $request->getRequestURI(); @@ -1312,4 +1317,133 @@ ->setShowViewAll(true); } + private function getOldDiffID(DifferentialRevision $revision, array $diffs) { + assert_instances_of($diffs, 'DifferentialDiff'); + $request = $this->getRequest(); + + $diffs = mpull($diffs, null, 'getID'); + + $is_new = ($request->getURIData('filter') === 'new'); + $old_id = $request->getInt('vs'); + + // This is ambiguous, so just 404 rather than trying to figure out what + // the user expects. + if ($is_new && $old_id) { + return new Aphront404Response(); + } + + if ($is_new) { + $viewer = $this->getViewer(); + + $xactions = id(new DifferentialTransactionQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($revision->getPHID())) + ->withAuthorPHIDs(array($viewer->getPHID())) + ->setOrder('newest') + ->setLimit(1) + ->execute(); + + if (!$xactions) { + $this->warnings[] = id(new PHUIInfoView()) + ->setTitle(pht('No Actions')) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->appendChild( + pht( + 'Showing all changes because you have never taken an '. + 'action on this revision.')); + } else { + $xaction = head($xactions); + + // Find the transactions which updated this revision. We want to + // figure out which diff was active when you last took an action. + $updates = id(new DifferentialTransactionQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($revision->getPHID())) + ->withTransactionTypes( + array( + DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE, + )) + ->setOrder('oldest') + ->execute(); + + // Sort the diffs into two buckets: those older than your last action + // and those newer than your last action. + $older = array(); + $newer = array(); + foreach ($updates as $update) { + // If you updated the revision with "arc diff", try to count that + // update as "before your last action". + if ($update->getDateCreated() <= $xaction->getDateCreated()) { + $older[] = $update->getNewValue(); + } else { + $newer[] = $update->getNewValue(); + } + } + + if (!$newer) { + $this->warnings[] = id(new PHUIInfoView()) + ->setTitle(pht('No Recent Updates')) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->appendChild( + pht( + 'Showing all changes because the diff for this revision '. + 'has not been updated since your last action.')); + } else { + $older = array_fuse($older); + + // Find the most recent diff from before the last action. + $old = null; + foreach ($diffs as $diff) { + if (!isset($older[$diff->getPHID()])) { + break; + } + + $old = $diff; + } + + // It's possible we may not find such a diff: transactions may have + // been removed from the database, for example. If we miss, just + // fail into some reasonable state since 404'ing would be perplexing. + if ($old) { + $this->warnings[] = id(new PHUIInfoView()) + ->setTitle(pht('New Changes Shown')) + ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) + ->appendChild( + pht( + 'Showing changes since the last action you took on this '. + 'revision.')); + + $old_id = $old->getID(); + } + } + } + } + + if (isset($diffs[$old_id])) { + return $old_id; + } + + return null; + } + + private function getNewDiffID(DifferentialRevision $revision, array $diffs) { + assert_instances_of($diffs, 'DifferentialDiff'); + $request = $this->getRequest(); + + $diffs = mpull($diffs, null, 'getID'); + + $is_new = ($request->getURIData('filter') === 'new'); + $new_id = $request->getInt('id'); + + if ($is_new && $new_id) { + return new Aphront404Response(); + } + + if (isset($diffs[$new_id])) { + return $new_id; + } + + return (int)last_key($diffs); + } + } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -624,7 +624,9 @@ $body = new PhabricatorMetaMTAMailBody(); $body->setViewer($this->requireActor()); - $revision_uri = PhabricatorEnv::getProductionURI('/D'.$object->getID()); + $revision_uri = $object->getURI(); + $revision_uri = PhabricatorEnv::getProductionURI($revision_uri); + $new_uri = $revision_uri.'/new/'; $this->addHeadersAndCommentsToMailBody( $body, @@ -645,6 +647,21 @@ $this->appendInlineCommentsForMail($object, $inlines, $body); } + $update_xaction = null; + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: + $update_xaction = $xaction; + break; + } + } + + if ($update_xaction) { + $diff = $this->requireDiff($update_xaction->getNewValue(), true); + } else { + $diff = null; + } + $changed_uri = $this->getChangedPriorToCommitURI(); if ($changed_uri) { $body->addLinkSection( @@ -654,22 +671,15 @@ $this->addCustomFieldsToMailBody($body, $object, $xactions); + if (!$this->getIsNewObject()) { + $body->addLinkSection(pht('CHANGES SINCE LAST ACTION'), $new_uri); + } + $body->addLinkSection( pht('REVISION DETAIL'), $revision_uri); - $update_xaction = null; - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: - $update_xaction = $xaction; - break; - } - } - if ($update_xaction) { - $diff = $this->requireDiff($update_xaction->getNewValue(), true); - $body->addTextSection( pht('AFFECTED FILES'), $this->renderAffectedFilesForMail($diff)); diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -307,7 +307,7 @@ $content = phabricator_form( $this->getUser(), array( - 'action' => '#toc', + 'action' => '/D'.$revision_id.'#toc', ), array( $table,