From 0e645b8f11d2ff3430c3f04cf01be4c30e803f01 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 14 Oct 2017 07:14:31 -0700 Subject: [PATCH 01/10] Disconnect rate limits in the PhabricatorStartup shutdown handler This makes counts more accurate, particularly for connection limits. --- support/startup/PhabricatorStartup.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php index 41164fbd84..1911a46b8a 100644 --- a/support/startup/PhabricatorStartup.php +++ b/support/startup/PhabricatorStartup.php @@ -150,6 +150,11 @@ final class PhabricatorStartup { * @task hook */ public static function didShutdown() { + // Disconnect any active rate limits before we shut down. If we don't do + // this, requests which exit early will lock a slot in any active + // connection limits, and won't count for rate limits. + self::disconnectRateLimits(array()); + $event = error_get_last(); if (!$event) { @@ -730,6 +735,10 @@ final class PhabricatorStartup { public static function disconnectRateLimits(array $request_state) { $limits = self::$limits; + // Remove all limits before disconnecting them so this works properly if + // it runs twice. (We run this automatically as a shutdown handler.) + self::$limits = array(); + foreach ($limits as $limit) { $limit->didDisconnect($request_state); } From 819b833607a6f9f05c60390a99cc1c206fde93e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 14 Oct 2017 07:58:23 -0700 Subject: [PATCH 02/10] Tweak rate limiting point counts for omnipotent users Summary: Ref T13008. We haven't hit any issues with this, but I can imagine we might in the future. When one host makes an intracluster request to another host, the `$viewer` ends up as the omnipotent viewer. This viewer isn't logged in, so they'll currently accumulate rate limit points at a high rate. Instead, don't give them any points. These requests are always legitimate, and if they originated from a user request, that request should be the one getting rate limited. Test Plan: Browsed around. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13008 Differential Revision: https://secure.phabricator.com/D18708 --- support/startup/PhabricatorClientRateLimit.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/support/startup/PhabricatorClientRateLimit.php b/support/startup/PhabricatorClientRateLimit.php index 85a6def878..89a273e3bf 100644 --- a/support/startup/PhabricatorClientRateLimit.php +++ b/support/startup/PhabricatorClientRateLimit.php @@ -35,7 +35,15 @@ final class PhabricatorClientRateLimit // If the user was logged in, let them make more requests. if (isset($request_state['viewer'])) { $viewer = $request_state['viewer']; - if ($viewer->isLoggedIn()) { + if ($viewer->isOmnipotent()) { + // If the viewer was omnipotent, this was an intracluster request or + // some other kind of special request, so don't give it any points + // toward rate limiting. + $score = 0; + } else if ($viewer->isLoggedIn()) { + // If the viewer was logged in, give them fewer points than if they + // were logged out, since this traffic is much more likely to be + // legitimate. $score = 0.25; } } From 63d1230ade0f610729edc1eee99bd515213c1f31 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Oct 2017 13:58:27 -0700 Subject: [PATCH 03/10] Parameterize the common ngrams threshold Summary: Ref T13000. Since other changes have generally made the ngrams table manageable, I'm not planning to enable common ngrams by default at this time. Instead, make the threshold configurable with "--threshold" so we can guide installs through tuning this if they want (e.g. PHI110), and tune hosted instances. (This might eventually become automatic, but just smoothing this bit off for now feels reasonable to me.) Test Plan: Ran with `--reset`, and with various invalid and valid `--threshold` arguments. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13000 Differential Revision: https://secure.phabricator.com/D18710 --- ...bricatorSearchManagementNgramsWorkflow.php | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php index 4b983fe0f9..ef0d73fa3d 100644 --- a/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php @@ -16,19 +16,49 @@ final class PhabricatorSearchManagementNgramsWorkflow 'name' => 'reset', 'help' => pht('Reset all common ngram records.'), ), + array( + 'name' => 'threshold', + 'param' => 'threshold', + 'help' => pht( + 'Prune ngrams present in more than this fraction of '. + 'documents.'), + ), )); } public function execute(PhutilArgumentParser $args) { + $min_documents = 4096; + $is_reset = $args->getArg('reset'); + $threshold = $args->getArg('threshold'); + + if ($is_reset && $threshold !== null) { + throw new PhutilArgumentUsageException( + pht('Specify either --reset or --threshold, not both.')); + } + + if (!$is_reset && $threshold === null) { + throw new PhutilArgumentUsageException( + pht('Specify either --reset or --threshold.')); + } + + if (!$is_reset) { + if (!is_numeric($threshold)) { + throw new PhutilArgumentUsageException( + pht('Specify a numeric threshold between 0 and 1.')); + } + + $threshold = (double)$threshold; + if ($threshold <= 0 || $threshold >= 1) { + throw new PhutilArgumentUsageException( + pht('Threshold must be greater than 0.0 and less than 1.0.')); + } + } $all_objects = id(new PhutilClassMapQuery()) ->setAncestorClass('PhabricatorFerretInterface') ->execute(); - $min_documents = 4096; - $threshold = 0.15; - foreach ($all_objects as $object) { $engine = $object->newFerretEngine(); $conn = $object->establishConnection('w'); From d36f98a15a6aab7cc403db0311f020174f9aa307 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Oct 2017 14:29:05 -0700 Subject: [PATCH 04/10] Clarify acceptable values for `--threshold` in `search ngrams` Summary: See D18710. Test Plan: o_O Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18712 --- .../management/PhabricatorSearchManagementNgramsWorkflow.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php index ef0d73fa3d..1a1124b4d8 100644 --- a/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php @@ -21,7 +21,7 @@ final class PhabricatorSearchManagementNgramsWorkflow 'param' => 'threshold', 'help' => pht( 'Prune ngrams present in more than this fraction of '. - 'documents.'), + 'documents. Provide a value between 0.0 and 1.0.'), ), )); } From bfabe49c5ae88b62556238443e536c986a78d1f6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 Oct 2017 13:35:02 -0700 Subject: [PATCH 05/10] Start revisions in "Draft" if prototypes are enabled Summary: Ref T2543. This is a less ambitious version of the rule in D18628, which I backed off from, since I think this probably still has a fair number of loose ends to tie up. Test Plan: Created a revision locally. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18713 --- .../differential/storage/DifferentialRevision.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index e29db36b14..d14f69f563 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -67,13 +67,19 @@ final class DifferentialRevision extends DifferentialDAO $view_policy = $app->getPolicy( DifferentialDefaultViewCapability::CAPABILITY); + if (PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) { + $initial_state = DifferentialRevisionStatus::DRAFT; + } else { + $initial_state = DifferentialRevisionStatus::NEEDS_REVIEW; + } + return id(new DifferentialRevision()) ->setViewPolicy($view_policy) ->setAuthorPHID($actor->getPHID()) ->attachRepository(null) ->attachActiveDiff(null) ->attachReviewers(array()) - ->setModernRevisionStatus(DifferentialRevisionStatus::NEEDS_REVIEW); + ->setModernRevisionStatus($initial_state); } protected function getConfiguration() { From 1755ec242989fabf1c27ab71f5364ce9be68a74a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 Oct 2017 14:09:25 -0700 Subject: [PATCH 06/10] Show more detailed hints about draft revisions in the UI Summary: Ref T2543. When revisions are in the draft state, tell the user what we're waiting for or why they aren't moving forward. Test Plan: {F5228840} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18714 --- src/__phutil_library_map__.php | 2 + .../customfield/DifferentialDraftField.php | 90 +++++++++++++++++++ .../editor/DifferentialTransactionEditor.php | 48 +--------- .../storage/DifferentialRevision.php | 52 +++++++++++ 4 files changed, 145 insertions(+), 47 deletions(-) create mode 100644 src/applications/differential/customfield/DifferentialDraftField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a650e33fcf..93efcc65e1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -444,6 +444,7 @@ phutil_register_library_map(array( 'DifferentialDiffTransactionQuery' => 'applications/differential/query/DifferentialDiffTransactionQuery.php', 'DifferentialDiffViewController' => 'applications/differential/controller/DifferentialDiffViewController.php', 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php', + 'DifferentialDraftField' => 'applications/differential/customfield/DifferentialDraftField.php', 'DifferentialExactUserFunctionDatasource' => 'applications/differential/typeahead/DifferentialExactUserFunctionDatasource.php', 'DifferentialFieldParseException' => 'applications/differential/exception/DifferentialFieldParseException.php', 'DifferentialFieldValidationException' => 'applications/differential/exception/DifferentialFieldValidationException.php', @@ -5451,6 +5452,7 @@ phutil_register_library_map(array( 'DifferentialDiffTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'DifferentialDiffViewController' => 'DifferentialController', 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher', + 'DifferentialDraftField' => 'DifferentialCoreCustomField', 'DifferentialExactUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DifferentialFieldParseException' => 'Exception', 'DifferentialFieldValidationException' => 'Exception', diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php new file mode 100644 index 0000000000..e37d622a33 --- /dev/null +++ b/src/applications/differential/customfield/DifferentialDraftField.php @@ -0,0 +1,90 @@ +getViewer(); + $revision = $this->getObject(); + + if (!$revision->isDraft()) { + return array(); + } + + $warnings = array(); + + $blocking_map = array( + HarbormasterBuildStatus::STATUS_FAILED, + HarbormasterBuildStatus::STATUS_ABORTED, + HarbormasterBuildStatus::STATUS_ERROR, + HarbormasterBuildStatus::STATUS_PAUSED, + HarbormasterBuildStatus::STATUS_DEADLOCKED, + ); + $blocking_map = array_fuse($blocking_map); + + $builds = $revision->loadActiveBuilds($viewer); + + $waiting = array(); + $blocking = array(); + foreach ($builds as $build) { + if (isset($blocking_map[$build->getBuildStatus()])) { + $blocking[] = $build; + } else { + $waiting[] = $build; + } + } + + $blocking_list = $viewer->renderHandleList(mpull($blocking, 'getPHID')) + ->setAsInline(true); + $waiting_list = $viewer->renderHandleList(mpull($waiting, 'getPHID')) + ->setAsInline(true); + + if ($blocking) { + $warnings[] = pht( + 'This draft revision will not be submitted for review because %s '. + 'build(s) failed: %s.', + phutil_count($blocking), + $blocking_list); + $warnings[] = pht( + 'Fix build failures and update the revision.'); + } else if ($waiting) { + $warnings[] = pht( + 'This draft revision will be sent for review once %s '. + 'build(s) pass: %s.', + phutil_count($waiting), + $waiting_list); + } else { + $warnings[] = pht( + 'This is a draft revision that has not yet been submitted for '. + 'review.'); + } + + return $warnings; + } + +} diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 253d49c1ec..7c9bb01b07 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1570,58 +1570,12 @@ final class DifferentialTransactionEditor private function hasActiveBuilds($object) { $viewer = $this->requireActor(); - $diff = $object->getActiveDiff(); - $buildables = id(new HarbormasterBuildableQuery()) - ->setViewer($viewer) - ->withContainerPHIDs(array($object->getPHID())) - ->withBuildablePHIDs(array($diff->getPHID())) - ->withManualBuildables(false) - ->execute(); - if (!$buildables) { - return false; - } - - $builds = id(new HarbormasterBuildQuery()) - ->setViewer($viewer) - ->withBuildablePHIDs(mpull($buildables, 'getPHID')) - ->withBuildStatuses( - array( - HarbormasterBuildStatus::STATUS_INACTIVE, - HarbormasterBuildStatus::STATUS_PENDING, - HarbormasterBuildStatus::STATUS_BUILDING, - HarbormasterBuildStatus::STATUS_FAILED, - HarbormasterBuildStatus::STATUS_ABORTED, - HarbormasterBuildStatus::STATUS_ERROR, - HarbormasterBuildStatus::STATUS_PAUSED, - HarbormasterBuildStatus::STATUS_DEADLOCKED, - )) - ->needBuildTargets(true) - ->execute(); + $builds = $object->loadActiveBuilds($viewer); if (!$builds) { return false; } - $active = array(); - foreach ($builds as $key => $build) { - foreach ($build->getBuildTargets() as $target) { - if ($target->isAutotarget()) { - // Ignore autotargets when looking for active of failed builds. If - // local tests fail and you continue anyway, you don't need to - // double-confirm them. - continue; - } - - // This build has at least one real target that's doing something. - $active[$key] = $build; - break; - } - } - - if (!$active) { - return false; - } - return true; } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index d14f69f563..76c7c4da95 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -708,6 +708,58 @@ final class DifferentialRevision extends DifferentialDAO return false; } + public function loadActiveBuilds(PhabricatorUser $viewer) { + $diff = $this->getActiveDiff(); + + $buildables = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withContainerPHIDs(array($this->getPHID())) + ->withBuildablePHIDs(array($diff->getPHID())) + ->withManualBuildables(false) + ->execute(); + if (!$buildables) { + return array(); + } + + $builds = id(new HarbormasterBuildQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs(mpull($buildables, 'getPHID')) + ->withBuildStatuses( + array( + HarbormasterBuildStatus::STATUS_INACTIVE, + HarbormasterBuildStatus::STATUS_PENDING, + HarbormasterBuildStatus::STATUS_BUILDING, + HarbormasterBuildStatus::STATUS_FAILED, + HarbormasterBuildStatus::STATUS_ABORTED, + HarbormasterBuildStatus::STATUS_ERROR, + HarbormasterBuildStatus::STATUS_PAUSED, + HarbormasterBuildStatus::STATUS_DEADLOCKED, + )) + ->needBuildTargets(true) + ->execute(); + if (!$builds) { + return array(); + } + + $active = array(); + foreach ($builds as $key => $build) { + foreach ($build->getBuildTargets() as $target) { + if ($target->isAutotarget()) { + // Ignore autotargets when looking for active of failed builds. If + // local tests fail and you continue anyway, you don't need to + // double-confirm them. + continue; + } + + // This build has at least one real target that's doing something. + $active[$key] = $build; + break; + } + } + + return $active; + } + /* -( HarbormasterBuildableInterface )------------------------------------- */ From 01b1854fb45c3fda896362bc565ba09300e59ccd Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 07:40:11 -0700 Subject: [PATCH 07/10] Fix an issue with attempting to index comments on packages Summary: See rPcd14194a329788d5fff6365bcade278fd18f3612 for a similar change. Implement `getApplicationTransactionCommentObject()` to return `null` explicitly. Test Plan: Ran `bin/search index --type ownerspackage`, got indexing after change. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18715 --- .../owners/storage/PhabricatorOwnersPackageTransaction.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php index 66e15b634a..1dfc944f63 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php @@ -15,4 +15,8 @@ final class PhabricatorOwnersPackageTransaction return 'PhabricatorOwnersPackageTransactionType'; } + public function getApplicationTransactionCommentObject() { + return null; + } + } From cbcab60fbb95e8ba6e00860e71b7e703e71f3d71 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 10:47:24 -0700 Subject: [PATCH 08/10] Remove obsolete instructions from information on prototype applications Summary: Most of this document is no longer relevant, since we're happy to work on prototypes if you're paying us and no longer have any meaningful free support. Test Plan: Read document. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18719 --- src/docs/user/userguide/prototypes.diviner | 33 ---------------------- 1 file changed, 33 deletions(-) diff --git a/src/docs/user/userguide/prototypes.diviner b/src/docs/user/userguide/prototypes.diviner index 7352502f99..c84bad1178 100644 --- a/src/docs/user/userguide/prototypes.diviner +++ b/src/docs/user/userguide/prototypes.diviner @@ -9,8 +9,6 @@ Overview Phabricator includes //prototype applications//, which are applications in an early stage of development. -IMPORTANT: The upstream does not offer support for these applications. - When we begin working on a new application, we usually implement it as a prototype first. This allows us to get a better sense of how the application might work and integrate with other applications, and what technical and product @@ -32,34 +30,3 @@ on hold indefinitely if we're less excited about it after we begin building it. If you're interested in previewing upcoming applications, you can use the `phabricator.show-prototypes` configuration setting to enable prototypes. - -Feedback on Prototypes -====================== - -We're usually interested in this sort of feedback on prototypes: - - - {icon check, color=green} **Use Cases**: If we're building something that - you think you'd use, we'd love to hear about your use cases for it. This can - help us figure out what features to add and how users may think about, use, - and integrate the application. - - {icon check, color=green} **General Interest**: Is an application something - you're looking forward to? Knowing which applications users are interested - in can help us set priorities. - -We're usually **not** interested in this sort of feedback on prototypes: - - - {icon times, color=red} **Support Requests**: We do not support these - applications. Use them at your own risk, or wait for them to leave the - prototype phase. - - {icon times, color=red} **Bug Reports**: We know these applications don't - work well yet, and usually know about most of the open bugs. Even if we - don't, whatever isn't working yet may change completely before the - application leaves the prototype phase. - - {icon times, color=red} **Contributions / Pull Requests**: These - applications are usually in too early a state to accept contributions. Let - us know about your use case, but wait for release to send code. - -Overall, using prototypes makes it easier for us to explore and develop -application ideas, and to share a preview of what's coming in the future with -users, but prototypes are not yet full applications and we do not provide -support until applications leave the prototype phase. From 65f13b156f5ecdbf689cce16f2bc7b9680818789 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 10:15:07 -0700 Subject: [PATCH 09/10] Improve "refengine" performance for testing large numbers of Mercurial branches Summary: See PHI158. In the RefEngine, we test if any old branch positions have been removed from the repository. This is uncommon (but not impossible) in Mercurial, and corresponds to users deleting branches in Git. Currently, we end up running `hg log` for each position, in parallel. Because of Python's large startup overhead, this can be resource intensive for repositories with a large number of branches. We have to do this in the general case because the caller may be asking us to resolve `tip`, `newfeature`, `tip~3`, `9`, etc. However, in the specific case where the refs are 40-digit hashes, we can bulk resolve them if they exist, like this: ``` hg log ... --rev (abcd or def0 or ab12 or ...) ``` In the general case, we could probably do less of this than we currently do (instead of testing all old heads, we could prune the list by removing commits which we know are still pointed to by current heads) but that's a slightly more involved change and the effect here is already dramatic. Test Plan: Verified that CPU usage drops from ~110s -> ~0.9s: Before: ``` epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss Updating refs in "nss"... Done. real 0m14.676s user 1m24.714s sys 0m21.645s ``` After: ``` epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss Updating refs in "nss"... Done. real 0m0.861s user 0m0.882s sys 0m0.213s ``` - Manually resolved `blue`, `tip`, `9`, etc., got expected results. - Tried to resolve invalid hashes, got expected result (no resolution). Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18717 --- .../DiffusionLowLevelResolveRefsQuery.php | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php index b649ca65a8..f9e9f74774 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php @@ -256,6 +256,66 @@ final class DiffusionLowLevelResolveRefsQuery return $results; } + // If some of the refs look like hashes, try to bulk resolve them. This + // workflow happens via RefEngine and bulk resolution is dramatically + // faster than individual resolution. See PHI158. + + $hashlike = array(); + foreach ($unresolved as $key => $ref) { + if (preg_match('/^[a-f0-9]{40}\z/', $ref)) { + $hashlike[$key] = $ref; + } + } + + if (count($hashlike) > 1) { + $hashlike_map = array(); + + $hashlike_groups = array_chunk($hashlike, 64, true); + foreach ($hashlike_groups as $hashlike_group) { + $hashlike_arg = array(); + foreach ($hashlike_group as $hashlike_ref) { + $hashlike_arg[] = hgsprintf('%s', $hashlike_ref); + } + $hashlike_arg = '('.implode(' or ', $hashlike_arg).')'; + + list($err, $refs) = $repository->execLocalCommand( + 'log --template=%s --rev %s', + '{node}\n', + $hashlike_arg); + if ($err) { + // NOTE: If any ref fails to resolve, Mercurial will exit with an + // error. We just give up on the whole group and resolve it + // individually below. In theory, we could split it into subgroups + // but the pathway where this bulk resolution matters rarely tries + // to resolve missing refs (see PHI158). + continue; + } + + $refs = phutil_split_lines($refs, false); + + foreach ($refs as $ref) { + $hashlike_map[$ref] = true; + } + } + + foreach ($unresolved as $key => $ref) { + if (!isset($hashlike_map[$ref])) { + continue; + } + + $results[$ref][] = array( + 'type' => 'commit', + 'identifier' => $ref, + ); + + unset($unresolved[$key]); + } + } + + if (!$unresolved) { + return $results; + } + // If we still have unresolved refs (which might be things like "tip"), // try to resolve them individually. From 672247eff30b65210e2f0f00b009f8c6c3f4b64f Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 10:43:33 -0700 Subject: [PATCH 10/10] Add aural "+" and "-" hints to unified diffs for users who use screenreaders Summary: See PHI160 for discussion. Test Plan: With `?__aural__=1`, saw aural hints: {F5229986} Without, saw normal visual diff. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18718 --- .../DifferentialChangesetOneUpRenderer.php | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 7a694cfd98..586680d1b8 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -41,6 +41,20 @@ final class DifferentialChangesetOneUpRenderer $column_width = 4; + $aural_minus = javelin_tag( + 'span', + array( + 'aural' => true, + ), + '- '); + + $aural_plus = javelin_tag( + 'span', + array( + 'aural' => true, + ), + '+ '); + $out = array(); foreach ($primitives as $k => $p) { $type = $p['type']; @@ -55,8 +69,10 @@ final class DifferentialChangesetOneUpRenderer if ($is_old) { if ($p['htype']) { $class = 'left old'; + $aural = $aural_minus; } else { $class = 'left'; + $aural = null; } if ($type == 'old-file') { @@ -79,14 +95,20 @@ final class DifferentialChangesetOneUpRenderer ), $line); + $render = $p['render']; + if ($aural !== null) { + $render = array($aural, $render); + } + $cells[] = phutil_tag('th', array('class' => $class)); $cells[] = $no_copy; - $cells[] = phutil_tag('td', array('class' => $class), $p['render']); + $cells[] = phutil_tag('td', array('class' => $class), $render); $cells[] = $no_coverage; } else { if ($p['htype']) { $class = 'right new'; $cells[] = phutil_tag('th', array('class' => $class)); + $aural = $aural_plus; } else { $class = 'right'; if ($left_prefix) { @@ -98,6 +120,7 @@ final class DifferentialChangesetOneUpRenderer $oline = $p['oline']; $cells[] = phutil_tag('th', array('id' => $left_id), $oline); + $aural = null; } if ($type == 'new-file') { @@ -120,8 +143,13 @@ final class DifferentialChangesetOneUpRenderer ), $line); + $render = $p['render']; + if ($aural !== null) { + $render = array($aural, $render); + } + $cells[] = $no_copy; - $cells[] = phutil_tag('td', array('class' => $class), $p['render']); + $cells[] = phutil_tag('td', array('class' => $class), $render); $cells[] = $no_coverage; }