diff --git a/bin/lock b/bin/lock new file mode 120000 index 0000000000..686fa38798 --- /dev/null +++ b/bin/lock @@ -0,0 +1 @@ +../scripts/setup/manage_lock.php \ No newline at end of file diff --git a/resources/sql/autopatches/20140212.dx.1.armageddon.php b/resources/sql/autopatches/20140212.dx.1.armageddon.php index 021e886629..a7e978b559 100644 --- a/resources/sql/autopatches/20140212.dx.1.armageddon.php +++ b/resources/sql/autopatches/20140212.dx.1.armageddon.php @@ -75,7 +75,7 @@ foreach ($rows as $row) { if ($diff_id || $row['action'] == DifferentialAction::ACTION_UPDATE) { $xactions[] = array( - 'type' => DifferentialTransaction::TYPE_UPDATE, + 'type' => DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE, 'old' => null, 'new' => $diff_id, ); diff --git a/resources/sql/autopatches/20180305.lock.01.locklog.sql b/resources/sql/autopatches/20180305.lock.01.locklog.sql new file mode 100644 index 0000000000..fa10c21c07 --- /dev/null +++ b/resources/sql/autopatches/20180305.lock.01.locklog.sql @@ -0,0 +1,9 @@ +CREATE TABLE {$NAMESPACE}_daemon.daemon_locklog ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + lockName VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT}, + lockReleased INT UNSIGNED, + lockParameters LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + lockContext LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180306.opath.01.digest.sql b/resources/sql/autopatches/20180306.opath.01.digest.sql new file mode 100644 index 0000000000..2418366fc5 --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.01.digest.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_path + ADD pathIndex BINARY(12) NOT NULL; diff --git a/resources/sql/autopatches/20180306.opath.02.digestpopulate.php b/resources/sql/autopatches/20180306.opath.02.digestpopulate.php new file mode 100644 index 0000000000..a6b817fc46 --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.02.digestpopulate.php @@ -0,0 +1,19 @@ +establishConnection('w'); + +foreach (new LiskMigrationIterator($table) as $path) { + $index = PhabricatorHash::digestForIndex($path->getPath()); + + if ($index === $path->getPathIndex()) { + continue; + } + + queryfx( + $conn, + 'UPDATE %T SET pathIndex = %s WHERE id = %d', + $table->getTableName(), + $index, + $path->getID()); +} diff --git a/resources/sql/autopatches/20180306.opath.03.purge.php b/resources/sql/autopatches/20180306.opath.03.purge.php new file mode 100644 index 0000000000..91a15e2a58 --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.03.purge.php @@ -0,0 +1,22 @@ +establishConnection('w'); + +$seen = array(); +foreach (new LiskMigrationIterator($table) as $path) { + $package_id = $path->getPackageID(); + $repository_phid = $path->getRepositoryPHID(); + $path_index = $path->getPathIndex(); + + if (!isset($seen[$package_id][$repository_phid][$path_index])) { + $seen[$package_id][$repository_phid][$path_index] = true; + continue; + } + + queryfx( + $conn, + 'DELETE FROM %T WHERE id = %d', + $table->getTableName(), + $path->getID()); +} diff --git a/resources/sql/autopatches/20180306.opath.04.unique.sql b/resources/sql/autopatches/20180306.opath.04.unique.sql new file mode 100644 index 0000000000..2349533b1f --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.04.unique.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_path + ADD UNIQUE KEY `key_path` (packageID, repositoryPHID, pathIndex); diff --git a/resources/sql/autopatches/20180306.opath.05.longpath.sql b/resources/sql/autopatches/20180306.opath.05.longpath.sql new file mode 100644 index 0000000000..79ff2f7a7f --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.05.longpath.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_path + CHANGE path path LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180306.opath.06.pathdisplay.sql b/resources/sql/autopatches/20180306.opath.06.pathdisplay.sql new file mode 100644 index 0000000000..b9b336ecd7 --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.06.pathdisplay.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_path + ADD pathDisplay LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180306.opath.07.copypaths.sql b/resources/sql/autopatches/20180306.opath.07.copypaths.sql new file mode 100644 index 0000000000..74ebecfa9a --- /dev/null +++ b/resources/sql/autopatches/20180306.opath.07.copypaths.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_owners.owners_path + SET pathDisplay = path WHERE pathDisplay = ''; diff --git a/resources/sql/autopatches/20180309.owners.01.primaryowner.sql b/resources/sql/autopatches/20180309.owners.01.primaryowner.sql new file mode 100644 index 0000000000..a5eb4368f2 --- /dev/null +++ b/resources/sql/autopatches/20180309.owners.01.primaryowner.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_package + DROP primaryOwnerPHID; diff --git a/scripts/setup/manage_lock.php b/scripts/setup/manage_lock.php new file mode 100755 index 0000000000..ec5405ec01 --- /dev/null +++ b/scripts/setup/manage_lock.php @@ -0,0 +1,21 @@ +#!/usr/bin/env php +setTagline(pht('manage locks')); +$args->setSynopsis(<<parseStandardArguments(); + +$workflows = id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorLockManagementWorkflow') + ->execute(); +$workflows[] = new PhutilHelpArgumentWorkflow(); +$args->parseWorkflows($workflows); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bb08685634..8677f47b6e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -598,6 +598,7 @@ phutil_register_library_map(array( 'DifferentialRevisionTitleTransaction' => 'applications/differential/xaction/DifferentialRevisionTitleTransaction.php', 'DifferentialRevisionTransactionType' => 'applications/differential/xaction/DifferentialRevisionTransactionType.php', 'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/DifferentialRevisionUpdateHistoryView.php', + 'DifferentialRevisionUpdateTransaction' => 'applications/differential/xaction/DifferentialRevisionUpdateTransaction.php', 'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php', 'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php', 'DifferentialRevisionWrongStateTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php', @@ -1882,6 +1883,7 @@ phutil_register_library_map(array( 'PHUIPropertyGroupView' => 'view/phui/PHUIPropertyGroupView.php', 'PHUIPropertyListExample' => 'applications/uiexample/examples/PHUIPropertyListExample.php', 'PHUIPropertyListView' => 'view/phui/PHUIPropertyListView.php', + 'PHUIRemarkupImageView' => 'infrastructure/markup/view/PHUIRemarkupImageView.php', 'PHUIRemarkupPreviewPanel' => 'view/phui/PHUIRemarkupPreviewPanel.php', 'PHUIRemarkupView' => 'infrastructure/markup/view/PHUIRemarkupView.php', 'PHUISegmentBarSegmentView' => 'view/phui/PHUISegmentBarSegmentView.php', @@ -2670,6 +2672,8 @@ phutil_register_library_map(array( 'PhabricatorDaemonController' => 'applications/daemon/controller/PhabricatorDaemonController.php', 'PhabricatorDaemonDAO' => 'applications/daemon/storage/PhabricatorDaemonDAO.php', 'PhabricatorDaemonEventListener' => 'applications/daemon/event/PhabricatorDaemonEventListener.php', + 'PhabricatorDaemonLockLog' => 'applications/daemon/storage/PhabricatorDaemonLockLog.php', + 'PhabricatorDaemonLockLogGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php', 'PhabricatorDaemonLog' => 'applications/daemon/storage/PhabricatorDaemonLog.php', 'PhabricatorDaemonLogEvent' => 'applications/daemon/storage/PhabricatorDaemonLogEvent.php', 'PhabricatorDaemonLogEventGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLogEventGarbageCollector.php', @@ -3204,6 +3208,8 @@ phutil_register_library_map(array( 'PhabricatorLocalTimeTestCase' => 'view/__tests__/PhabricatorLocalTimeTestCase.php', 'PhabricatorLocaleScopeGuard' => 'infrastructure/internationalization/scope/PhabricatorLocaleScopeGuard.php', 'PhabricatorLocaleScopeGuardTestCase' => 'infrastructure/internationalization/scope/__tests__/PhabricatorLocaleScopeGuardTestCase.php', + 'PhabricatorLockLogManagementWorkflow' => 'applications/daemon/management/PhabricatorLockLogManagementWorkflow.php', + 'PhabricatorLockManagementWorkflow' => 'applications/daemon/management/PhabricatorLockManagementWorkflow.php', 'PhabricatorLogTriggerAction' => 'infrastructure/daemon/workers/action/PhabricatorLogTriggerAction.php', 'PhabricatorLogoutController' => 'applications/auth/controller/PhabricatorLogoutController.php', 'PhabricatorLunarPhasePolicyRule' => 'applications/policy/rule/PhabricatorLunarPhasePolicyRule.php', @@ -3289,6 +3295,7 @@ phutil_register_library_map(array( 'PhabricatorMarkupInterface' => 'infrastructure/markup/PhabricatorMarkupInterface.php', 'PhabricatorMarkupOneOff' => 'infrastructure/markup/PhabricatorMarkupOneOff.php', 'PhabricatorMarkupPreviewController' => 'infrastructure/markup/PhabricatorMarkupPreviewController.php', + 'PhabricatorMemeEngine' => 'applications/macro/engine/PhabricatorMemeEngine.php', 'PhabricatorMemeRemarkupRule' => 'applications/macro/markup/PhabricatorMemeRemarkupRule.php', 'PhabricatorMentionRemarkupRule' => 'applications/people/markup/PhabricatorMentionRemarkupRule.php', 'PhabricatorMentionableInterface' => 'applications/transactions/interface/PhabricatorMentionableInterface.php', @@ -5818,6 +5825,7 @@ phutil_register_library_map(array( 'DifferentialRevisionTitleTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionTransactionType' => 'PhabricatorModularTransactionType', 'DifferentialRevisionUpdateHistoryView' => 'AphrontView', + 'DifferentialRevisionUpdateTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionViewController' => 'DifferentialController', 'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionWrongStateTransaction' => 'DifferentialRevisionTransactionType', @@ -7281,6 +7289,7 @@ phutil_register_library_map(array( 'PHUIPropertyGroupView' => 'AphrontTagView', 'PHUIPropertyListExample' => 'PhabricatorUIExample', 'PHUIPropertyListView' => 'AphrontView', + 'PHUIRemarkupImageView' => 'AphrontView', 'PHUIRemarkupPreviewPanel' => 'AphrontTagView', 'PHUIRemarkupView' => 'AphrontView', 'PHUISegmentBarSegmentView' => 'AphrontTagView', @@ -8194,6 +8203,8 @@ phutil_register_library_map(array( 'PhabricatorDaemonController' => 'PhabricatorController', 'PhabricatorDaemonDAO' => 'PhabricatorLiskDAO', 'PhabricatorDaemonEventListener' => 'PhabricatorEventListener', + 'PhabricatorDaemonLockLog' => 'PhabricatorDaemonDAO', + 'PhabricatorDaemonLockLogGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorDaemonLog' => array( 'PhabricatorDaemonDAO', 'PhabricatorPolicyInterface', @@ -8794,6 +8805,8 @@ phutil_register_library_map(array( 'PhabricatorLocalTimeTestCase' => 'PhabricatorTestCase', 'PhabricatorLocaleScopeGuard' => 'Phobject', 'PhabricatorLocaleScopeGuardTestCase' => 'PhabricatorTestCase', + 'PhabricatorLockLogManagementWorkflow' => 'PhabricatorLockManagementWorkflow', + 'PhabricatorLockManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorLogTriggerAction' => 'PhabricatorTriggerAction', 'PhabricatorLogoutController' => 'PhabricatorAuthController', 'PhabricatorLunarPhasePolicyRule' => 'PhabricatorPolicyRule', @@ -8881,6 +8894,7 @@ phutil_register_library_map(array( 'PhabricatorMarkupInterface', ), 'PhabricatorMarkupPreviewController' => 'PhabricatorController', + 'PhabricatorMemeEngine' => 'Phobject', 'PhabricatorMemeRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorMentionRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorMercurialGraphStream' => 'PhabricatorRepositoryGraphStream', diff --git a/src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php b/src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php new file mode 100644 index 0000000000..1fc62b9862 --- /dev/null +++ b/src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php @@ -0,0 +1,29 @@ +establishConnection('w'); + + queryfx( + $conn, + 'DELETE FROM %T WHERE dateCreated < %d LIMIT 100', + $table->getTableName(), + $this->getGarbageEpoch()); + + return ($conn->getAffectedRows() == 100); + } + +} diff --git a/src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php b/src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php new file mode 100644 index 0000000000..5602bda309 --- /dev/null +++ b/src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php @@ -0,0 +1,222 @@ +setName('log') + ->setSynopsis(pht('Enable, disable, or show the lock log.')) + ->setArguments( + array( + array( + 'name' => 'enable', + 'help' => pht('Enable the lock log.'), + ), + array( + 'name' => 'disable', + 'help' => pht('Disable the lock log.'), + ), + array( + 'name' => 'name', + 'param' => 'name', + 'help' => pht('Review logs for a specific lock.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $is_enable = $args->getArg('enable'); + $is_disable = $args->getArg('disable'); + + if ($is_enable && $is_disable) { + throw new PhutilArgumentUsageException( + pht( + 'You can not both "--enable" and "--disable" the lock log.')); + } + + $with_name = $args->getArg('name'); + + if ($is_enable || $is_disable) { + if (strlen($with_name)) { + throw new PhutilArgumentUsageException( + pht( + 'You can not both "--enable" or "--disable" with search '. + 'parameters like "--name".')); + } + + $gc = new PhabricatorDaemonLockLogGarbageCollector(); + $is_enabled = (bool)$gc->getRetentionPolicy(); + + $config_key = 'phd.garbage-collection'; + $const = $gc->getCollectorConstant(); + $value = PhabricatorEnv::getEnvConfig($config_key); + + if ($is_disable) { + if (!$is_enabled) { + echo tsprintf( + "%s\n", + pht('Lock log is already disabled.')); + return 0; + } + echo tsprintf( + "%s\n", + pht('Disabling the lock log.')); + + unset($value[$const]); + } else { + if ($is_enabled) { + echo tsprintf( + "%s\n", + pht('Lock log is already enabled.')); + return 0; + } + echo tsprintf( + "%s\n", + pht('Enabling the lock log.')); + + $value[$const] = phutil_units('24 hours in seconds'); + } + + id(new PhabricatorConfigLocalSource()) + ->setKeys( + array( + $config_key => $value, + )); + + echo tsprintf( + "%s\n", + pht('Done.')); + + echo tsprintf( + "%s\n", + pht('Restart daemons to apply changes.')); + + return 0; + } + + $table = new PhabricatorDaemonLockLog(); + $conn = $table->establishConnection('r'); + + $parts = array(); + if (strlen($with_name)) { + $parts[] = qsprintf( + $conn, + 'lockName = %s', + $with_name); + } + + if (!$parts) { + $constraint = '1 = 1'; + } else { + $constraint = '('.implode(') AND (', $parts).')'; + } + + $logs = $table->loadAllWhere( + '%Q ORDER BY id DESC LIMIT 100', + $constraint); + $logs = array_reverse($logs); + + if (!$logs) { + echo tsprintf( + "%s\n", + pht('No matching lock logs.')); + return 0; + } + + $table = id(new PhutilConsoleTable()) + ->setBorders(true) + ->addColumn( + 'id', + array( + 'title' => pht('Lock'), + )) + ->addColumn( + 'name', + array( + 'title' => pht('Name'), + )) + ->addColumn( + 'acquired', + array( + 'title' => pht('Acquired'), + )) + ->addColumn( + 'released', + array( + 'title' => pht('Released'), + )) + ->addColumn( + 'held', + array( + 'title' => pht('Held'), + )) + ->addColumn( + 'parameters', + array( + 'title' => pht('Parameters'), + )) + ->addColumn( + 'context', + array( + 'title' => pht('Context'), + )); + + $viewer = $this->getViewer(); + + foreach ($logs as $log) { + $created = $log->getDateCreated(); + $released = $log->getLockReleased(); + + if ($released) { + $held = '+'.($released - $created); + } else { + $held = null; + } + + $created = phabricator_datetime($created, $viewer); + $released = phabricator_datetime($released, $viewer); + + $parameters = $log->getLockParameters(); + $context = $log->getLockContext(); + + $table->addRow( + array( + 'id' => $log->getID(), + 'name' => $log->getLockName(), + 'acquired' => $created, + 'released' => $released, + 'held' => $held, + 'parameters' => $this->flattenParameters($parameters), + 'context' => $this->flattenParameters($context), + )); + } + + $table->draw(); + + return 0; + } + + private function flattenParameters(array $params, $keys = true) { + $flat = array(); + foreach ($params as $key => $value) { + if (is_array($value)) { + $value = $this->flattenParameters($value, false); + } + if ($keys) { + $flat[] = "{$key}={$value}"; + } else { + $flat[] = "{$value}"; + } + } + + if ($keys) { + $flat = implode(', ', $flat); + } else { + $flat = implode(' ', $flat); + } + + return $flat; + } + +} diff --git a/src/applications/daemon/management/PhabricatorLockManagementWorkflow.php b/src/applications/daemon/management/PhabricatorLockManagementWorkflow.php new file mode 100644 index 0000000000..e791f51090 --- /dev/null +++ b/src/applications/daemon/management/PhabricatorLockManagementWorkflow.php @@ -0,0 +1,4 @@ + array( + 'lockParameters' => self::SERIALIZATION_JSON, + 'lockContext' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array( + 'lockName' => 'text64', + 'lockReleased' => 'epoch?', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_lock' => array( + 'columns' => array('lockName'), + ), + 'key_created' => array( + 'columns' => array('dateCreated'), + ), + ), + ) + parent::getConfiguration(); + } + +} diff --git a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php index 34843b5609..13ed71c96e 100644 --- a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php @@ -58,7 +58,7 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod { $xactions = array(); $xactions[] = array( - 'type' => DifferentialRevisionEditEngine::KEY_UPDATE, + 'type' => DifferentialRevisionUpdateTransaction::EDITKEY, 'value' => $diff->getPHID(), ); diff --git a/src/applications/differential/constants/DifferentialRevisionStatus.php b/src/applications/differential/constants/DifferentialRevisionStatus.php index 70f0e33b28..e3acace3f0 100644 --- a/src/applications/differential/constants/DifferentialRevisionStatus.php +++ b/src/applications/differential/constants/DifferentialRevisionStatus.php @@ -172,10 +172,10 @@ final class DifferentialRevisionStatus extends Phobject { 'name' => pht('Draft'), // For legacy clients, treat this as though it is "Needs Review". 'legacy' => 0, - 'icon' => 'fa-file-text-o', + 'icon' => 'fa-spinner', 'closed' => false, - 'color.icon' => 'grey', - 'color.tag' => 'grey', + 'color.icon' => 'pink', + 'color.tag' => 'pink', 'color.ansi' => null, ), ); diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index c2bd4bf8de..9d02202f9f 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -390,10 +390,20 @@ final class DifferentialChangesetViewController extends DifferentialController { return array(); } + $change_type = $changeset->getChangeType(); + if (DifferentialChangeType::isDeleteChangeType($change_type)) { + // If this is a lint message on a deleted file, show it on the left + // side of the UI because there are no source code lines on the right + // side of the UI so inlines don't have anywhere to render. See PHI416. + $is_new = 0; + } else { + $is_new = 1; + } + $template = id(new DifferentialInlineComment()) - ->setChangesetID($changeset->getID()) - ->setIsNewFile(1) - ->setLineLength(0); + ->setChangesetID($changeset->getID()) + ->setIsNewFile($is_new) + ->setLineLength(0); $inlines = array(); foreach ($messages as $message) { diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php index 5d625e3ce9..8147708bca 100644 --- a/src/applications/differential/customfield/DifferentialDraftField.php +++ b/src/applications/differential/customfield/DifferentialDraftField.php @@ -37,9 +37,14 @@ final class DifferentialDraftField } // If the author has held this revision as a draft explicitly, don't - // show any misleading messages about it autosubmitting later. + // show any misleading messages about it autosubmitting later. We do show + // reminder text. if ($revision->getHoldAsDraft()) { - return array(); + return array( + pht( + 'This is a draft revision that has not yet been submitted for '. + 'review.'), + ); } $warnings = array(); @@ -93,4 +98,19 @@ final class DifferentialDraftField return $warnings; } + public function getWarningsForDetailView() { + $revision = $this->getObject(); + + if (!$revision->isDraft()) { + return array(); + } + + return array( + pht( + 'This revision is currently a draft. You can leave comments, but '. + 'no one will be notified until the revision is submitted for '. + 'review.'), + ); + } + } diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index 2bd3a5cdc3..0404bd6201 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -7,8 +7,6 @@ final class DifferentialRevisionEditEngine const ENGINECONST = 'differential.revision'; - const KEY_UPDATE = 'update'; - const ACTIONGROUP_REVIEW = 'review'; const ACTIONGROUP_REVISION = 'revision'; @@ -73,6 +71,14 @@ final class DifferentialRevisionEditEngine return pht('Revision'); } + protected function getCommentViewButtonText($object) { + if ($object->isDraft()) { + return pht('Submit Quietly'); + } + + return parent::getCommentViewButtonText($object); + } + protected function getObjectViewURI($object) { return $object->getURI(); } @@ -123,12 +129,13 @@ final class DifferentialRevisionEditEngine $fields = array(); $fields[] = id(new PhabricatorHandlesEditField()) - ->setKey(self::KEY_UPDATE) + ->setKey(DifferentialRevisionUpdateTransaction::EDITKEY) ->setLabel(pht('Update Diff')) ->setDescription(pht('New diff to create or update the revision with.')) ->setConduitDescription(pht('Create or update a revision with a diff.')) ->setConduitTypeDescription(pht('PHID of the diff.')) - ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setTransactionType( + DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE) ->setHandleParameterType(new AphrontPHIDListHTTPParameterType()) ->setSingleValue($diff_phid) ->setIsConduitOnly(!$diff) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index a092bf2693..d04b0d684a 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -33,7 +33,7 @@ final class DifferentialTransactionEditor } public function getDiffUpdateTransaction(array $xactions) { - $type_update = DifferentialTransaction::TYPE_UPDATE; + $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE; foreach ($xactions as $xaction) { if ($xaction->getTransactionType() == $type_update) { @@ -76,7 +76,6 @@ final class DifferentialTransactionEditor $types[] = PhabricatorTransactions::TYPE_INLINESTATE; $types[] = DifferentialTransaction::TYPE_INLINE; - $types[] = DifferentialTransaction::TYPE_UPDATE; return $types; } @@ -88,12 +87,6 @@ final class DifferentialTransactionEditor switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_INLINE: return null; - case DifferentialTransaction::TYPE_UPDATE: - if ($this->getIsNewObject()) { - return null; - } else { - return $object->getActiveDiff()->getPHID(); - } } return parent::getCustomTransactionOldValue($object, $xaction); @@ -104,8 +97,6 @@ final class DifferentialTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: - return $xaction->getNewValue(); case DifferentialTransaction::TYPE_INLINE: return null; } @@ -120,29 +111,6 @@ final class DifferentialTransactionEditor switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_INLINE: return; - case DifferentialTransaction::TYPE_UPDATE: - if (!$this->getIsCloseByCommit()) { - if ($object->isNeedsRevision() || - $object->isChangePlanned() || - $object->isAbandoned()) { - $object->setModernRevisionStatus( - DifferentialRevisionStatus::NEEDS_REVIEW); - } - } - - $diff = $this->requireDiff($xaction->getNewValue()); - - $this->updateRevisionLineCounts($object, $diff); - - if ($this->repositoryPHIDOverride !== false) { - $object->setRepositoryPHID($this->repositoryPHIDOverride); - } else { - $object->setRepositoryPHID($diff->getRepositoryPHID()); - } - - $object->attachActiveDiff($diff); - $object->setActiveDiffPHID($diff->getPHID()); - return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -196,7 +164,7 @@ final class DifferentialTransactionEditor // commit. } else { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: $downgrade_rejects = true; if (!$is_sticky_accept) { // If "sticky accept" is disabled, also downgrade the accepts. @@ -243,7 +211,7 @@ final class DifferentialTransactionEditor $is_commandeer = false; switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: if ($this->getIsCloseByCommit()) { // Don't bother with any of this if this update is a side effect of // commit detection. @@ -293,7 +261,7 @@ final class DifferentialTransactionEditor if (!$this->didExpandInlineState) { switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_COMMENT: - case DifferentialTransaction::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: case DifferentialTransaction::TYPE_INLINE: $this->didExpandInlineState = true; @@ -343,45 +311,6 @@ final class DifferentialTransactionEditor if ($reply && !$reply->getHasReplies()) { $reply->setHasReplies(1)->save(); } - return; - case DifferentialTransaction::TYPE_UPDATE: - // Now that we're inside the transaction, do a final check. - $diff = $this->requireDiff($xaction->getNewValue()); - - // TODO: It would be slightly cleaner to just revalidate this - // transaction somehow using the same validation code, but that's - // not easy to do at the moment. - - $revision_id = $diff->getRevisionID(); - if ($revision_id && ($revision_id != $object->getID())) { - throw new Exception( - pht( - 'Diff is already attached to another revision. You lost '. - 'a race?')); - } - - // TODO: This can race with diff updates, particularly those from - // Harbormaster. See discussion in T8650. - $diff->setRevisionID($object->getID()); - $diff->save(); - - // If there are any outstanding buildables for this diff, tell - // Harbormaster that their containers need to be updated. This is - // common, because `arc` creates buildables so it can upload lint - // and unit results. - - $buildables = id(new HarbormasterBuildableQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withManualBuildables(false) - ->withBuildablePHIDs(array($diff->getPHID())) - ->execute(); - foreach ($buildables as $buildable) { - $buildable->sendMessage( - $this->getActor(), - HarbormasterMessageType::BUILDABLE_CONTAINER, - true); - } - return; } @@ -437,7 +366,7 @@ final class DifferentialTransactionEditor foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: $diff = $this->requireDiff($xaction->getNewValue(), true); // Update these denormalized index tables when we attach a new @@ -554,44 +483,6 @@ final class DifferentialTransactionEditor return $xactions; } - - protected function validateTransaction( - PhabricatorLiskDAO $object, - $type, - array $xactions) { - - $errors = parent::validateTransaction($object, $type, $xactions); - - $config_self_accept_key = 'differential.allow-self-accept'; - $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key); - - foreach ($xactions as $xaction) { - switch ($type) { - case DifferentialTransaction::TYPE_UPDATE: - $diff = $this->loadDiff($xaction->getNewValue()); - if (!$diff) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - pht('The specified diff does not exist.'), - $xaction); - } else if (($diff->getRevisionID()) && - ($diff->getRevisionID() != $object->getID())) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - pht( - 'You can not update this revision to the specified diff, '. - 'because the diff is already attached to another revision.'), - $xaction); - } - break; - } - } - - return $errors; - } - protected function sortTransactions(array $xactions) { $xactions = parent::sortTransactions($xactions); @@ -674,7 +565,7 @@ final class DifferentialTransactionEditor $action = parent::getMailAction($object, $xactions); $strongest = $this->getStrongestAction($object, $xactions); - $type_update = DifferentialTransaction::TYPE_UPDATE; + $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE; if ($strongest->getTransactionType() == $type_update) { $show_lines = true; } @@ -772,7 +663,7 @@ final class DifferentialTransactionEditor $update_xaction = null; foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: $update_xaction = $xaction; break; } @@ -1053,7 +944,7 @@ final class DifferentialTransactionEditor return $query->executeOne(); } - private function requireDiff($phid, $need_changesets = false) { + public function requireDiff($phid, $need_changesets = false) { $diff = $this->loadDiff($phid, $need_changesets); if (!$diff) { throw new Exception(pht('Diff "%s" does not exist!', $phid)); @@ -1091,25 +982,43 @@ final class DifferentialTransactionEditor return array(); } - // Remove packages that the revision author is an owner of. If you own - // code, you don't need another owner to review it. - $authority = id(new PhabricatorOwnersPackageQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs(mpull($packages, 'getPHID')) - ->withAuthorityPHIDs(array($object->getAuthorPHID())) - ->execute(); - $authority = mpull($authority, null, 'getPHID'); + // Identify the packages with "Non-Owner Author" review rules and remove + // them if the author has authority over the package. - foreach ($packages as $key => $package) { - $package_phid = $package->getPHID(); - if (isset($authority[$package_phid])) { - unset($packages[$key]); + $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); + $need_authority = array(); + foreach ($packages as $package) { + $autoreview_setting = $package->getAutoReview(); + + $spec = idx($autoreview_map, $autoreview_setting); + if (!$spec) { continue; } + + if (idx($spec, 'authority')) { + $need_authority[$package->getPHID()] = $package->getPHID(); + } } - if (!$packages) { - return array(); + if ($need_authority) { + $authority = id(new PhabricatorOwnersPackageQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($need_authority) + ->withAuthorityPHIDs(array($object->getAuthorPHID())) + ->execute(); + $authority = mpull($authority, null, 'getPHID'); + + foreach ($packages as $key => $package) { + $package_phid = $package->getPHID(); + if (isset($authority[$package_phid])) { + unset($packages[$key]); + continue; + } + } + + if (!$packages) { + return array(); + } } $auto_subscribe = array(); @@ -1118,15 +1027,18 @@ final class DifferentialTransactionEditor foreach ($packages as $package) { switch ($package->getAutoReview()) { - case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE: - $auto_subscribe[] = $package; - break; case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW: + case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW_ALWAYS: $auto_review[] = $package; break; case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK: + case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK_ALWAYS: $auto_block[] = $package; break; + case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE: + case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE_ALWAYS: + $auto_subscribe[] = $package; + break; case PhabricatorOwnersPackage::AUTOREVIEW_NONE: default: break; @@ -1274,7 +1186,7 @@ final class DifferentialTransactionEditor $has_update = false; $has_commit = false; - $type_update = DifferentialTransaction::TYPE_UPDATE; + $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE; foreach ($xactions as $xaction) { if ($xaction->getTransactionType() != $type_update) { continue; @@ -1721,27 +1633,6 @@ final class DifferentialTransactionEditor return true; } - private function updateRevisionLineCounts( - DifferentialRevision $revision, - DifferentialDiff $diff) { - - $revision->setLineCount($diff->getLineCount()); - - $conn = $revision->establishConnection('r'); - - $row = queryfx_one( - $conn, - 'SELECT SUM(addLines) A, SUM(delLines) D FROM %T - WHERE diffID = %d', - id(new DifferentialChangeset())->getTableName(), - $diff->getID()); - - if ($row) { - $revision->setAddedLineCount((int)$row['A']); - $revision->setRemovedLineCount((int)$row['D']); - } - } - private function requireReviewers(DifferentialRevision $revision) { if ($revision->hasAttachedReviewers()) { return; diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index 6fb5e70de5..d7d7c767e1 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -278,11 +278,14 @@ final class DifferentialDiffExtractionEngine extends Phobject { ->setNewValue($revision->getModernRevisionStatus()); } + $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE; + $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setTransactionType($type_update) ->setIgnoreOnNoEffect(true) ->setNewValue($new_diff->getPHID()) - ->setMetadataValue('isCommitUpdate', true); + ->setMetadataValue('isCommitUpdate', true) + ->setMetadataValue('commitPHIDs', array($commit->getPHID())); foreach ($more_xactions as $more_xaction) { $xactions[] = $more_xaction; diff --git a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php index c37974fa2e..2443fe9651 100644 --- a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php +++ b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php @@ -22,14 +22,14 @@ final class PhabricatorDifferentialRevisionTestDataGenerator $revision->setTestPlan($this->generateDescription()); $diff = $this->generateDiff($author); + $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE; $xactions = array(); $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setTransactionType($type_update) ->setNewValue($diff->getPHID()); - id(new DifferentialTransactionEditor()) ->setActor($author) ->setContentSource($this->getLipsumContentSource()) diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index f7dcf29860..53fdc71a15 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -6,7 +6,6 @@ final class DifferentialTransaction private $isCommandeerSideEffect; const TYPE_INLINE = 'differential:inline'; - const TYPE_UPDATE = 'differential:update'; const TYPE_ACTION = 'differential:action'; const MAILTAG_REVIEWERS = 'differential-reviewers'; @@ -75,18 +74,6 @@ final class DifferentialTransaction $new = $this->getNewValue(); switch ($this->getTransactionType()) { - case self::TYPE_UPDATE: - // Older versions of this transaction have an ID for the new value, - // and/or do not record the old value. Only hide the transaction if - // the new value is a PHID, indicating that this is a newer style - // transaction. - if ($old === null) { - if (phid_get_type($new) == DifferentialDiffPHIDType::TYPECONST) { - return true; - } - } - break; - case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE: // Don't hide the initial "X requested review: ..." transaction from // mail or feed even when it occurs during creation. We need this @@ -139,11 +126,6 @@ final class DifferentialTransaction } } break; - case self::TYPE_UPDATE: - if ($new) { - $phids[] = $new; - } - break; } return $phids; @@ -153,8 +135,6 @@ final class DifferentialTransaction switch ($this->getTransactionType()) { case self::TYPE_ACTION: return 3; - case self::TYPE_UPDATE: - return 2; } return parent::getActionStrength(); @@ -165,13 +145,6 @@ final class DifferentialTransaction switch ($this->getTransactionType()) { case self::TYPE_INLINE: return pht('Commented On'); - case self::TYPE_UPDATE: - $old = $this->getOldValue(); - if ($old === null) { - return pht('Request'); - } else { - return pht('Updated'); - } case self::TYPE_ACTION: $map = array( DifferentialAction::ACTION_ACCEPT => pht('Accepted'), @@ -209,7 +182,7 @@ final class DifferentialTransaction break; } break; - case self::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: $old = $this->getOldValue(); if ($old === null) { $tags[] = self::MAILTAG_REVIEW_REQUEST; @@ -248,28 +221,6 @@ final class DifferentialTransaction return pht( '%s added inline comments.', $author_handle); - case self::TYPE_UPDATE: - if ($this->getMetadataValue('isCommitUpdate')) { - return pht( - 'This revision was automatically updated to reflect the '. - 'committed changes.'); - } else if ($new) { - // TODO: Migrate to PHIDs and use handles here? - if (phid_get_type($new) == DifferentialDiffPHIDType::TYPECONST) { - return pht( - '%s updated this revision to %s.', - $author_handle, - $this->renderHandleLink($new)); - } else { - return pht( - '%s updated this revision.', - $author_handle); - } - } else { - return pht( - '%s updated this revision.', - $author_handle); - } case self::TYPE_ACTION: switch ($new) { case DifferentialAction::ACTION_CLOSE: @@ -347,11 +298,6 @@ final class DifferentialTransaction '%s added inline comments to %s.', $author_link, $object_link); - case self::TYPE_UPDATE: - return pht( - '%s updated the diff for %s.', - $author_link, - $object_link); case self::TYPE_ACTION: switch ($new) { case DifferentialAction::ACTION_ACCEPT: @@ -462,8 +408,6 @@ final class DifferentialTransaction switch ($this->getTransactionType()) { case self::TYPE_INLINE: return 'fa-comment'; - case self::TYPE_UPDATE: - return 'fa-refresh'; case self::TYPE_ACTION: switch ($this->getNewValue()) { case DifferentialAction::ACTION_CLOSE: @@ -526,8 +470,6 @@ final class DifferentialTransaction public function getColor() { switch ($this->getTransactionType()) { - case self::TYPE_UPDATE: - return PhabricatorTransactions::COLOR_SKY; case self::TYPE_ACTION: switch ($this->getNewValue()) { case DifferentialAction::ACTION_CLOSE: diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php index b1c796dca4..7ef0f2ba60 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php @@ -136,4 +136,23 @@ final class DifferentialRevisionCloseTransaction $this->renderObject()); } + public function getTransactionTypeForConduit($xaction) { + return 'close'; + } + + public function getFieldValuesForConduit($object, $data) { + $commit_phid = $object->getMetadataValue('commitPHID'); + + if ($commit_phid) { + $commit_phids = array($commit_phid); + } else { + $commit_phids = array(); + } + + return array( + 'commitPHIDs' => $commit_phids, + ); + } + + } diff --git a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php new file mode 100644 index 0000000000..c89c3c79e1 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php @@ -0,0 +1,199 @@ +getActiveDiffPHID(); + } + + public function applyInternalEffects($object, $value) { + $should_review = $this->shouldRequestReviewAfterUpdate($object); + if ($should_review) { + $object->setModernRevisionStatus( + DifferentialRevisionStatus::NEEDS_REVIEW); + } + + $editor = $this->getEditor(); + $diff = $editor->requireDiff($value); + + $this->updateRevisionLineCounts($object, $diff); + + $object->setRepositoryPHID($diff->getRepositoryPHID()); + $object->setActiveDiffPHID($diff->getPHID()); + $object->attachActiveDiff($diff); + } + + private function shouldRequestReviewAfterUpdate($object) { + if ($this->isCommitUpdate()) { + return false; + } + + $should_update = + $object->isNeedsRevision() || + $object->isChangePlanned() || + $object->isAbandoned(); + if ($should_update) { + return true; + } + + return false; + } + + public function applyExternalEffects($object, $value) { + $editor = $this->getEditor(); + $diff = $editor->requireDiff($value); + + // TODO: This can race with diff updates, particularly those from + // Harbormaster. See discussion in T8650. + $diff->setRevisionID($object->getID()); + $diff->save(); + + // If there are any outstanding buildables for this diff, tell + // Harbormaster that their containers need to be updated. This is + // common, because `arc` creates buildables so it can upload lint + // and unit results. + + $buildables = id(new HarbormasterBuildableQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withManualBuildables(false) + ->withBuildablePHIDs(array($diff->getPHID())) + ->execute(); + foreach ($buildables as $buildable) { + $buildable->sendMessage( + $this->getActor(), + HarbormasterMessageType::BUILDABLE_CONTAINER, + true); + } + } + + public function getColor() { + return 'sky'; + } + + public function getIcon() { + return 'fa-refresh'; + } + + public function getActionName() { + if ($this->isCreateTransaction()) { + return pht('Request'); + } else { + return pht('Updated'); + } + } + + public function getActionStrength() { + return 2; + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + if ($this->isCommitUpdate()) { + return pht( + 'This revision was automatically updated to reflect the '. + 'committed changes.'); + } + + // NOTE: Very, very old update transactions did not have a new value or + // did not use a diff PHID as a new value. This was changed years ago, + // but wasn't migrated. We might consider migrating if this causes issues. + + return pht( + '%s updated this revision to %s.', + $this->renderAuthor(), + $this->renderNewHandle()); + } + + public function getTitleForFeed() { + return pht( + '%s updated the diff for %s.', + $this->renderAuthor(), + $this->renderObject()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $diff_phid = null; + foreach ($xactions as $xaction) { + $diff_phid = $xaction->getNewValue(); + + $diff = id(new DifferentialDiffQuery()) + ->withPHIDs(array($diff_phid)) + ->setViewer($this->getActor()) + ->executeOne(); + if (!$diff) { + $errors[] = $this->newInvalidError( + pht( + 'Specified diff ("%s") does not exist.', + $diff_phid), + $xaction); + continue; + } + + if ($diff->getRevisionID()) { + $errors[] = $this->newInvalidError( + pht( + 'You can not update this revision with the specified diff ("%s") '. + 'because the diff is already attached to another revision.', + $diff_phid), + $xaction); + continue; + } + } + + if (!$diff_phid && !$object->getActiveDiffPHID()) { + $errors[] = $this->newInvalidError( + pht( + 'You must specify an initial diff when creating a revision.')); + } + + return $errors; + } + + public function isCommitUpdate() { + return (bool)$this->getMetadataValue('isCommitUpdate'); + } + + private function updateRevisionLineCounts( + DifferentialRevision $revision, + DifferentialDiff $diff) { + + $revision->setLineCount($diff->getLineCount()); + + $conn = $revision->establishConnection('r'); + + $row = queryfx_one( + $conn, + 'SELECT SUM(addLines) A, SUM(delLines) D FROM %T + WHERE diffID = %d', + id(new DifferentialChangeset())->getTableName(), + $diff->getID()); + + if ($row) { + $revision->setAddedLineCount((int)$row['A']); + $revision->setRemovedLineCount((int)$row['D']); + } + } + + public function getTransactionTypeForConduit($xaction) { + return 'update'; + } + + public function getFieldValuesForConduit($object, $data) { + $commit_phids = $object->getMetadataValue('commitPHIDs', array()); + + return array( + 'old' => $object->getOldValue(), + 'new' => $object->getNewValue(), + 'commitPHIDs' => $commit_phids, + ); + } + +} diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php index e19e54199c..47678e886f 100644 --- a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php @@ -35,7 +35,8 @@ final class DifferentialRevisionWrongStateTransaction $this->renderValue($status->getDisplayName())); } - public function getTitleForFeed() { - return null; + public function shouldHideForFeed() { + return true; } + } diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index 7ae67c2b5d..a220ac05e0 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -69,13 +69,18 @@ abstract class DiffusionController extends PhabricatorController { // repository has a different canonical path like "/diffusion/XYZ/...", // redirect them to the canonical path. - $request_path = $request->getPath(); - $repository = $drequest->getRepository(); + // Skip this redirect if the request is an AJAX request, like the requests + // that Owners makes to complete and validate paths. - $canonical_path = $repository->getCanonicalPath($request_path); - if ($canonical_path !== null) { - if ($canonical_path != $request_path) { - return id(new AphrontRedirectResponse())->setURI($canonical_path); + if (!$request->isAjax()) { + $request_path = $request->getPath(); + $repository = $drequest->getRepository(); + + $canonical_path = $repository->getCanonicalPath($request_path); + if ($canonical_path !== null) { + if ($canonical_path != $request_path) { + return id(new AphrontRedirectResponse())->setURI($canonical_path); + } } } diff --git a/src/applications/diffusion/controller/DiffusionPathValidateController.php b/src/applications/diffusion/controller/DiffusionPathValidateController.php index e6fbc41132..62e9e04796 100644 --- a/src/applications/diffusion/controller/DiffusionPathValidateController.php +++ b/src/applications/diffusion/controller/DiffusionPathValidateController.php @@ -45,19 +45,6 @@ final class DiffusionPathValidateController extends DiffusionController { 'valid' => (bool)$valid, ); - if (!$valid) { - $branch = $drequest->getBranch(); - if ($branch) { - $message = pht('Not found in %s', $branch); - } else { - $message = pht('Not found at %s', 'HEAD'); - } - } else { - $message = pht('OK'); - } - - $output['message'] = $message; - return id(new AphrontAjaxResponse())->setContent($output); } } diff --git a/src/applications/files/PhabricatorImageTransformer.php b/src/applications/files/PhabricatorImageTransformer.php index f29ce97835..1075969a42 100644 --- a/src/applications/files/PhabricatorImageTransformer.php +++ b/src/applications/files/PhabricatorImageTransformer.php @@ -6,268 +6,6 @@ */ final class PhabricatorImageTransformer extends Phobject { - public function executeMemeTransform( - PhabricatorFile $file, - $upper_text, - $lower_text) { - $image = $this->applyMemeToFile($file, $upper_text, $lower_text); - return PhabricatorFile::newFromFileData( - $image, - array( - 'name' => 'meme-'.$file->getName(), - 'ttl.relative' => phutil_units('24 hours in seconds'), - 'canCDN' => true, - )); - } - - public function executeConpherenceTransform( - PhabricatorFile $file, - $top, - $left, - $width, - $height) { - - $image = $this->crasslyCropTo( - $file, - $top, - $left, - $width, - $height); - - return PhabricatorFile::newFromFileData( - $image, - array( - 'name' => 'conpherence-'.$file->getName(), - 'profile' => true, - 'canCDN' => true, - )); - } - - private function crasslyCropTo(PhabricatorFile $file, $top, $left, $w, $h) { - $data = $file->loadFileData(); - $src = imagecreatefromstring($data); - $dst = $this->getBlankDestinationFile($w, $h); - - $scale = self::getScaleForCrop($file, $w, $h); - $orig_x = $left / $scale; - $orig_y = $top / $scale; - $orig_w = $w / $scale; - $orig_h = $h / $scale; - - imagecopyresampled( - $dst, - $src, - 0, 0, - $orig_x, $orig_y, - $w, $h, - $orig_w, $orig_h); - - return self::saveImageDataInAnyFormat($dst, $file->getMimeType()); - } - - private function getBlankDestinationFile($dx, $dy) { - $dst = imagecreatetruecolor($dx, $dy); - imagesavealpha($dst, true); - imagefill($dst, 0, 0, imagecolorallocatealpha($dst, 255, 255, 255, 127)); - - return $dst; - } - - public static function getScaleForCrop( - PhabricatorFile $file, - $des_width, - $des_height) { - - $metadata = $file->getMetadata(); - $width = $metadata[PhabricatorFile::METADATA_IMAGE_WIDTH]; - $height = $metadata[PhabricatorFile::METADATA_IMAGE_HEIGHT]; - - if ($height < $des_height) { - $scale = $height / $des_height; - } else if ($width < $des_width) { - $scale = $width / $des_width; - } else { - $scale_x = $des_width / $width; - $scale_y = $des_height / $height; - $scale = max($scale_x, $scale_y); - } - - return $scale; - } - - private function applyMemeToFile( - PhabricatorFile $file, - $upper_text, - $lower_text) { - $data = $file->loadFileData(); - - $img_type = $file->getMimeType(); - $imagemagick = PhabricatorEnv::getEnvConfig('files.enable-imagemagick'); - - if ($img_type != 'image/gif' || $imagemagick == false) { - return $this->applyMemeTo( - $data, $upper_text, $lower_text, $img_type); - } - - $data = $file->loadFileData(); - $input = new TempFile(); - Filesystem::writeFile($input, $data); - - list($out) = execx('convert %s info:', $input); - $split = phutil_split_lines($out); - if (count($split) > 1) { - return $this->applyMemeWithImagemagick( - $input, - $upper_text, - $lower_text, - count($split), - $img_type); - } else { - return $this->applyMemeTo($data, $upper_text, $lower_text, $img_type); - } - } - - private function applyMemeTo( - $data, - $upper_text, - $lower_text, - $mime_type) { - $img = imagecreatefromstring($data); - - // Some PNGs have color palettes, and allocating the dark border color - // fails and gives us whatever's first in the color table. Copy the image - // to a fresh truecolor canvas before working with it. - - $truecolor = imagecreatetruecolor(imagesx($img), imagesy($img)); - imagecopy($truecolor, $img, 0, 0, 0, 0, imagesx($img), imagesy($img)); - $img = $truecolor; - - $phabricator_root = dirname(phutil_get_library_root('phabricator')); - $font_root = $phabricator_root.'/resources/font/'; - $font_path = $font_root.'tuffy.ttf'; - if (Filesystem::pathExists($font_root.'impact.ttf')) { - $font_path = $font_root.'impact.ttf'; - } - $text_color = imagecolorallocate($img, 255, 255, 255); - $border_color = imagecolorallocatealpha($img, 0, 0, 0, 110); - $border_width = 4; - $font_max = 200; - $font_min = 5; - for ($i = $font_max; $i > $font_min; $i--) { - $fit = $this->doesTextBoundingBoxFitInImage( - $img, - $upper_text, - $i, - $font_path); - if ($fit['doesfit']) { - $x = ($fit['imgwidth'] - $fit['txtwidth']) / 2; - $y = $fit['txtheight'] + 10; - $this->makeImageWithTextBorder($img, - $i, - $x, - $y, - $text_color, - $border_color, - $border_width, - $font_path, - $upper_text); - break; - } - } - for ($i = $font_max; $i > $font_min; $i--) { - $fit = $this->doesTextBoundingBoxFitInImage($img, - $lower_text, $i, $font_path); - if ($fit['doesfit']) { - $x = ($fit['imgwidth'] - $fit['txtwidth']) / 2; - $y = $fit['imgheight'] - 10; - $this->makeImageWithTextBorder( - $img, - $i, - $x, - $y, - $text_color, - $border_color, - $border_width, - $font_path, - $lower_text); - break; - } - } - return self::saveImageDataInAnyFormat($img, $mime_type); - } - - private function makeImageWithTextBorder($img, $font_size, $x, $y, - $color, $stroke_color, $bw, $font, $text) { - $angle = 0; - $bw = abs($bw); - for ($c1 = $x - $bw; $c1 <= $x + $bw; $c1++) { - for ($c2 = $y - $bw; $c2 <= $y + $bw; $c2++) { - if (!(($c1 == $x - $bw || $x + $bw) && - $c2 == $y - $bw || $c2 == $y + $bw)) { - $bg = imagettftext($img, $font_size, - $angle, $c1, $c2, $stroke_color, $font, $text); - } - } - } - imagettftext($img, $font_size, $angle, - $x , $y, $color , $font, $text); - } - - private function doesTextBoundingBoxFitInImage($img, - $text, $font_size, $font_path) { - // Default Angle = 0 - $angle = 0; - - $bbox = imagettfbbox($font_size, $angle, $font_path, $text); - $text_height = abs($bbox[3] - $bbox[5]); - $text_width = abs($bbox[0] - $bbox[2]); - return array( - 'doesfit' => ($text_height * 1.05 <= imagesy($img) / 2 - && $text_width * 1.05 <= imagesx($img)), - 'txtwidth' => $text_width, - 'txtheight' => $text_height, - 'imgwidth' => imagesx($img), - 'imgheight' => imagesy($img), - ); - } - - private function applyMemeWithImagemagick( - $input, - $above, - $below, - $count, - $img_type) { - - $output = new TempFile(); - $future = new ExecFuture( - 'convert %s -coalesce +adjoin %s_%s', - $input, - $input, - '%09d'); - $future->setTimeout(10)->resolvex(); - - $output_files = array(); - for ($ii = 0; $ii < $count; $ii++) { - $frame_name = sprintf('%s_%09d', $input, $ii); - $output_name = sprintf('%s_%09d', $output, $ii); - - $output_files[] = $output_name; - - $frame_data = Filesystem::readFile($frame_name); - $memed_frame_data = $this->applyMemeTo( - $frame_data, - $above, - $below, - $img_type); - Filesystem::writeFile($output_name, $memed_frame_data); - } - - $future = new ExecFuture('convert -loop 0 %Ls %s', $output_files, $output); - $future->setTimeout(10)->resolvex(); - - return Filesystem::readFile($output); - } - /* -( Saving Image Data )-------------------------------------------------- */ diff --git a/src/applications/files/controller/PhabricatorFileImageProxyController.php b/src/applications/files/controller/PhabricatorFileImageProxyController.php index 44573aea46..05a49c7f3e 100644 --- a/src/applications/files/controller/PhabricatorFileImageProxyController.php +++ b/src/applications/files/controller/PhabricatorFileImageProxyController.php @@ -8,15 +8,6 @@ final class PhabricatorFileImageProxyController } public function handleRequest(AphrontRequest $request) { - - $show_prototypes = PhabricatorEnv::getEnvConfig( - 'phabricator.show-prototypes'); - if (!$show_prototypes) { - throw new Exception( - pht('Show prototypes is disabled. - Set `phabricator.show-prototypes` to `true` to use the image proxy')); - } - $viewer = $request->getViewer(); $img_uri = $request->getStr('uri'); @@ -24,9 +15,16 @@ final class PhabricatorFileImageProxyController PhabricatorEnv::requireValidRemoteURIForLink($img_uri); $uri = new PhutilURI($img_uri); $proto = $uri->getProtocol(); - if (!in_array($proto, array('http', 'https'))) { + + $allowed_protocols = array( + 'http', + 'https', + ); + if (!in_array($proto, $allowed_protocols)) { throw new Exception( - pht('The provided image URI must be either http or https')); + pht( + 'The provided image URI must use one of these protocols: %s.', + implode(', ', $allowed_protocols))); } // Check if we already have the specified image URI downloaded @@ -43,8 +41,9 @@ final class PhabricatorFileImageProxyController ->setURI($img_uri) ->setTTL($ttl); + // Cache missed, so we'll need to validate and download the image. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - // Cache missed so we'll need to validate and download the image + $save_request = false; try { // Rate limit outbound fetches to make this mechanism less useful for // scanning networks and ports. @@ -59,6 +58,7 @@ final class PhabricatorFileImageProxyController 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, 'canCDN' => true, )); + if (!$file->isViewableImage()) { $mime_type = $file->getMimeType(); $engine = new PhabricatorDestructionEngine(); @@ -66,53 +66,82 @@ final class PhabricatorFileImageProxyController $file = null; throw new Exception( pht( - 'The URI "%s" does not correspond to a valid image file, got '. - 'a file with MIME type "%s". You must specify the URI of a '. + 'The URI "%s" does not correspond to a valid image file (got '. + 'a file with MIME type "%s"). You must specify the URI of a '. 'valid image file.', $uri, $mime_type)); - } else { - $file->save(); } - $external_request->setIsSuccessful(true) - ->setFilePHID($file->getPHID()) - ->save(); - unset($unguarded); - return $this->getExternalResponse($external_request); + $file->save(); + + $external_request + ->setIsSuccessful(1) + ->setFilePHID($file->getPHID()); + + $save_request = true; } catch (HTTPFutureHTTPResponseStatus $status) { - $external_request->setIsSuccessful(false) - ->setResponseMessage($status->getMessage()) - ->save(); - return $this->getExternalResponse($external_request); + $external_request + ->setIsSuccessful(0) + ->setResponseMessage($status->getMessage()); + + $save_request = true; } catch (Exception $ex) { // Not actually saving the request in this case $external_request->setResponseMessage($ex->getMessage()); - return $this->getExternalResponse($external_request); } + + if ($save_request) { + try { + $external_request->save(); + } catch (AphrontDuplicateKeyQueryException $ex) { + // We may have raced against another identical request. If we did, + // just throw our result away and use the winner's result. + $external_request = $external_request->loadOneWhere( + 'uriIndex = %s', + PhabricatorHash::digestForIndex($img_uri)); + if (!$external_request) { + throw new Exception( + pht( + 'Hit duplicate key collision when saving proxied image, but '. + 'failed to load duplicate row (for URI "%s").', + $img_uri)); + } + } + } + + unset($unguarded); + + + return $this->getExternalResponse($external_request); } private function getExternalResponse( PhabricatorFileExternalRequest $request) { - if ($request->getIsSuccessful()) { - $file = id(new PhabricatorFileQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs(array($request->getFilePHID())) - ->executeOne(); - if (!$file) { - throw new Exception(pht( - 'The underlying file does not exist, but the cached request was '. - 'successful. This likely means the file record was manually deleted '. - 'by an administrator.')); - } - return id(new AphrontRedirectResponse()) - ->setIsExternal(true) - ->setURI($file->getViewURI()); - } else { - throw new Exception(pht( - "The request to get the external file from '%s' was unsuccessful:\n %s", - $request->getURI(), - $request->getResponseMessage())); + if (!$request->getIsSuccessful()) { + throw new Exception( + pht( + 'Request to "%s" failed: %s', + $request->getURI(), + $request->getResponseMessage())); } + + $file = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($request->getFilePHID())) + ->executeOne(); + if (!$file) { + throw new Exception( + pht( + 'The underlying file does not exist, but the cached request was '. + 'successful. This likely means the file record was manually '. + 'deleted by an administrator.')); + } + + return id(new AphrontAjaxResponse()) + ->setContent( + array( + 'imageURI' => $file->getViewURI(), + )); } } diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php index 36089dc57e..5d1979ed3c 100644 --- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php +++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php @@ -1,6 +1,9 @@ isFlatText($matches[0])) { return $matches[0]; } + $args = array(); $defaults = array( 'uri' => null, @@ -23,9 +27,10 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { 'width' => null, 'height' => null, ); + $trimmed_match = trim($matches[2]); if ($this->isURI($trimmed_match)) { - $args['uri'] = new PhutilURI($trimmed_match); + $args['uri'] = $trimmed_match; } else { $parser = new PhutilSimpleOptions(); $keys = $parser->parse($trimmed_match); @@ -37,28 +42,124 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { } } if ($uri_key) { - $args['uri'] = new PhutilURI($keys[$uri_key]); + $args['uri'] = $keys[$uri_key]; } $args += $keys; } $args += $defaults; - if ($args['uri']) { - $src_uri = id(new PhutilURI('/file/imageproxy/')) - ->setQueryParam('uri', (string)$args['uri']); - $img = $this->newTag( - 'img', - array( - 'src' => $src_uri, - 'alt' => $args['alt'], - 'width' => $args['width'], - 'height' => $args['height'], - )); - return $this->getEngine()->storeText($img); - } else { + if (!strlen($args['uri'])) { return $matches[0]; } + + // Make sure this is something that looks roughly like a real URI. We'll + // validate it more carefully before proxying it, but if whatever the user + // has typed isn't even close, just decline to activate the rule behavior. + try { + $uri = new PhutilURI($args['uri']); + + if (!strlen($uri->getProtocol())) { + return $matches[0]; + } + + $args['uri'] = (string)$uri; + } catch (Exception $ex) { + return $matches[0]; + } + + $engine = $this->getEngine(); + $metadata_key = self::KEY_RULE_EXTERNAL_IMAGE; + $metadata = $engine->getTextMetadata($metadata_key, array()); + + $token = $engine->storeText(''); + + $metadata[] = array( + 'token' => $token, + 'args' => $args, + ); + + $engine->setTextMetadata($metadata_key, $metadata); + + return $token; + } + + public function didMarkupText() { + $engine = $this->getEngine(); + $metadata_key = self::KEY_RULE_EXTERNAL_IMAGE; + $images = $engine->getTextMetadata($metadata_key, array()); + $engine->setTextMetadata($metadata_key, array()); + + if (!$images) { + return; + } + + // Look for images we've already successfully fetched that aren't about + // to get eaten by the GC. For any we find, we can just emit a normal + // "" tag pointing directly to the file. + + // For files which we don't hit in the cache, we emit a placeholder + // instead and use AJAX to actually perform the fetch. + + $digests = array(); + foreach ($images as $image) { + $uri = $image['args']['uri']; + $digests[] = PhabricatorHash::digestForIndex($uri); + } + + $caches = id(new PhabricatorFileExternalRequest())->loadAllWhere( + 'uriIndex IN (%Ls) AND isSuccessful = 1 AND ttl > %d', + $digests, + PhabricatorTime::getNow() + phutil_units('1 hour in seconds')); + + $file_phids = array(); + foreach ($caches as $cache) { + $file_phids[$cache->getFilePHID()] = $cache->getURI(); + } + + $file_map = array(); + if ($file_phids) { + $files = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array_keys($file_phids)) + ->execute(); + foreach ($files as $file) { + $phid = $file->getPHID(); + + $file_remote_uri = $file_phids[$phid]; + $file_view_uri = $file->getViewURI(); + + $file_map[$file_remote_uri] = $file_view_uri; + } + } + + foreach ($images as $image) { + $args = $image['args']; + $uri = $args['uri']; + + $direct_uri = idx($file_map, $uri); + if ($direct_uri) { + $img = phutil_tag( + 'img', + array( + 'src' => $direct_uri, + 'alt' => $args['alt'], + 'width' => $args['width'], + 'height' => $args['height'], + )); + } else { + $src_uri = id(new PhutilURI('/file/imageproxy/')) + ->setQueryParam('uri', $uri); + + $img = id(new PHUIRemarkupImageView()) + ->setURI($src_uri) + ->setAlt($args['alt']) + ->setWidth($args['width']) + ->setHeight($args['height']); + } + + $engine->overwriteStoredText($image['token'], $img); + } } private function isURI($uri_string) { @@ -66,4 +167,5 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule { // If it does, we'll try to treat it like a valid URI return preg_match('~^https?\:\/\/.*\z~i', $uri_string); } + } diff --git a/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php b/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php index e63d831a70..3c939f97d5 100644 --- a/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php +++ b/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php @@ -35,22 +35,15 @@ final class MacroCreateMemeConduitAPIMethod extends MacroConduitAPIMethod { protected function execute(ConduitAPIRequest $request) { $user = $request->getUser(); - $macro_name = $request->getValue('macroName'); - $upper_text = $request->getValue('upperText'); - $lower_text = $request->getValue('lowerText'); - - $uri = PhabricatorMacroMemeController::generateMacro( - $user, - $macro_name, - $upper_text, - $lower_text); - - if (!$uri) { - throw new ConduitException('ERR-NOT-FOUND'); - } + $file = id(new PhabricatorMemeEngine()) + ->setViewer($user) + ->setTemplate($request->getValue('macroName')) + ->setAboveText($request->getValue('upperText')) + ->setBelowText($request->getValue('lowerText')) + ->newAsset(); return array( - 'uri' => $uri, + 'uri' => $file->getViewURI(), ); } diff --git a/src/applications/macro/controller/PhabricatorMacroMemeController.php b/src/applications/macro/controller/PhabricatorMacroMemeController.php index 03b13f6205..eb1dcbeb2f 100644 --- a/src/applications/macro/controller/PhabricatorMacroMemeController.php +++ b/src/applications/macro/controller/PhabricatorMacroMemeController.php @@ -13,56 +13,18 @@ final class PhabricatorMacroMemeController $lower_text = $request->getStr('lowertext'); $viewer = $request->getViewer(); - $uri = self::generateMacro($viewer, $macro_name, - $upper_text, $lower_text); - if ($uri === false) { - return new Aphront404Response(); - } - return id(new AphrontRedirectResponse()) - ->setIsExternal(true) - ->setURI($uri); - } - - public static function generateMacro($viewer, $macro_name, $upper_text, - $lower_text) { - $macro = id(new PhabricatorMacroQuery()) + $file = id(new PhabricatorMemeEngine()) ->setViewer($viewer) - ->withNames(array($macro_name)) - ->needFiles(true) - ->executeOne(); - if (!$macro) { - return false; - } - $file = $macro->getFile(); + ->setTemplate($macro_name) + ->setAboveText($request->getStr('above')) + ->setBelowText($request->getStr('below')) + ->newAsset(); - $upper_text = strtoupper($upper_text); - $lower_text = strtoupper($lower_text); - $mixed_text = md5($upper_text).':'.md5($lower_text); - $hash = 'meme'.hash('sha256', $mixed_text); - $xform = id(new PhabricatorTransformedFile()) - ->loadOneWhere('originalphid=%s and transform=%s', - $file->getPHID(), $hash); + $content = array( + 'imageURI' => $file->getViewURI(), + ); - if ($xform) { - $memefile = id(new PhabricatorFileQuery()) - ->setViewer($viewer) - ->withPHIDs(array($xform->getTransformedPHID())) - ->executeOne(); - if ($memefile) { - return $memefile->getBestURI(); - } - } - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $transformers = (new PhabricatorImageTransformer()); - $newfile = $transformers - ->executeMemeTransform($file, $upper_text, $lower_text); - $xfile = new PhabricatorTransformedFile(); - $xfile->setOriginalPHID($file->getPHID()); - $xfile->setTransformedPHID($newfile->getPHID()); - $xfile->setTransform($hash); - $xfile->save(); - - return $newfile->getBestURI(); + return id(new AphrontAjaxResponse())->setContent($content); } + } diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php new file mode 100644 index 0000000000..f72a161171 --- /dev/null +++ b/src/applications/macro/engine/PhabricatorMemeEngine.php @@ -0,0 +1,384 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setTemplate($template) { + $this->template = $template; + return $this; + } + + public function getTemplate() { + return $this->template; + } + + public function setAboveText($above_text) { + $this->aboveText = $above_text; + return $this; + } + + public function getAboveText() { + return $this->aboveText; + } + + public function setBelowText($below_text) { + $this->belowText = $below_text; + return $this; + } + + public function getBelowText() { + return $this->belowText; + } + + public function getGenerateURI() { + return id(new PhutilURI('/macro/meme/')) + ->alter('macro', $this->getTemplate()) + ->alter('above', $this->getAboveText()) + ->alter('below', $this->getBelowText()); + } + + public function newAsset() { + $cache = $this->loadCachedFile(); + if ($cache) { + return $cache; + } + + $template = $this->loadTemplateFile(); + if (!$template) { + throw new Exception( + pht( + 'Template "%s" is not a valid template.', + $template)); + } + + $hash = $this->newTransformHash(); + + $asset = $this->newAssetFile($template); + + $xfile = id(new PhabricatorTransformedFile()) + ->setOriginalPHID($template->getPHID()) + ->setTransformedPHID($asset->getPHID()) + ->setTransform($hash); + + try { + $caught = null; + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + try { + $xfile->save(); + } catch (Exception $ex) { + $caught = $ex; + } + unset($unguarded); + + if ($caught) { + throw $caught; + } + + return $asset; + } catch (AphrontDuplicateKeyQueryException $ex) { + $xfile = $this->loadCachedFile(); + if (!$xfile) { + throw $ex; + } + return $xfile; + } + } + + private function newTransformHash() { + $properties = array( + 'kind' => 'meme', + 'above' => phutil_utf8_strtoupper($this->getAboveText()), + 'below' => phutil_utf8_strtoupper($this->getBelowText()), + ); + + $properties = phutil_json_encode($properties); + + return PhabricatorHash::digestForIndex($properties); + } + + public function loadCachedFile() { + $viewer = $this->getViewer(); + + $template_file = $this->loadTemplateFile(); + if (!$template_file) { + return null; + } + + $hash = $this->newTransformHash(); + + $xform = id(new PhabricatorTransformedFile())->loadOneWhere( + 'originalPHID = %s AND transform = %s', + $template_file->getPHID(), + $hash); + if (!$xform) { + return null; + } + + return id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($xform->getTransformedPHID())) + ->executeOne(); + } + + private function loadTemplateFile() { + if ($this->templateFile === null) { + $viewer = $this->getViewer(); + $template = $this->getTemplate(); + + $macro = id(new PhabricatorMacroQuery()) + ->setViewer($viewer) + ->withNames(array($template)) + ->needFiles(true) + ->executeOne(); + if (!$macro) { + return null; + } + + $this->templateFile = $macro->getFile(); + } + + return $this->templateFile; + } + + private function newAssetFile(PhabricatorFile $template) { + $data = $this->newAssetData($template); + return PhabricatorFile::newFromFileData( + $data, + array( + 'name' => 'meme-'.$template->getName(), + 'canCDN' => true, + + // In modern code these can end up linked directly in email, so let + // them stick around for a while. + 'ttl.relative' => phutil_units('30 days in seconds'), + )); + } + + private function newAssetData(PhabricatorFile $template) { + $template_data = $template->loadFileData(); + + $result = $this->newImagemagickAsset($template, $template_data); + if ($result) { + return $result; + } + + return $this->newGDAsset($template, $template_data); + } + + private function newImagemagickAsset( + PhabricatorFile $template, + $template_data) { + + // We're only going to use Imagemagick on GIFs. + $mime_type = $template->getMimeType(); + if ($mime_type != 'image/gif') { + return null; + } + + // We're only going to use Imagemagick if it is actually available. + $available = PhabricatorEnv::getEnvConfig('files.enable-imagemagick'); + if (!$available) { + return null; + } + + // Test of the GIF is an animated GIF. If it's a flat GIF, we'll fall + // back to GD. + $input = new TempFile(); + Filesystem::writeFile($input, $template_data); + list($err, $out) = exec_manual('convert %s info:', $input); + if ($err) { + return null; + } + + $split = phutil_split_lines($out); + $frames = count($split); + if ($frames <= 1) { + return null; + } + + // Split the frames apart, transform each frame, then merge them back + // together. + $output = new TempFile(); + + $future = new ExecFuture( + 'convert %s -coalesce +adjoin %s_%s', + $input, + $input, + '%09d'); + $future->setTimeout(10)->resolvex(); + + $output_files = array(); + for ($ii = 0; $ii < $frames; $ii++) { + $frame_name = sprintf('%s_%09d', $input, $ii); + $output_name = sprintf('%s_%09d', $output, $ii); + + $output_files[] = $output_name; + + $frame_data = Filesystem::readFile($frame_name); + $memed_frame_data = $this->newGDAsset($template, $frame_data); + Filesystem::writeFile($output_name, $memed_frame_data); + } + + $future = new ExecFuture('convert -loop 0 %Ls %s', $output_files, $output); + $future->setTimeout(10)->resolvex(); + + return Filesystem::readFile($output); + } + + private function newGDAsset(PhabricatorFile $template, $data) { + $img = imagecreatefromstring($data); + if (!$img) { + throw new Exception( + pht('Failed to imagecreatefromstring() image template data.')); + } + + $dx = imagesx($img); + $dy = imagesy($img); + + $metrics = $this->getMetrics($dx, $dy); + $font = $this->getFont(); + $size = $metrics['size']; + + $above = $this->getAboveText(); + if (strlen($above)) { + $x = (int)floor(($dx - $metrics['text']['above']['width']) / 2); + $y = $metrics['text']['above']['height'] + 12; + + $this->drawText($img, $font, $metrics['size'], $x, $y, $above); + } + + $below = $this->getBelowText(); + if (strlen($below)) { + $x = (int)floor(($dx - $metrics['text']['below']['width']) / 2); + $y = $dy - 12 - $metrics['text']['below']['descend']; + + $this->drawText($img, $font, $metrics['size'], $x, $y, $below); + } + + return PhabricatorImageTransformer::saveImageDataInAnyFormat( + $img, + $template->getMimeType()); + } + + private function getFont() { + $phabricator_root = dirname(phutil_get_library_root('phabricator')); + + $font_root = $phabricator_root.'/resources/font/'; + if (Filesystem::pathExists($font_root.'impact.ttf')) { + $font_path = $font_root.'impact.ttf'; + } else { + $font_path = $font_root.'tuffy.ttf'; + } + + return $font_path; + } + + private function getMetrics($dim_x, $dim_y) { + if ($this->metrics === null) { + $font = $this->getFont(); + + $font_max = 72; + $font_min = 5; + + $last = null; + $cursor = floor(($font_max + $font_min) / 2); + $min = $font_min; + $max = $font_max; + + $texts = array( + 'above' => $this->getAboveText(), + 'below' => $this->getBelowText(), + ); + + $metrics = null; + $best = null; + while (true) { + $all_fit = true; + $text_metrics = array(); + foreach ($texts as $key => $text) { + $box = imagettfbbox($cursor, 0, $font, $text); + $height = abs($box[3] - $box[5]); + $width = abs($box[0] - $box[2]); + + // This is the number of pixels below the baseline that the + // text extends, for example if it has a "y". + $descend = $box[3]; + + if ($height > $dim_y) { + $all_fit = false; + break; + } + + if ($width > $dim_x) { + $all_fit = false; + break; + } + + $text_metrics[$key]['width'] = $width; + $text_metrics[$key]['height'] = $height; + $text_metrics[$key]['descend'] = $descend; + } + + if ($all_fit || $best === null) { + $best = $cursor; + $metrics = $text_metrics; + } + + if ($all_fit) { + $min = $cursor; + } else { + $max = $cursor; + } + + $last = $cursor; + $cursor = floor(($max + $min) / 2); + if ($cursor === $last) { + break; + } + } + + $this->metrics = array( + 'size' => $best, + 'text' => $metrics, + ); + } + + return $this->metrics; + } + + private function drawText($img, $font, $size, $x, $y, $text) { + $text_color = imagecolorallocate($img, 255, 255, 255); + $border_color = imagecolorallocate($img, 0, 0, 0); + + $border = 2; + for ($xx = ($x - $border); $xx <= ($x + $border); $xx += $border) { + for ($yy = ($y - $border); $yy <= ($y + $border); $yy += $border) { + if (($xx === $x) && ($yy === $y)) { + continue; + } + imagettftext($img, $size, 0, $xx, $yy, $border_color, $font, $text); + } + } + + imagettftext($img, $size, 0, $x, $y, $text_color, $font, $text); + } + + +} diff --git a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php index 004e415450..eacb1a33ed 100644 --- a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php +++ b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php @@ -29,34 +29,72 @@ final class PhabricatorMemeRemarkupRule extends PhutilRemarkupRule { $parser = new PhutilSimpleOptions(); $options = $parser->parse($matches[1]) + $options; - $uri = id(new PhutilURI('/macro/meme/')) - ->alter('macro', $options['src']) - ->alter('uppertext', $options['above']) - ->alter('lowertext', $options['below']); + $engine = id(new PhabricatorMemeEngine()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setTemplate($options['src']) + ->setAboveText($options['above']) + ->setBelowText($options['below']); - if ($this->getEngine()->isHTMLMailMode()) { - $uri = PhabricatorEnv::getProductionURI($uri); + $asset = $engine->loadCachedFile(); + + $is_html_mail = $this->getEngine()->isHTMLMailMode(); + $is_text = $this->getEngine()->isTextMode(); + $must_inline = ($is_html_mail || $is_text); + + if ($must_inline) { + if (!$asset) { + try { + $asset = $engine->newAsset(); + } catch (Exception $ex) { + return $matches[0]; + } + } } - if ($this->getEngine()->isTextMode()) { - $img = - ($options['above'] != '' ? "\"{$options['above']}\"\n" : ''). - $options['src'].' <'.PhabricatorEnv::getProductionURI($uri).'>'. - ($options['below'] != '' ? "\n\"{$options['below']}\"" : ''); + if ($asset) { + $uri = $asset->getViewURI(); } else { - $alt_text = pht( - 'Macro %s: %s %s', - $options['src'], - $options['above'], - $options['below']); + $uri = $engine->getGenerateURI(); + } + if ($is_text) { + $parts = array(); + + $above = $options['above']; + if (strlen($above)) { + $parts[] = pht('"%s"', $above); + } + + $parts[] = $options['src'].' <'.$uri.'>'; + + $below = $options['below']; + if (strlen($below)) { + $parts[] = pht('"%s"', $below); + } + + $parts = implode("\n", $parts); + return $this->getEngine()->storeText($parts); + } + + $alt_text = pht( + 'Macro %s: %s %s', + $options['src'], + $options['above'], + $options['below']); + + if ($asset) { $img = $this->newTag( 'img', array( 'src' => $uri, - 'alt' => $alt_text, 'class' => 'phabricator-remarkup-macro', + 'alt' => $alt_text, )); + } else { + $img = id(new PHUIRemarkupImageView()) + ->setURI($uri) + ->addClass('phabricator-remarkup-macro') + ->setAlt($alt_text); } return $this->getEngine()->storeText($img); diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 6301241050..e2739c1d09 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -40,22 +40,6 @@ final class ManiphestTransaction return parent::shouldGenerateOldValue(); } - public function shouldHideForFeed() { - // NOTE: Modular transactions don't currently support this, and it has - // very few callsites, and it's publish-time rather than display-time. - // This should probably become a supported, display-time behavior. For - // discussion, see T12787. - - // Hide "alice created X, a task blocking Y." from feed because it - // will almost always appear adjacent to "alice created Y". - $is_new = $this->getMetadataValue('blocker.new'); - if ($is_new) { - return true; - } - - return parent::shouldHideForFeed(); - } - public function getRequiredHandlePHIDs() { $phids = parent::getRequiredHandlePHIDs(); diff --git a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php index 1c7a88dec7..8833e62b79 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php @@ -112,5 +112,15 @@ final class ManiphestTaskUnblockTransaction return 'fa-shield'; } + public function shouldHideForFeed() { + // Hide "alice created X, a task blocking Y." from feed because it + // will almost always appear adjacent to "alice created Y". + $is_new = $this->getMetadataValue('blocker.new'); + if ($is_new) { + return true; + } + + return parent::shouldHideForFeed(); + } } diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index 3c6ae3836c..c7e96594f3 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -279,7 +279,7 @@ final class PhabricatorOwnersDetailController $href = $repo->generateURI( array( 'branch' => $repo->getDefaultBranch(), - 'path' => $path->getPath(), + 'path' => $path->getPathDisplay(), 'action' => 'browse', )); @@ -288,7 +288,7 @@ final class PhabricatorOwnersDetailController array( 'href' => (string)$href, ), - $path->getPath()); + $path->getPathDisplay()); $rows[] = array( ($path->getExcluded() ? '-' : '+'), diff --git a/src/applications/owners/controller/PhabricatorOwnersPathsController.php b/src/applications/owners/controller/PhabricatorOwnersPathsController.php index e5463b4cb4..d4d97290f9 100644 --- a/src/applications/owners/controller/PhabricatorOwnersPathsController.php +++ b/src/applications/owners/controller/PhabricatorOwnersPathsController.php @@ -27,7 +27,7 @@ final class PhabricatorOwnersPathsController $path_refs = array(); foreach ($paths as $key => $path) { - if (!isset($repos[$key])) { + if (!isset($repos[$key]) || !strlen($repos[$key])) { throw new Exception( pht( 'No repository PHID for path "%s"!', @@ -70,26 +70,39 @@ final class PhabricatorOwnersPathsController $path_refs = mpull($paths, 'getRef'); } - $repos = id(new PhabricatorRepositoryQuery()) - ->setViewer($viewer) - ->execute(); + $template = new AphrontTokenizerTemplateView(); - $default_paths = array(); - foreach ($repos as $repo) { - $default_path = $repo->getDetail('default-owners-path'); - if ($default_path) { - $default_paths[$repo->getPHID()] = $default_path; - } + $datasource = id(new DiffusionRepositoryDatasource()) + ->setViewer($viewer); + + $tokenizer_spec = array( + 'markup' => $template->render(), + 'config' => array( + 'src' => $datasource->getDatasourceURI(), + 'browseURI' => $datasource->getBrowseURI(), + 'placeholder' => $datasource->getPlaceholderText(), + 'limit' => 1, + ), + ); + + foreach ($path_refs as $key => $path_ref) { + $path_refs[$key]['repositoryValue'] = $datasource->getWireTokens( + array( + $path_ref['repositoryPHID'], + )); } + $icon_test = id(new PHUIIconView()) + ->setIcon('fa-spinner grey') + ->setTooltip(pht('Validating...')); - $repo_map = array(); - foreach ($repos as $key => $repo) { - $monogram = $repo->getMonogram(); - $name = $repo->getName(); - $repo_map[$repo->getPHID()] = "{$monogram} {$name}"; - } - asort($repos); + $icon_okay = id(new PHUIIconView()) + ->setIcon('fa-check-circle green') + ->setTooltip(pht('Path Exists in Repository')); + + $icon_fail = id(new PHUIIconView()) + ->setIcon('fa-question-circle-o red') + ->setTooltip(pht('Path Not Found On Default Branch')); $template = new AphrontTypeaheadTemplateView(); $template = $template->render(); @@ -97,17 +110,23 @@ final class PhabricatorOwnersPathsController Javelin::initBehavior( 'owners-path-editor', array( - 'root' => 'path-editor', - 'table' => 'paths', - 'add_button' => 'addpath', - 'repositories' => $repo_map, - 'input_template' => $template, - 'pathRefs' => $path_refs, - - 'completeURI' => '/diffusion/services/path/complete/', - 'validateURI' => '/diffusion/services/path/validate/', - - 'repositoryDefaultPaths' => $default_paths, + 'root' => 'path-editor', + 'table' => 'paths', + 'add_button' => 'addpath', + 'input_template' => $template, + 'pathRefs' => $path_refs, + 'completeURI' => '/diffusion/services/path/complete/', + 'validateURI' => '/diffusion/services/path/validate/', + 'repositoryTokenizerSpec' => $tokenizer_spec, + 'icons' => array( + 'test' => hsprintf('%s', $icon_test), + 'okay' => hsprintf('%s', $icon_okay), + 'fail' => hsprintf('%s', $icon_fail), + ), + 'modeOptions' => array( + 0 => pht('Include'), + 1 => pht('Exclude'), + ), )); require_celerity_resource('owners-path-editor-css'); diff --git a/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php b/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php index ad5415ef69..19e699e8ff 100644 --- a/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php +++ b/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php @@ -22,7 +22,7 @@ final class PhabricatorOwnersPathsSearchEngineAttachment foreach ($paths as $path) { $list[] = array( 'repositoryPHID' => $path->getRepositoryPHID(), - 'path' => $path->getPath(), + 'path' => $path->getPathDisplay(), 'excluded' => (bool)$path->getExcluded(), ); } diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php index ce411aad35..ff0665b344 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php @@ -206,8 +206,8 @@ final class PhabricatorOwnersPackageQuery if ($this->paths !== null) { $where[] = qsprintf( $conn, - 'rpath.path IN (%Ls)', - $this->getFragmentsForPaths($this->paths)); + 'rpath.pathIndex IN (%Ls)', + $this->getFragmentIndexesForPaths($this->paths)); } if ($this->statuses !== null) { @@ -220,13 +220,13 @@ final class PhabricatorOwnersPackageQuery if ($this->controlMap) { $clauses = array(); foreach ($this->controlMap as $repository_phid => $paths) { - $fragments = $this->getFragmentsForPaths($paths); + $indexes = $this->getFragmentIndexesForPaths($paths); $clauses[] = qsprintf( $conn, - '(rpath.repositoryPHID = %s AND rpath.path IN (%Ls))', + '(rpath.repositoryPHID = %s AND rpath.pathIndex IN (%Ls))', $repository_phid, - $fragments); + $indexes); } $where[] = implode(' OR ', $clauses); } @@ -333,6 +333,16 @@ final class PhabricatorOwnersPackageQuery return $fragments; } + private function getFragmentIndexesForPaths(array $paths) { + $indexes = array(); + + foreach ($this->getFragmentsForPaths($paths) as $fragment) { + $indexes[] = PhabricatorHash::digestForIndex($fragment); + } + + return $indexes; + } + /* -( Path Control )------------------------------------------------------- */ diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index a09137c4df..56dd51a3c6 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -16,7 +16,6 @@ final class PhabricatorOwnersPackage protected $auditingEnabled; protected $autoReview; protected $description; - protected $primaryOwnerPHID; protected $mailKey; protected $status; protected $viewPolicy; @@ -33,8 +32,11 @@ final class PhabricatorOwnersPackage const AUTOREVIEW_NONE = 'none'; const AUTOREVIEW_SUBSCRIBE = 'subscribe'; + const AUTOREVIEW_SUBSCRIBE_ALWAYS = 'subscribe-always'; const AUTOREVIEW_REVIEW = 'review'; + const AUTOREVIEW_REVIEW_ALWAYS = 'review-always'; const AUTOREVIEW_BLOCK = 'block'; + const AUTOREVIEW_BLOCK_ALWAYS = 'block-always'; const DOMINION_STRONG = 'strong'; const DOMINION_WEAK = 'weak'; @@ -74,14 +76,26 @@ final class PhabricatorOwnersPackage self::AUTOREVIEW_NONE => array( 'name' => pht('No Autoreview'), ), - self::AUTOREVIEW_SUBSCRIBE => array( - 'name' => pht('Subscribe to Changes'), - ), self::AUTOREVIEW_REVIEW => array( - 'name' => pht('Review Changes'), + 'name' => pht('Review Changes With Non-Owner Author'), + 'authority' => true, ), self::AUTOREVIEW_BLOCK => array( - 'name' => pht('Review Changes (Blocking)'), + 'name' => pht('Review Changes With Non-Owner Author (Blocking)'), + 'authority' => true, + ), + self::AUTOREVIEW_SUBSCRIBE => array( + 'name' => pht('Subscribe to Changes With Non-Owner Author'), + 'authority' => true, + ), + self::AUTOREVIEW_REVIEW_ALWAYS => array( + 'name' => pht('Review All Changes'), + ), + self::AUTOREVIEW_BLOCK_ALWAYS => array( + 'name' => pht('Review All Changes (Blocking)'), + ), + self::AUTOREVIEW_SUBSCRIBE_ALWAYS => array( + 'name' => pht('Subscribe to All Changes'), ), ); } @@ -107,7 +121,6 @@ final class PhabricatorOwnersPackage self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort', 'description' => 'text', - 'primaryOwnerPHID' => 'phid?', 'auditingEnabled' => 'bool', 'mailKey' => 'bytes20', 'status' => 'text32', @@ -203,15 +216,20 @@ final class PhabricatorOwnersPackage // and then merge results in PHP. $rows = array(); - foreach (array_chunk(array_keys($fragments), 128) as $chunk) { + foreach (array_chunk(array_keys($fragments), 1024) as $chunk) { + $indexes = array(); + foreach ($chunk as $fragment) { + $indexes[] = PhabricatorHash::digestForIndex($fragment); + } + $rows[] = queryfx_all( $conn, 'SELECT pkg.id, pkg.dominion, p.excluded, p.path FROM %T pkg JOIN %T p ON p.packageID = pkg.id - WHERE p.path IN (%Ls) AND pkg.status IN (%Ls) %Q', + WHERE p.pathIndex IN (%Ls) AND pkg.status IN (%Ls) %Q', $package->getTableName(), $path->getTableName(), - $chunk, + $indexes, array( self::STATUS_ACTIVE, ), @@ -581,6 +599,18 @@ final class PhabricatorOwnersPackage ->setKey('owners') ->setType('list>') ->setDescription(pht('List of package owners.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('review') + ->setType('map') + ->setDescription(pht('Auto review information.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('audit') + ->setType('map') + ->setDescription(pht('Auto audit information.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('dominion') + ->setType('map') + ->setDescription(pht('Dominion setting information.')), ); } @@ -592,11 +622,56 @@ final class PhabricatorOwnersPackage ); } + $review_map = self::getAutoreviewOptionsMap(); + $review_value = $this->getAutoReview(); + if (isset($review_map[$review_value])) { + $review_label = $review_map[$review_value]['name']; + } else { + $review_label = pht('Unknown ("%s")', $review_value); + } + + $review = array( + 'value' => $review_value, + 'label' => $review_label, + ); + + if ($this->getAuditingEnabled()) { + $audit_value = 'audit'; + $audit_label = pht('Auditing Enabled'); + } else { + $audit_value = 'none'; + $audit_label = pht('No Auditing'); + } + + $audit = array( + 'value' => $audit_value, + 'label' => $audit_label, + ); + + $dominion_value = $this->getDominion(); + $dominion_map = self::getDominionOptionsMap(); + if (isset($dominion_map[$dominion_value])) { + $dominion_label = $dominion_map[$dominion_value]['name']; + $dominion_short = $dominion_map[$dominion_value]['short']; + } else { + $dominion_label = pht('Unknown ("%s")', $dominion_value); + $dominion_short = pht('Unknown ("%s")', $dominion_value); + } + + $dominion = array( + 'value' => $dominion_value, + 'label' => $dominion_label, + 'short' => $dominion_short, + ); + return array( 'name' => $this->getName(), 'description' => $this->getDescription(), 'status' => $this->getStatus(), 'owners' => $owner_list, + 'review' => $review, + 'audit' => $audit, + 'dominion' => $dominion, ); } diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php index f240db2851..7022cb887c 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPath.php +++ b/src/applications/owners/storage/PhabricatorOwnersPath.php @@ -4,7 +4,9 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO { protected $packageID; protected $repositoryPHID; + protected $pathIndex; protected $path; + protected $pathDisplay; protected $excluded; private $fragments; @@ -14,23 +16,35 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO { return array( self::CONFIG_TIMESTAMPS => false, self::CONFIG_COLUMN_SCHEMA => array( - 'path' => 'text255', + 'path' => 'text', + 'pathDisplay' => 'text', + 'pathIndex' => 'bytes12', 'excluded' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( - 'packageID' => array( - 'columns' => array('packageID'), + 'key_path' => array( + 'columns' => array('packageID', 'repositoryPHID', 'pathIndex'), + 'unique' => true, + ), + 'key_repository' => array( + 'columns' => array('repositoryPHID', 'pathIndex'), ), ), ) + parent::getConfiguration(); } - public static function newFromRef(array $ref) { $path = new PhabricatorOwnersPath(); $path->repositoryPHID = $ref['repositoryPHID']; - $path->path = $ref['path']; + + $raw_path = $ref['path']; + + $path->pathIndex = PhabricatorHash::digestForIndex($raw_path); + $path->path = $raw_path; + $path->pathDisplay = $raw_path; + $path->excluded = $ref['excluded']; + return $path; } @@ -38,6 +52,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO { return array( 'repositoryPHID' => $this->getRepositoryPHID(), 'path' => $this->getPath(), + 'display' => $this->getPathDisplay(), 'excluded' => (int)$this->getExcluded(), ); } diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php index 5952b78aed..984b26b8ac 100644 --- a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php +++ b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php @@ -103,6 +103,26 @@ final class PhabricatorOwnersPackagePathsTransaction $paths = $object->getPaths(); + // We store paths in a normalized format with a trailing slash, regardless + // of whether the user enters "path/to/file.c" or "src/backend/". Normalize + // paths now. + + $display_map = array(); + foreach ($new as $key => $spec) { + $display_path = $spec['path']; + $raw_path = rtrim($display_path, '/').'/'; + + // If the user entered two paths which normalize to the same value + // (like "src/main.c" and "src/main.c/"), discard the duplicates. + if (isset($display_map[$raw_path])) { + unset($new[$key]); + continue; + } + + $new[$key]['path'] = $raw_path; + $display_map[$raw_path] = $display_path; + } + $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new); list($rem, $add) = $diffs; @@ -111,12 +131,24 @@ final class PhabricatorOwnersPackagePathsTransaction $ref = $path->getRef(); if (PhabricatorOwnersPath::isRefInSet($ref, $set)) { $path->delete(); + continue; + } + + // If the user has changed the display value for a path but the raw + // storage value hasn't changed, update the display value. + + if (isset($display_map[$path->getPath()])) { + $path + ->setPathDisplay($display_map[$path->getPath()]) + ->save(); + continue; } } foreach ($add as $ref) { $path = PhabricatorOwnersPath::newFromRef($ref) ->setPackageID($object->getID()) + ->setPathDisplay($display_map[$ref['path']]) ->save(); } } diff --git a/src/applications/phriction/storage/PhrictionTransaction.php b/src/applications/phriction/storage/PhrictionTransaction.php index 2ce9d38a08..860a86be35 100644 --- a/src/applications/phriction/storage/PhrictionTransaction.php +++ b/src/applications/phriction/storage/PhrictionTransaction.php @@ -54,18 +54,6 @@ final class PhrictionTransaction return parent::shouldHideForMail($xactions); } - public function shouldHideForFeed() { - switch ($this->getTransactionType()) { - case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE: - case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE: - return true; - case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE: - return $this->getMetadataValue('stub:create:phid', false); - } - return parent::shouldHideForFeed(); - } - - public function getMailTags() { $tags = array(); switch ($this->getTransactionType()) { diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php index c827f7337d..3cd45f73b7 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php @@ -59,4 +59,8 @@ final class PhrictionDocumentMoveAwayTransaction return 'fa-arrows'; } + public function shouldHideForFeed() { + return true; + } + } diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php index a95e70ebdb..bdf2c5d8fc 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php @@ -102,4 +102,8 @@ final class PhrictionDocumentMoveToTransaction return 'fa-arrows'; } + public function shouldHideForFeed() { + return true; + } + } diff --git a/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php b/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php index 112ef09885..3b472bbae7 100644 --- a/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php +++ b/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php @@ -37,7 +37,7 @@ final class ReleephDiffChurnFieldSpecification case PhabricatorTransactions::TYPE_COMMENT: $comments++; break; - case DifferentialTransaction::TYPE_UPDATE: + case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: $updates++; break; case DifferentialTransaction::TYPE_ACTION: diff --git a/src/applications/repository/engine/PhabricatorRepositoryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryEngine.php index 244ed0cd45..fed5b09521 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryEngine.php @@ -56,19 +56,18 @@ abstract class PhabricatorRepositoryEngine extends Phobject { $lock_key, $lock_device_only) { - $lock_parts = array(); - $lock_parts[] = $lock_key; - $lock_parts[] = $repository->getID(); + $lock_parts = array( + 'repositoryPHID' => $repository->getPHID(), + ); if ($lock_device_only) { $device = AlmanacKeys::getLiveDevice(); if ($device) { - $lock_parts[] = $device->getID(); + $lock_parts['devicePHID'] = $device->getPHID(); } } - $lock_name = implode(':', $lock_parts); - return PhabricatorGlobalLock::newLock($lock_name); + return PhabricatorGlobalLock::newLock($lock_key, $lock_parts); } diff --git a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php index 5cd88183e1..9b5b721edb 100644 --- a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php @@ -24,6 +24,9 @@ final class PhabricatorRepositorySearchEngine id(new PhabricatorSearchStringListField()) ->setLabel(pht('Callsigns')) ->setKey('callsigns'), + id(new PhabricatorSearchStringListField()) + ->setLabel(pht('Short Names')) + ->setKey('shortNames'), id(new PhabricatorSearchSelectField()) ->setLabel(pht('Status')) ->setKey('status') @@ -51,6 +54,10 @@ final class PhabricatorRepositorySearchEngine $query->withCallsigns($map['callsigns']); } + if ($map['shortNames']) { + $query->withSlugs($map['shortNames']); + } + if ($map['status']) { $status = idx($this->getStatusValues(), $map['status']); if ($status) { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index ea171a421c..de8139b145 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -2092,7 +2092,7 @@ abstract class PhabricatorEditEngine return array( 'object' => array( - 'id' => $object->getID(), + 'id' => (int)$object->getID(), 'phid' => $object->getPHID(), ), 'transactions' => $xactions_struct, diff --git a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php index 6cc18d5181..305e38ab09 100644 --- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php +++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php @@ -6,6 +6,8 @@ class PhabricatorApplicationTransactionFeedStory extends PhabricatorFeedStory { + private $primaryTransactionPHID; + public function getPrimaryObjectPHID() { return $this->getValue('objectPHID'); } @@ -27,7 +29,36 @@ class PhabricatorApplicationTransactionFeedStory } protected function getPrimaryTransactionPHID() { - return head($this->getValue('transactionPHIDs')); + if ($this->primaryTransactionPHID === null) { + // Transactions are filtered and sorted before they're stored, but the + // rendering logic can change between the time an edit occurs and when + // we actually render the story. Recalculate the filtering at display + // time because it's cheap and gets us better results when things change + // by letting the changes apply retroactively. + + $xaction_phids = $this->getValue('transactionPHIDs'); + + $xactions = array(); + foreach ($xaction_phids as $xaction_phid) { + $xactions[] = $this->getObject($xaction_phid); + } + + foreach ($xactions as $key => $xaction) { + if ($xaction->shouldHideForFeed()) { + unset($xactions[$key]); + } + } + + if ($xactions) { + $primary_phid = head($xactions)->getPHID(); + } else { + $primary_phid = head($xaction_phids); + } + + $this->primaryTransactionPHID = $primary_phid; + } + + return $this->primaryTransactionPHID; } public function getPrimaryTransaction() { diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php index 9f3c24b946..fff2b842d1 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransaction.php +++ b/src/applications/transactions/storage/PhabricatorModularTransaction.php @@ -92,6 +92,14 @@ abstract class PhabricatorModularTransaction return parent::shouldHide(); } + final public function shouldHideForFeed() { + if ($this->getTransactionImplementation()->shouldHideForFeed()) { + return true; + } + + return parent::shouldHideForFeed(); + } + /* final */ public function getIcon() { $icon = $this->getTransactionImplementation()->getIcon(); if ($icon !== null) { diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index f2b59c8a2b..d93831affc 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -47,6 +47,10 @@ abstract class PhabricatorModularTransactionType return false; } + public function shouldHideForFeed() { + return false; + } + public function getIcon() { return null; } diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner index f30fe5023c..35260b08af 100644 --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -84,21 +84,26 @@ You can configure **Auto Review** for packages. When a new code review is created in Differential which affects code in a package, the package can automatically be added as a subscriber or reviewer. -The available settings are: +The available settings allow you to take these actions: - - **No Autoreview**: This package will not be added to new reviews. - - **Subscribe to Changes**: This package will be added to reviews as a - subscriber. Owners will be notified of changes, but not required to act. - **Review Changes**: This package will be added to reviews as a reviewer. Reviews will appear on the dashboards of package owners. - - **Review Changes (Blocking)** This package will be added to reviews - as a blocking reviewer. A package owner will be required to accept changes + - **Review Changes (Blocking)** This package will be added to reviews as a + blocking reviewer. A package owner will be required to accept changes before they may land. + - **Subscribe to Changes**: This package will be added to reviews as a + subscriber. Owners will be notified of changes, but not required to act. -NOTE: These rules **do not trigger** if the change author is a package owner. -They only apply to changes made by users who aren't already owners. +If you select the **With Non-Owner Author** option for these actions, the +action will not trigger if the author of the revision is a package owner. This +mode may be helpful if you are using Owners mostly to make sure that someone +who is qualified is involved in each change to a piece of code. -These rules also do not trigger if the package has been archived. +If you select the **All** option for these actions, the action will always +trigger even if the author is a package owner. This mode may be helpful if you +are using Owners mostly to suggest reviewers. + +These rules do not trigger if the package has been archived. The intent of this feature is to make it easy to configure simple, reasonable behaviors. If you want more tailored or specific triggers, you can write more diff --git a/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php b/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php index f5960e9504..9cf1c22527 100644 --- a/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php +++ b/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php @@ -100,8 +100,10 @@ abstract class PhabricatorGarbageCollector extends Phobject { // Hold a lock while performing collection to avoid racing other daemons // running the same collectors. - $lock_name = 'gc:'.$this->getCollectorConstant(); - $lock = PhabricatorGlobalLock::newLock($lock_name); + $params = array( + 'collector' => $this->getCollectorConstant(), + ); + $lock = PhabricatorGlobalLock::newLock('gc', $params); try { $lock->lock(5); diff --git a/src/infrastructure/markup/view/PHUIRemarkupImageView.php b/src/infrastructure/markup/view/PHUIRemarkupImageView.php new file mode 100644 index 0000000000..0402f000a4 --- /dev/null +++ b/src/infrastructure/markup/view/PHUIRemarkupImageView.php @@ -0,0 +1,79 @@ +uri = $uri; + return $this; + } + + public function getURI() { + return $this->uri; + } + + public function setWidth($width) { + $this->width = $width; + return $this; + } + + public function getWidth() { + return $this->width; + } + + public function setHeight($height) { + $this->height = $height; + return $this; + } + + public function getHeight() { + return $this->height; + } + + public function setAlt($alt) { + $this->alt = $alt; + return $this; + } + + public function getAlt() { + return $this->alt; + } + + public function addClass($class) { + $this->classes[] = $class; + return $this; + } + + public function render() { + $id = celerity_generate_unique_node_id(); + + Javelin::initBehavior( + 'remarkup-load-image', + array( + 'uri' => (string)$this->uri, + 'imageID' => $id, + )); + + $classes = null; + if ($this->classes) { + $classes = implode(' ', $this->classes); + } + + return phutil_tag( + 'img', + array( + 'id' => $id, + 'width' => $this->getWidth(), + 'height' => $this->getHeight(), + 'alt' => $this->getAlt(), + 'class' => $classes, + )); + } + +} diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php index f6b120ba83..5b9459cfb8 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php @@ -1163,12 +1163,16 @@ abstract class PhabricatorStorageManagementWorkflow // Although we're holding this lock on different databases so it could // have the same name on each as far as the database is concerned, the // locks would be the same within this process. - $ref_key = $api->getRef()->getRefKey(); - $ref_hash = PhabricatorHash::digestForIndex($ref_key); - $lock_name = 'adjust('.$ref_hash.')'; + $parameters = array( + 'refKey' => $api->getRef()->getRefKey(), + ); - return PhabricatorGlobalLock::newLock($lock_name) + // We disable logging for this lock because we may not have created the + // log table yet, or may need to adjust it. + + return PhabricatorGlobalLock::newLock('adjust', $parameters) ->useSpecificConnection($api->getConn(null)) + ->setDisableLogging(true) ->lock(); } diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php index 26e11d1899..827d7e0e3d 100644 --- a/src/infrastructure/util/PhabricatorGlobalLock.php +++ b/src/infrastructure/util/PhabricatorGlobalLock.php @@ -28,8 +28,11 @@ */ final class PhabricatorGlobalLock extends PhutilLock { + private $parameters; private $conn; private $isExternalConnection = false; + private $log; + private $disableLogging; private static $pool = array(); @@ -37,27 +40,42 @@ final class PhabricatorGlobalLock extends PhutilLock { /* -( Constructing Locks )------------------------------------------------- */ - public static function newLock($name) { + public static function newLock($name, $parameters = array()) { $namespace = PhabricatorLiskDAO::getStorageNamespace(); $namespace = PhabricatorHash::digestToLength($namespace, 20); - $full_name = 'ph:'.$namespace.':'.$name; + $parts = array(); + ksort($parameters); + foreach ($parameters as $key => $parameter) { + if (!preg_match('/^[a-zA-Z0-9]+\z/', $key)) { + throw new Exception( + pht( + 'Lock parameter key "%s" must be alphanumeric.', + $key)); + } - $length_limit = 64; - if (strlen($full_name) > $length_limit) { - throw new Exception( - pht( - 'Lock name "%s" is too long (full lock name is "%s"). The '. - 'full lock name must not be longer than %s bytes.', - $name, - $full_name, - new PhutilNumber($length_limit))); + if (!is_scalar($parameter) && !is_null($parameter)) { + throw new Exception( + pht( + 'Lock parameter for key "%s" must be a scalar.', + $key)); + } + + $value = phutil_json_encode($parameter); + $parts[] = "{$key}={$value}"; } + $parts = implode(', ', $parts); + $local = "{$name}({$parts})"; + $local = PhabricatorHash::digestToLength($local, 20); + + $full_name = "ph:{$namespace}:{$local}"; $lock = self::getLock($full_name); if (!$lock) { $lock = new PhabricatorGlobalLock($full_name); self::registerLock($lock); + + $lock->parameters = $parameters; } return $lock; @@ -79,6 +97,11 @@ final class PhabricatorGlobalLock extends PhutilLock { return $this; } + public function setDisableLogging($disable) { + $this->disableLogging = $disable; + return $this; + } + /* -( Implementation )----------------------------------------------------- */ @@ -127,6 +150,24 @@ final class PhabricatorGlobalLock extends PhutilLock { $conn->rememberLock($lock_name); $this->conn = $conn; + + if ($this->shouldLogLock()) { + global $argv; + + $lock_context = array( + 'pid' => getmypid(), + 'host' => php_uname('n'), + 'argv' => $argv, + ); + + $log = id(new PhabricatorDaemonLockLog()) + ->setLockName($lock_name) + ->setLockParameters($this->parameters) + ->setLockContext($lock_context) + ->save(); + + $this->log = $log; + } } protected function doUnlock() { @@ -159,6 +200,32 @@ final class PhabricatorGlobalLock extends PhutilLock { $conn->close(); self::$pool[] = $conn; } + + if ($this->log) { + $log = $this->log; + $this->log = null; + + $conn = $log->establishConnection('w'); + queryfx( + $conn, + 'UPDATE %T SET lockReleased = UNIX_TIMESTAMP() WHERE id = %d', + $log->getTableName(), + $log->getID()); + } + } + + private function shouldLogLock() { + if ($this->disableLogging) { + return false; + } + + $policy = id(new PhabricatorDaemonLockLogGarbageCollector()) + ->getRetentionPolicy(); + if (!$policy) { + return false; + } + + return true; } } diff --git a/webroot/rsrc/css/application/owners/owners-path-editor.css b/webroot/rsrc/css/application/owners/owners-path-editor.css index 026420123b..b703656369 100644 --- a/webroot/rsrc/css/application/owners/owners-path-editor.css +++ b/webroot/rsrc/css/application/owners/owners-path-editor.css @@ -3,7 +3,8 @@ */ .owners-path-editor-table { - margin: 10px; + margin: 10px 0; + width: 100%; } .owners-path-editor-table td { @@ -11,27 +12,38 @@ vertical-align: middle; } -.owners-path-editor-table select.owners-repo { - width: 150px; +.owners-path-editor-table td.owners-path-mode-control { + width: 180px; } -.owners-path-editor-table input { - width: 400px; +.owners-path-editor-table td.owners-path-mode-control select { + width: 100%; } -.owners-path-editor-table div.error-display { - padding: 4px 12px 0; +.owners-path-editor-table td.owners-path-repo-control { + width: 280px; } -.owners-path-editor-table div.validating { - color: {$greytext}; +.owners-path-editor-table td.owners-path-path-control { + width: auto; } -.owners-path-editor-table div.invalid { - color: #aa0000; +.owners-path-editor-table td.owners-path-path-control input { + width: 100%; } -.owners-path-editor-table div.valid { - color: #00aa00; - font-weight: bold; +.owners-path-editor-table td.owners-path-path-control .jx-typeahead-results a { + padding: 4px; +} + +.owners-path-editor-table td.owners-path-icon-control { + width: 18px; +} + +.owners-path-editor-table td.remove-column { + width: 100px; +} + +.owners-path-editor-table td.remove-column a { + display: block; } diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index 8750598780..a51c1ce1d9 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -472,6 +472,13 @@ video.phabricator-media { margin: .5em 1em 0; } +.phabricator-remarkup-image-error { + border: 1px solid {$redborder}; + background: {$sh-redbackground}; + padding: 8px 12px; + color: {$darkgreytext}; +} + .phabricator-remarkup-embed-image { display: inline-block; border: 3px solid white; diff --git a/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js b/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js index 91eec515f5..0dedc08666 100644 --- a/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js +++ b/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js @@ -9,8 +9,7 @@ JX.install('TypeaheadSource', { construct : function() { - this._raw = {}; - this._lookup = {}; + this.resetResults(); this.setNormalizer(JX.TypeaheadNormalizer.normalize); this._excludeIDs = {}; }, @@ -359,6 +358,12 @@ JX.install('TypeaheadSource', { } return str.split(/\s+/g); }, + + resetResults: function() { + this._raw = {}; + this._lookup = {}; + }, + _defaultTransformer : function(object) { return { name : object[0], diff --git a/webroot/rsrc/js/application/herald/PathTypeahead.js b/webroot/rsrc/js/application/herald/PathTypeahead.js index 988d728ced..0ce9823d13 100644 --- a/webroot/rsrc/js/application/herald/PathTypeahead.js +++ b/webroot/rsrc/js/application/herald/PathTypeahead.js @@ -11,27 +11,22 @@ JX.install('PathTypeahead', { construct : function(config) { - this._repositorySelect = config.repo_select; + this._repositoryTokenizer = config.repositoryTokenizer; this._hardpoint = config.hardpoint; this._input = config.path_input; this._completeURI = config.completeURI; this._validateURI = config.validateURI; this._errorDisplay = config.error_display; + this._textInputValues = {}; - /* - * Default values to preload the typeahead with, for extremely common - * cases. - */ - this._textInputValues = config.repositoryDefaultPaths; + this._icons = config.icons; this._initializeDatasource(); this._initializeTypeahead(this._input); }, members : { - /* - * DOM