From a4a22dd2f88916bcaaa51057369b200ffe9d9f02 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 6 May 2018 06:30:28 -0700 Subject: [PATCH 01/10] Mention the "inline comments" rule in the callout for "Large" diffs Summary: See PHI638. When a diff is large (between 100 and 1000 files), we collapse content by default unless a change also has inline comments. This rule isn't explicitly explained anywhere. Although it's not really a critical rule, it fits easily enough into the UI callout. Also render the UI callout in a slightly more modern way and avoid `hsprintf()`. Test Plan: {F5596496} - Also, clicked the "Expand" link and saw everything expand properly. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19430 --- .../DifferentialRevisionViewController.php | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 9959bfabab..6b13e893d5 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -182,25 +182,31 @@ final class DifferentialRevisionViewController if ($large_warning) { $count = $this->getChangesetCount(); - $warning = new PHUIInfoView(); - $warning->setTitle(pht('Large Diff')); - $warning->setSeverity(PHUIInfoView::SEVERITY_WARNING); - $warning->appendChild(hsprintf( - '%s %s', + $expand_uri = $request_uri + ->alter('large', 'true') + ->setFragment('toc'); + + $message = array( pht( - 'This diff is large and affects %s files. '. - 'You may load each file individually or ', + 'This large diff affects %s files. Files without inline '. + 'comments have been collapsed.', new PhutilNumber($count)), + ' ', phutil_tag( - 'a', - array( - 'class' => 'button button-grey', - 'href' => $request_uri - ->alter('large', 'true') - ->setFragment('toc'), - ), - pht('Show All Files Inline')))); - $warning = $warning->render(); + 'strong', + array(), + phutil_tag( + 'a', + array( + 'href' => $expand_uri, + ), + pht('Expand All Files'))), + ); + + $warning = id(new PHUIInfoView()) + ->setTitle(pht('Large Diff')) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->appendChild($message); $old = array_select_keys($changesets, $old_ids); $new = array_select_keys($changesets, $new_ids); From fddb506e987fdae149a1dd6d28dd22f7de399880 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 May 2018 13:55:00 -0700 Subject: [PATCH 02/10] Don't render the Maniphest edit form bottom-of-page preview panel if "Description" is locked or hidden Summary: See . If you hide the "Description" field in Maniphest, we still try to render a remarkup preview for it. This causes a JS error and a nonfunctional element on the page. Instead, hide the preview panel if the field has been locked or hidden. Test Plan: - Hid the field, loaded the form, no more preview panel / JS error. - Used a normal form with the field visible, saw a normal preview. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19432 --- .../transactions/editfield/PhabricatorEditField.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/applications/transactions/editfield/PhabricatorEditField.php b/src/applications/transactions/editfield/PhabricatorEditField.php index 94c95a5052..07bf3589a8 100644 --- a/src/applications/transactions/editfield/PhabricatorEditField.php +++ b/src/applications/transactions/editfield/PhabricatorEditField.php @@ -302,6 +302,14 @@ abstract class PhabricatorEditField extends Phobject { } public function getPreviewPanel() { + if ($this->getIsHidden()) { + return null; + } + + if ($this->getIsLocked()) { + return null; + } + return $this->previewPanel; } From 304c6a45976d434786afdcbe1f1263dd1b55ff7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 May 2018 14:09:15 -0700 Subject: [PATCH 03/10] Improve UI and documentation for "Ignore Attributes" in Owners slightly Summary: See PHI251. Ref T13137. - Replace the perplexing text box with a checkbox that explains what it does. - Mention this feature in the documentation. Test Plan: - Clicked/unclicked checkbox. - Read documentation. - Used an existing checkbox control in Slowvote to make sure I didn't break it. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13137 Differential Revision: https://secure.phabricator.com/D19433 --- src/__phutil_library_map__.php | 2 ++ .../PhabricatorOwnersPackageEditEngine.php | 8 +++-- .../PhabricatorCheckboxesEditField.php | 36 +++++++++++++++++++ src/docs/user/userguide/owners.diviner | 15 ++++++++ .../control/AphrontFormCheckboxControl.php | 32 +++++++++++++++-- 5 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 src/applications/transactions/editfield/PhabricatorCheckboxesEditField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 78ae048715..85ae70c3e3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2525,6 +2525,7 @@ phutil_register_library_map(array( 'PhabricatorChatLogDAO' => 'applications/chatlog/storage/PhabricatorChatLogDAO.php', 'PhabricatorChatLogEvent' => 'applications/chatlog/storage/PhabricatorChatLogEvent.php', 'PhabricatorChatLogQuery' => 'applications/chatlog/query/PhabricatorChatLogQuery.php', + 'PhabricatorCheckboxesEditField' => 'applications/transactions/editfield/PhabricatorCheckboxesEditField.php', 'PhabricatorChunkedFileStorageEngine' => 'applications/files/engine/PhabricatorChunkedFileStorageEngine.php', 'PhabricatorClassConfigType' => 'applications/config/type/PhabricatorClassConfigType.php', 'PhabricatorClusterConfigOptions' => 'applications/config/option/PhabricatorClusterConfigOptions.php', @@ -8140,6 +8141,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', ), 'PhabricatorChatLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorCheckboxesEditField' => 'PhabricatorEditField', 'PhabricatorChunkedFileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorClassConfigType' => 'PhabricatorTextConfigType', 'PhabricatorClusterConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php index 8c48727a0b..d2eee5685e 100644 --- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php @@ -162,13 +162,17 @@ EOTEXT ->setIsConduitOnly(true) ->setValue($object->getStatus()) ->setOptions($object->getStatusNameMap()), - id(new PhabricatorStringListEditField()) + id(new PhabricatorCheckboxesEditField()) ->setKey('ignored') ->setLabel(pht('Ignored Attributes')) ->setDescription(pht('Ignore paths with any of these attributes.')) ->setTransactionType( PhabricatorOwnersPackageIgnoredTransaction::TRANSACTIONTYPE) - ->setValue(array_keys($object->getIgnoredPathAttributes())), + ->setValue(array_keys($object->getIgnoredPathAttributes())) + ->setOptions( + array( + 'generated' => pht('Ignore generated files (review only).'), + )), id(new PhabricatorConduitEditField()) ->setKey('paths.set') ->setLabel(pht('Paths')) diff --git a/src/applications/transactions/editfield/PhabricatorCheckboxesEditField.php b/src/applications/transactions/editfield/PhabricatorCheckboxesEditField.php new file mode 100644 index 0000000000..82d85c4425 --- /dev/null +++ b/src/applications/transactions/editfield/PhabricatorCheckboxesEditField.php @@ -0,0 +1,36 @@ +getOptions(); + + return id(new AphrontFormCheckboxControl()) + ->setOptions($options); + } + + protected function newConduitParameterType() { + return new ConduitStringListParameterType(); + } + + protected function newHTTPParameterType() { + return new AphrontStringListHTTPParameterType(); + } + + public function setOptions(array $options) { + $this->options = $options; + return $this; + } + + public function getOptions() { + if ($this->options === null) { + throw new PhutilInvalidStateException('setOptions'); + } + + return $this->options; + } + +} diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner index 35260b08af..95a3882552 100644 --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -132,6 +132,21 @@ behavior. If you want more powerful auditing behavior, you can use Herald to write more sophisticated rules. +Ignored Attributes +================== + +You can automatically exclude certain types of files, like generated files, +with **Ignored Attributes**. + +When a package is marked as ignoring files with a particular attribute, and +a file in a particular change has that attribute, the file will be ignored when +computing ownership. + +(This feature is currently rough, only works for Differential revisions, and +may not always compute the correct set of owning packages in some complex +cases where it interacts with dominion rules.) + + Files in Multiple Packages ========================== diff --git a/src/view/form/control/AphrontFormCheckboxControl.php b/src/view/form/control/AphrontFormCheckboxControl.php index ed6cde3776..e030b7d17e 100644 --- a/src/view/form/control/AphrontFormCheckboxControl.php +++ b/src/view/form/control/AphrontFormCheckboxControl.php @@ -34,6 +34,20 @@ final class AphrontFormCheckboxControl extends AphrontFormControl { return 'aphront-form-control-checkbox'; } + public function setOptions(array $options) { + $boxes = array(); + foreach ($options as $key => $value) { + $boxes[] = array( + 'value' => $key, + 'label' => $value, + ); + } + + $this->boxes = $boxes; + + return $this; + } + protected function renderInput() { $rows = array(); foreach ($this->boxes as $box) { @@ -41,14 +55,28 @@ final class AphrontFormCheckboxControl extends AphrontFormControl { if ($id === null) { $id = celerity_generate_unique_node_id(); } + + $name = idx($box, 'name'); + if ($name === null) { + $name = $this->getName().'[]'; + } + + $value = $box['value']; + + if (array_key_exists('checked', $box)) { + $checked = $box['checked']; + } else { + $checked = in_array($value, $this->getValue()); + } + $checkbox = phutil_tag( 'input', array( 'id' => $id, 'type' => 'checkbox', - 'name' => $box['name'], + 'name' => $name, 'value' => $box['value'], - 'checked' => $box['checked'] ? 'checked' : null, + 'checked' => $checked ? 'checked' : null, 'disabled' => $this->getDisabled() ? 'disabled' : null, )); $label = phutil_tag( From 397645b2731be94d313252c55baf9fdb21d1346c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 8 May 2018 11:24:10 -0700 Subject: [PATCH 04/10] Export task point values as double, not int Summary: See . We currently export the Maniphest "points" field as an integer, but allow it to accept decimal values (e.g. "6.25"). Also fix a bug where we wouldn't roll over from "..., X, Y, Z, AA, AB, ..." correctly for Excel column names if sheet had more than 26 columns. Test Plan: - Set a task point value to 6.25. - Exported to text, JSON, XLS. - Saw 6.25 represented accurately in exports. - Exported an excel sheet with 27+ columns. - Manually printed the first 200 column names to check that the algorithm looks correct. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19434 --- src/__phutil_library_map__.php | 2 ++ .../query/ManiphestTaskSearchEngine.php | 2 +- .../field/PhabricatorDoubleExportField.php | 25 +++++++++++++++++++ .../format/PhabricatorExcelExportFormat.php | 6 ++++- 4 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 src/infrastructure/export/field/PhabricatorDoubleExportField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 85ae70c3e3..6c91c0f0a4 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2882,6 +2882,7 @@ phutil_register_library_map(array( 'PhabricatorDocumentRef' => 'applications/files/document/PhabricatorDocumentRef.php', 'PhabricatorDocumentRenderingEngine' => 'applications/files/document/render/PhabricatorDocumentRenderingEngine.php', 'PhabricatorDoorkeeperApplication' => 'applications/doorkeeper/application/PhabricatorDoorkeeperApplication.php', + 'PhabricatorDoubleExportField' => 'infrastructure/export/field/PhabricatorDoubleExportField.php', 'PhabricatorDraft' => 'applications/draft/storage/PhabricatorDraft.php', 'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php', 'PhabricatorDraftEngine' => 'applications/transactions/draft/PhabricatorDraftEngine.php', @@ -8540,6 +8541,7 @@ phutil_register_library_map(array( 'PhabricatorDocumentRef' => 'Phobject', 'PhabricatorDocumentRenderingEngine' => 'Phobject', 'PhabricatorDoorkeeperApplication' => 'PhabricatorApplication', + 'PhabricatorDoubleExportField' => 'PhabricatorExportField', 'PhabricatorDraft' => 'PhabricatorDraftDAO', 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', 'PhabricatorDraftEngine' => 'Phobject', diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index f85c79621c..68f87a6f6b 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -516,7 +516,7 @@ final class ManiphestTaskSearchEngine ); if (ManiphestTaskPoints::getIsEnabled()) { - $fields[] = id(new PhabricatorIntExportField()) + $fields[] = id(new PhabricatorDoubleExportField()) ->setKey('points') ->setLabel('Points'); } diff --git a/src/infrastructure/export/field/PhabricatorDoubleExportField.php b/src/infrastructure/export/field/PhabricatorDoubleExportField.php new file mode 100644 index 0000000000..18087dd706 --- /dev/null +++ b/src/infrastructure/export/field/PhabricatorDoubleExportField.php @@ -0,0 +1,25 @@ +setDataType(PHPExcel_Cell_DataType::TYPE_NUMERIC); + } + + public function getCharacterWidth() { + return 8; + } + +} diff --git a/src/infrastructure/export/format/PhabricatorExcelExportFormat.php b/src/infrastructure/export/format/PhabricatorExcelExportFormat.php index 2b0c787884..606df393d0 100644 --- a/src/infrastructure/export/format/PhabricatorExcelExportFormat.php +++ b/src/infrastructure/export/format/PhabricatorExcelExportFormat.php @@ -155,8 +155,12 @@ EOHELP return $this->sheet; } + + /** + * @phutil-external-symbol class PHPExcel_Cell + */ private function getCellName($col, $row = null) { - $col_name = chr(ord('A') + $col); + $col_name = PHPExcel_Cell::stringFromColumnIndex($col); if ($row === null) { return $col_name; From 5b640a434cc4d5d18beae015295deca70b224d95 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 May 2018 13:38:11 -0700 Subject: [PATCH 05/10] Support an "Ancestors Of: ..." constraint in commit queries Summary: Ref T13137. See PHI609. An install would like to filter audit requests on a particular branch, e.g. "master". This is difficult in the general case because we can not apply this constraint efficiently under every conceivable data shape, but we can do a reasonable job in most practical cases. See T13137#238822 for more detailed discussion on the approach here. This is a bit rough, but should do the job for now. Test Plan: - Filtered commits by various branches, e.g. "master"; "lfs". Saw correct-seeming results. - Stubbed out the "just list everything" path to hit the `diffusion.internal.ancestors` path, saw the same correct-seeming results. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13137 Differential Revision: https://secure.phabricator.com/D19431 --- src/__phutil_library_map__.php | 2 + .../query/PhabricatorCommitSearchEngine.php | 11 ++ ...usionInternalAncestorsConduitAPIMethod.php | 51 ++++++ .../diffusion/query/DiffusionCommitQuery.php | 148 +++++++++++++++++- 4 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6c91c0f0a4..e1b13dbecb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -816,6 +816,7 @@ phutil_register_library_map(array( 'DiffusionHovercardEngineExtension' => 'applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php', 'DiffusionInlineCommentController' => 'applications/diffusion/controller/DiffusionInlineCommentController.php', 'DiffusionInlineCommentPreviewController' => 'applications/diffusion/controller/DiffusionInlineCommentPreviewController.php', + 'DiffusionInternalAncestorsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php', 'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php', 'DiffusionLastModifiedController' => 'applications/diffusion/controller/DiffusionLastModifiedController.php', 'DiffusionLastModifiedQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php', @@ -6149,6 +6150,7 @@ phutil_register_library_map(array( 'DiffusionHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', 'DiffusionInlineCommentController' => 'PhabricatorInlineCommentController', 'DiffusionInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController', + 'DiffusionInternalAncestorsConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionLastModifiedController' => 'DiffusionController', 'DiffusionLastModifiedQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 6811427c93..993626e1e3 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -53,6 +53,10 @@ final class PhabricatorCommitSearchEngine $query->withUnreachable($map['unreachable']); } + if ($map['ancestorsOf']) { + $query->withAncestorsOf($map['ancestorsOf']); + } + return $query; } @@ -103,6 +107,13 @@ final class PhabricatorCommitSearchEngine pht( 'Find or exclude unreachable commits which are not ancestors of '. 'any branch, tag, or ref.')), + id(new PhabricatorSearchStringListField()) + ->setLabel(pht('Ancestors Of')) + ->setKey('ancestorsOf') + ->setDescription( + pht( + 'Find commits which are ancestors of a particular ref, '. + 'like "master".')), ); } diff --git a/src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php new file mode 100644 index 0000000000..01808bb4eb --- /dev/null +++ b/src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php @@ -0,0 +1,51 @@ +'; + } + + protected function defineCustomParamTypes() { + return array( + 'ref' => 'required string', + 'commits' => 'required list', + ); + } + + protected function getResult(ConduitAPIRequest $request) { + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $commits = $request->getValue('commits'); + $ref = $request->getValue('ref'); + + $graph = new PhabricatorGitGraphStream($repository, $ref); + + $keep = array(); + foreach ($commits as $identifier) { + try { + $graph->getCommitDate($identifier); + $keep[] = $identifier; + } catch (Exception $ex) { + // Not an ancestor. + } + } + + return $keep; + } + +} diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 8996bab628..53e8d51bf9 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -22,10 +22,14 @@ final class DiffusionCommitQuery private $epochMin; private $epochMax; private $importing; + private $ancestorsOf; private $needCommitData; private $needDrafts; + private $mustFilterRefs = false; + private $refRepository; + public function withIDs(array $ids) { $this->ids = $ids; return $this; @@ -92,7 +96,7 @@ final class DiffusionCommitQuery } public function withRepositoryIDs(array $repository_ids) { - $this->repositoryIDs = $repository_ids; + $this->repositoryIDs = array_unique($repository_ids); return $this; } @@ -152,6 +156,11 @@ final class DiffusionCommitQuery return $this; } + public function withAncestorsOf(array $refs) { + $this->ancestorsOf = $refs; + return $this; + } + public function getIdentifierMap() { if ($this->identifierMap === null) { throw new Exception( @@ -307,6 +316,54 @@ final class DiffusionCommitQuery protected function didFilterPage(array $commits) { $viewer = $this->getViewer(); + if ($this->mustFilterRefs) { + // If this flag is set, the query has an "Ancestors Of" constraint and + // at least one of the constraining refs had too many ancestors for us + // to apply the constraint with a big "commitIdentifier IN (%Ls)" clause. + // We're going to filter each page and hope we get a full result set + // before the query overheats. + + $ancestor_list = mpull($commits, 'getCommitIdentifier'); + $ancestor_list = array_values($ancestor_list); + + foreach ($this->ancestorsOf as $ref) { + try { + $ancestor_list = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + DiffusionRequest::newFromDictionary( + array( + 'repository' => $this->refRepository, + 'user' => $viewer, + )), + 'diffusion.internal.ancestors', + array( + 'ref' => $ref, + 'commits' => $ancestor_list, + )); + } catch (ConduitClientException $ex) { + throw new PhabricatorSearchConstraintException( + $ex->getMessage()); + } + + if (!$ancestor_list) { + break; + } + } + + $ancestor_list = array_fuse($ancestor_list); + foreach ($commits as $key => $commit) { + $identifier = $commit->getCommitIdentifier(); + if (!isset($ancestor_list[$identifier])) { + $this->didRejectResult($commit); + unset($commits[$key]); + } + } + + if (!$commits) { + return $commits; + } + } + if ($this->needCommitData) { $data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( 'commitID in (%Ld)', @@ -364,6 +421,95 @@ final class DiffusionCommitQuery $this->withRepositoryIDs($repository_ids); } + if ($this->ancestorsOf !== null) { + if (count($this->repositoryIDs) !== 1) { + throw new PhabricatorSearchConstraintException( + pht( + 'To search for commits which are ancestors of particular refs, '. + 'you must constrain the search to exactly one repository.')); + } + + $repository_id = head($this->repositoryIDs); + $history_limit = $this->getRawResultLimit() * 32; + $viewer = $this->getViewer(); + + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withIDs(array($repository_id)) + ->executeOne(); + + if (!$repository) { + throw new PhabricatorEmptyQueryException(); + } + + if ($repository->isSVN()) { + throw new PhabricatorSearchConstraintException( + pht( + 'Subversion does not support searching for ancestors of '. + 'a particular ref. This operation is not meaningful in '. + 'Subversion.')); + } + + if ($repository->isHg()) { + throw new PhabricatorSearchConstraintException( + pht( + 'Mercurial does not currently support searching for ancestors of '. + 'a particular ref.')); + } + + $can_constrain = true; + $history_identifiers = array(); + foreach ($this->ancestorsOf as $key => $ref) { + try { + $raw_history = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + DiffusionRequest::newFromDictionary( + array( + 'repository' => $repository, + 'user' => $viewer, + )), + 'diffusion.historyquery', + array( + 'commit' => $ref, + 'limit' => $history_limit, + )); + } catch (ConduitClientException $ex) { + throw new PhabricatorSearchConstraintException( + $ex->getMessage()); + } + + $ref_identifiers = array(); + foreach ($raw_history['pathChanges'] as $change) { + $ref_identifiers[] = $change['commitIdentifier']; + } + + // If this ref had fewer total commits than the limit, we're safe to + // apply the constraint as a large `IN (...)` query for a list of + // commit identifiers. This is efficient. + if ($history_limit) { + if (count($ref_identifiers) >= $history_limit) { + $can_constrain = false; + break; + } + } + + $history_identifiers += array_fuse($ref_identifiers); + } + + // If all refs had a small number of ancestors, we can just put the + // constraint into the query here and we're done. Otherwise, we need + // to filter each page after it comes out of the MySQL layer. + if ($can_constrain) { + $where[] = qsprintf( + $conn, + 'commit.commitIdentifier IN (%Ls)', + $history_identifiers); + } else { + $this->mustFilterRefs = true; + $this->refRepository = $repository; + } + } + if ($this->ids !== null) { $where[] = qsprintf( $conn, From 26c0db8dd797d88f9e6481a3b88622fc28f924c8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 9 May 2018 11:25:08 -0700 Subject: [PATCH 06/10] Allow navigation breadcrumbs to be marked as "always visible" so they show up on phones Summary: See PHI624. Some of the mobile navigation and breadcrumbs in support pacts aren't as good as they could be. In particular, we generally collapse crumbs on mobile to just the first and last crumbs. The first crumb is the application; the last is the current page. On `/PHIxxx` pages, the first crumb isn't very useful since the Support landing page is two levels up: you usually want to go back to the pact, not all the way back to the Support landing page. We also don't need the space since the last crumb (`PHIxxx`) is always small. Allow Support and other similar applications to tailor the crumb behavior more narrowly if they end up in situations like this. Test Plan: - With an additional change to instances (see next diff), viewed a support issue page (`/PHI123`) on mobile and desktop. - Saw a link directly back to the pact on both mobile and desktop. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19438 --- resources/celerity/map.php | 6 +++--- src/view/phui/PHUICrumbView.php | 21 +++++++++++++++++++++ webroot/rsrc/css/phui/phui-crumbs-view.css | 2 ++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 78b3660c4c..dd6ad438fd 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'cb8ae4dc', + 'core.pkg.css' => '8be474cc', 'core.pkg.js' => 'e1f0f7bd', 'differential.pkg.css' => '06dc617c', 'differential.pkg.js' => 'c2ca903a', @@ -144,7 +144,7 @@ return array( 'rsrc/css/phui/phui-cms.css' => '504b4b23', 'rsrc/css/phui/phui-comment-form.css' => 'ac68149f', 'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad', - 'rsrc/css/phui/phui-crumbs-view.css' => '6ece3bbb', + 'rsrc/css/phui/phui-crumbs-view.css' => '10728aaa', 'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026', 'rsrc/css/phui/phui-document-pro.css' => '8af7ea27', 'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf', @@ -810,7 +810,7 @@ return array( 'phui-cms-css' => '504b4b23', 'phui-comment-form-css' => 'ac68149f', 'phui-comment-panel-css' => 'f50152ad', - 'phui-crumbs-view-css' => '6ece3bbb', + 'phui-crumbs-view-css' => '10728aaa', 'phui-curtain-view-css' => '2bdaf026', 'phui-document-summary-view-css' => '9ca48bdf', 'phui-document-view-css' => '878c2f52', diff --git a/src/view/phui/PHUICrumbView.php b/src/view/phui/PHUICrumbView.php index 68c43be4e3..1e5dad2ec6 100644 --- a/src/view/phui/PHUICrumbView.php +++ b/src/view/phui/PHUICrumbView.php @@ -8,6 +8,7 @@ final class PHUICrumbView extends AphrontView { private $isLastCrumb; private $workflow; private $aural; + private $alwaysVisible; public function setAural($aural) { $this->aural = $aural; @@ -18,6 +19,22 @@ final class PHUICrumbView extends AphrontView { return $this->aural; } + /** + * Make this crumb always visible, even on devices where it would normally + * be hidden. + * + * @param bool True to make the crumb always visible. + * @return this + */ + public function setAlwaysVisible($always_visible) { + $this->alwaysVisible = $always_visible; + return $this; + } + + public function getAlwaysVisible() { + return $this->alwaysVisible; + } + public function setWorkflow($workflow) { $this->workflow = $workflow; return $this; @@ -98,6 +115,10 @@ final class PHUICrumbView extends AphrontView { $classes[] = 'phabricator-last-crumb'; } + if ($this->getAlwaysVisible()) { + $classes[] = 'phui-crumb-always-visible'; + } + $tag = javelin_tag( $this->href ? 'a' : 'span', array( diff --git a/webroot/rsrc/css/phui/phui-crumbs-view.css b/webroot/rsrc/css/phui/phui-crumbs-view.css index 6155e9b2b8..0811b36134 100644 --- a/webroot/rsrc/css/phui/phui-crumbs-view.css +++ b/webroot/rsrc/css/phui/phui-crumbs-view.css @@ -55,6 +55,8 @@ } .device-phone .phui-crumb-view.phabricator-last-crumb .phui-crumb-name, +.device-phone .phui-crumb-view.phui-crumb-always-visible .phui-crumb-name, +.device-phone .phui-crumb-view.phui-crumb-always-visible + .phui-crumb-divider, .device-phone .phui-crumb-view.phui-crumb-has-icon, .device-phone .phui-crumb-has-icon + .phui-crumb-divider { display: inline-block; From d280b24239fc0e5d148a7be0724a28223bb95f48 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 9 May 2018 10:49:49 -0700 Subject: [PATCH 07/10] Fix "arc paste" to stop creating pastes with an empty string ("") as the "language" Summary: See PHI652. When you `echo x | arc paste` today, you end up with a Paste object that has the empty string as its "language". This is normally not valid. Pastes where the language should be autodetected should have the value `null`, not the empty string. This behavior likely changed when `paste.create` got rewritten in terms of `paste.edit`. Adjust the implementation so it only adds the LANGUAGE transaction if there's an actual language. Also, fix an issue where you can't use the "delete" key to delete tokens with the empty string as their value. Test Plan: - Created a paste with `echo x | arc paste`, got a paste in autodetect mode instead of with a bogus language value. - Created a paste with `echo x | arc paste --lang rainbow`, got a rainbow paste. - Deleted an empty string token with the keyboard. - Deleted normal tokens with the keyboard. - Edited subscribers/etc normally with the keyboard and mouse to make sure I didn't ruin anything. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19437 --- resources/celerity/map.php | 18 +++++++++--------- .../conduit/PasteCreateConduitAPIMethod.php | 9 ++++++--- .../PhabricatorPasteLanguageTransaction.php | 16 ++++++++++++++++ .../javelin/lib/control/tokenizer/Tokenizer.js | 6 +++++- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index dd6ad438fd..2631b54f8c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', 'core.pkg.css' => '8be474cc', - 'core.pkg.js' => 'e1f0f7bd', + 'core.pkg.js' => 'e452721e', 'differential.pkg.css' => '06dc617c', 'differential.pkg.js' => 'c2ca903a', 'diffusion.pkg.css' => 'a2d17c7d', @@ -259,7 +259,7 @@ return array( 'rsrc/externals/javelin/lib/__tests__/URI.js' => '1e45fda9', 'rsrc/externals/javelin/lib/__tests__/behavior.js' => '1ea62783', 'rsrc/externals/javelin/lib/behavior.js' => '61cbc29a', - 'rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js' => '8d3bc1b2', + 'rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js' => 'dfaf006b', 'rsrc/externals/javelin/lib/control/typeahead/Typeahead.js' => '70baed2f', 'rsrc/externals/javelin/lib/control/typeahead/normalizer/TypeaheadNormalizer.js' => '185bbd53', 'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadCompositeSource.js' => '503e17fd', @@ -710,7 +710,7 @@ return array( 'javelin-scrollbar' => '9065f639', 'javelin-sound' => '949c0fe5', 'javelin-stratcom' => '327f418a', - 'javelin-tokenizer' => '8d3bc1b2', + 'javelin-tokenizer' => 'dfaf006b', 'javelin-typeahead' => '70baed2f', 'javelin-typeahead-composite-source' => '503e17fd', 'javelin-typeahead-normalizer' => '185bbd53', @@ -1573,12 +1573,6 @@ return array( 'javelin-stratcom', 'javelin-behavior', ), - '8d3bc1b2' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - ), '8d4a8c72' => array( 'javelin-install', 'javelin-dom', @@ -2025,6 +2019,12 @@ return array( 'phuix-icon-view', 'phabricator-prefab', ), + 'dfaf006b' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + ), 'e1d25dfb' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/paste/conduit/PasteCreateConduitAPIMethod.php b/src/applications/paste/conduit/PasteCreateConduitAPIMethod.php index 3badd04da7..c62ecd0494 100644 --- a/src/applications/paste/conduit/PasteCreateConduitAPIMethod.php +++ b/src/applications/paste/conduit/PasteCreateConduitAPIMethod.php @@ -64,9 +64,12 @@ final class PasteCreateConduitAPIMethod extends PasteConduitAPIMethod { ->setTransactionType(PhabricatorPasteTitleTransaction::TRANSACTIONTYPE) ->setNewValue($title); - $xactions[] = id(new PhabricatorPasteTransaction()) - ->setTransactionType(PhabricatorPasteLanguageTransaction::TRANSACTIONTYPE) - ->setNewValue($language); + if (strlen($language)) { + $xactions[] = id(new PhabricatorPasteTransaction()) + ->setTransactionType( + PhabricatorPasteLanguageTransaction::TRANSACTIONTYPE) + ->setNewValue($language); + } $editor = id(new PhabricatorPasteEditor()) ->setActor($viewer) diff --git a/src/applications/paste/xaction/PhabricatorPasteLanguageTransaction.php b/src/applications/paste/xaction/PhabricatorPasteLanguageTransaction.php index a0c060d430..8927270c0d 100644 --- a/src/applications/paste/xaction/PhabricatorPasteLanguageTransaction.php +++ b/src/applications/paste/xaction/PhabricatorPasteLanguageTransaction.php @@ -38,4 +38,20 @@ final class PhabricatorPasteLanguageTransaction $this->renderLanguageValue($this->getNewValue())); } + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if ($new !== null && !strlen($new)) { + $errors[] = $this->newInvalidError( + pht('Paste language must be null or a nonempty string.'), + $xaction); + } + } + + return $errors; + } + } diff --git a/webroot/rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js b/webroot/rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js index a60c91e528..5e293577aa 100644 --- a/webroot/rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js +++ b/webroot/rsrc/externals/javelin/lib/control/tokenizer/Tokenizer.js @@ -395,8 +395,12 @@ JX.install('Tokenizer', { break; case 'delete': if (!this._focus.value.length) { + // In unusual cases, it's possible for us to end up with a token + // that has the empty string ("") as a value. Support removal of + // this unusual token. + var tok; - while ((tok = this._tokens.pop())) { + while ((tok = this._tokens.pop()) !== null) { if (this._remove(tok, true)) { break; } From 10a4b05ecba60a35f61083044bffb3a33a4d9be7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 9 May 2018 12:58:10 -0700 Subject: [PATCH 08/10] Fix "Any Owner" and "No Owners" searches in Maniphest Summary: See . These special-token-only searches currently end up populating an empty `ownerPHIDs`, which fatals after the stricter check in D19417. Make the fatal on `withConstraint(array())` explicit and only set the PHID constraint if we have some PHIDs left. Test Plan: Searched for "No Owner", "Any Owner", an actual owner, "No Owner + actual user". Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19440 --- .../maniphest/query/ManiphestTaskQuery.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 16aa83a2ea..ccf93c4a2d 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -73,6 +73,10 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { } public function withOwners(array $owners) { + if ($owners === array()) { + throw new Exception(pht('Empty withOwners() constraint is not valid.')); + } + $no_owner = PhabricatorPeopleNoOwnerDatasource::FUNCTION_TOKEN; $any_owner = PhabricatorPeopleAnyOwnerDatasource::FUNCTION_TOKEN; @@ -88,7 +92,11 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { break; } } - $this->ownerPHIDs = $owners; + + if ($owners) { + $this->ownerPHIDs = $owners; + } + return $this; } From 72813004465261ba8e7a5243d55deb668993c260 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Fri, 11 May 2018 16:18:06 +0000 Subject: [PATCH 09/10] Allow number in generated clone uri Summary: See https://discourse.phabricator-community.org/t/numerical-characters-are-stripped-from-diffusion-git-repository-name-in-the-uri/ Digits are often considered reasonable characters. Test Plan: Looked at an ascii table. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, Sam2304, epriestley Differential Revision: https://secure.phabricator.com/D19447 --- src/applications/repository/storage/PhabricatorRepository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 0a09503ee9..8889040ddf 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -364,7 +364,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO if (!strlen($name)) { $name = $this->getName(); $name = phutil_utf8_strtolower($name); - $name = preg_replace('@[/ -:<>]+@', '-', $name); + $name = preg_replace('@[ -/:->]+@', '-', $name); $name = trim($name, '-'); if (!strlen($name)) { $name = $this->getCallsign(); From 26d0862f4f6fff3a38e6f7d6f070c1c16f203db7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 10 May 2018 09:43:23 -0700 Subject: [PATCH 10/10] Apply the new patch byte size limit to mail patch generation in Differential Summary: Ref T13137. See PHI592. Depends on D19444. Apply a limit up front to stop patches which are way too big (e.g., 600MB of videos) from generating in the first place. Test Plan: - Configured inline patches in git format. - Created a normal revision, got an inline git patch. - Created a revision with a 10MB video file, got no inline patch. - (Added a bunch of debugging stuff to make sure the internal pathway was working.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13137 Differential Revision: https://secure.phabricator.com/D19445 --- .../editor/DifferentialTransactionEditor.php | 14 ++++++++++---- .../render/DifferentialRawDiffRenderer.php | 17 +++++++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3271248ce8..64ee1d5e81 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -682,8 +682,13 @@ final class DifferentialTransactionEditor if ($config_inline || $config_attach) { $body_limit = PhabricatorEnv::getEnvConfig('metamta.email-body-limit'); - $patch = $this->buildPatchForMail($diff); - if ($config_inline) { + try { + $patch = $this->buildPatchForMail($diff, $body_limit); + } catch (ArcanistDiffByteSizeException $ex) { + $patch = null; + } + + if (($patch !== null) && $config_inline) { $lines = substr_count($patch, "\n"); $bytes = strlen($patch); @@ -706,7 +711,7 @@ final class DifferentialTransactionEditor } } - if ($config_attach) { + if (($patch !== null) && $config_attach) { // See T12033, T11767, and PHI55. This is a crude fix to stop the // major concrete problems that lackluster email size limits cause. if (strlen($patch) < $body_limit) { @@ -1411,13 +1416,14 @@ final class DifferentialTransactionEditor array('style' => 'font-family: monospace;'), $patch); } - private function buildPatchForMail(DifferentialDiff $diff) { + private function buildPatchForMail(DifferentialDiff $diff, $byte_limit) { $format = PhabricatorEnv::getEnvConfig('metamta.differential.patch-format'); return id(new DifferentialRawDiffRenderer()) ->setViewer($this->getActor()) ->setFormat($format) ->setChangesets($diff->getChangesets()) + ->setByteLimit($byte_limit) ->buildPatch(); } diff --git a/src/applications/differential/render/DifferentialRawDiffRenderer.php b/src/applications/differential/render/DifferentialRawDiffRenderer.php index 0b826e9ba8..4f25646e4a 100644 --- a/src/applications/differential/render/DifferentialRawDiffRenderer.php +++ b/src/applications/differential/render/DifferentialRawDiffRenderer.php @@ -5,6 +5,7 @@ final class DifferentialRawDiffRenderer extends Phobject { private $changesets; private $format = 'unified'; private $viewer; + private $byteLimit; public function setFormat($format) { $this->format = $format; @@ -35,6 +36,15 @@ final class DifferentialRawDiffRenderer extends Phobject { return $this->viewer; } + public function setByteLimit($byte_limit) { + $this->byteLimit = $byte_limit; + return $this; + } + + public function getByteLimit() { + return $this->byteLimit; + } + public function buildPatch() { $diff = new DifferentialDiff(); $diff->attachChangesets($this->getChangesets()); @@ -52,15 +62,18 @@ final class DifferentialRawDiffRenderer extends Phobject { $bundle = ArcanistBundle::newFromChanges($changes); $bundle->setLoadFileDataCallback(array($loader, 'loadFileData')); + $byte_limit = $this->getByteLimit(); + if ($byte_limit) { + $bundle->setByteLimit($byte_limit); + } + $format = $this->getFormat(); switch ($format) { case 'git': return $bundle->toGitPatch(); - break; case 'unified': default: return $bundle->toUnifiedDiff(); - break; } } }