From c9a0d68340ba131d8c8c0b7180293607a19da625 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Dec 2017 11:33:15 -0800 Subject: [PATCH 1/7] Allow Herald rules to add comments Summary: See PHI242. All use cases for this that I know of are pretty hacky, but they don't seem perilous, and it's easier than webhooks. See P1895, T10183, and T9853 for me previously refusing to implement this since all those use cases were also pretty bad. Test Plan: - Wrote a rule to add comments, saw it add comments. - Reviewed summary, re-edited rule, reviewed transcript to check that all the strings worked OK. - Wrote a new rule for a non-commentable object (a blog) to make sure I wasn't offered the "Add a comment" action. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18823 --- resources/celerity/map.php | 26 +++--- src/__phutil_library_map__.php | 4 + .../herald/action/HeraldAction.php | 3 + .../herald/action/HeraldCommentAction.php | 79 +++++++++++++++++++ .../herald/value/HeraldFieldValue.php | 1 + .../herald/value/HeraldRemarkupFieldValue.php | 22 ++++++ .../rsrc/css/application/herald/herald.css | 6 ++ .../js/application/herald/HeraldRuleEditor.js | 5 ++ 8 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 src/applications/herald/action/HeraldCommentAction.php create mode 100644 src/applications/herald/value/HeraldRemarkupFieldValue.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0ea66bec26..d937b91e4f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -80,7 +80,7 @@ return array( 'rsrc/css/application/flag/flag.css' => 'bba8f811', 'rsrc/css/application/harbormaster/harbormaster.css' => 'f491c9f4', 'rsrc/css/application/herald/herald-test.css' => 'a52e323e', - 'rsrc/css/application/herald/herald.css' => 'dc31f6e9', + 'rsrc/css/application/herald/herald.css' => 'cd8d0134', 'rsrc/css/application/maniphest/batch-editor.css' => 'b0f0b6d5', 'rsrc/css/application/maniphest/report.css' => '9b9580b7', 'rsrc/css/application/maniphest/task-edit.css' => 'fda62a9b', @@ -416,7 +416,7 @@ return array( 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', - 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'd6a7e717', + 'rsrc/js/application/herald/HeraldRuleEditor.js' => '2dff5579', 'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', 'rsrc/js/application/maniphest/behavior-batch-editor.js' => '782ab6e7', @@ -578,8 +578,8 @@ return array( 'font-lato' => 'c7ccd872', 'global-drag-and-drop-css' => 'b556a948', 'harbormaster-css' => 'f491c9f4', - 'herald-css' => 'dc31f6e9', - 'herald-rule-editor' => 'd6a7e717', + 'herald-css' => 'cd8d0134', + 'herald-rule-editor' => '2dff5579', 'herald-test-css' => 'a52e323e', 'inline-comment-summary-css' => 'f23d4e8f', 'javelin-aphlict' => 'e1d4b11a', @@ -1106,6 +1106,15 @@ return array( 'javelin-install', 'javelin-event', ), + '2dff5579' => array( + 'multirow-row-manager', + 'javelin-install', + 'javelin-util', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-json', + 'phabricator-prefab', + ), '2ee659ce' => array( 'javelin-install', ), @@ -2001,15 +2010,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - 'd6a7e717' => array( - 'multirow-row-manager', - 'javelin-install', - 'javelin-util', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-json', - 'phabricator-prefab', - ), 'd7a74243' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 35be621604..924e81a5ff 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1337,6 +1337,7 @@ phutil_register_library_map(array( 'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php', 'HeraldBasicFieldGroup' => 'applications/herald/field/HeraldBasicFieldGroup.php', 'HeraldBuildableState' => 'applications/herald/state/HeraldBuildableState.php', + 'HeraldCommentAction' => 'applications/herald/action/HeraldCommentAction.php', 'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php', 'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php', 'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php', @@ -1378,6 +1379,7 @@ phutil_register_library_map(array( 'HeraldProjectsField' => 'applications/project/herald/HeraldProjectsField.php', 'HeraldRecursiveConditionsException' => 'applications/herald/engine/exception/HeraldRecursiveConditionsException.php', 'HeraldRelatedFieldGroup' => 'applications/herald/field/HeraldRelatedFieldGroup.php', + 'HeraldRemarkupFieldValue' => 'applications/herald/value/HeraldRemarkupFieldValue.php', 'HeraldRemarkupRule' => 'applications/herald/remarkup/HeraldRemarkupRule.php', 'HeraldRepetitionPolicyConfig' => 'applications/herald/config/HeraldRepetitionPolicyConfig.php', 'HeraldRule' => 'applications/herald/storage/HeraldRule.php', @@ -6488,6 +6490,7 @@ phutil_register_library_map(array( 'HeraldApplyTranscript' => 'Phobject', 'HeraldBasicFieldGroup' => 'HeraldFieldGroup', 'HeraldBuildableState' => 'HeraldState', + 'HeraldCommentAction' => 'HeraldAction', 'HeraldCommitAdapter' => array( 'HeraldAdapter', 'HarbormasterBuildableAdapterInterface', @@ -6535,6 +6538,7 @@ phutil_register_library_map(array( 'HeraldProjectsField' => 'HeraldField', 'HeraldRecursiveConditionsException' => 'Exception', 'HeraldRelatedFieldGroup' => 'HeraldFieldGroup', + 'HeraldRemarkupFieldValue' => 'HeraldFieldValue', 'HeraldRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'HeraldRepetitionPolicyConfig' => 'Phobject', 'HeraldRule' => array( diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index a2d589f9f2..04884a94d4 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -9,6 +9,7 @@ abstract class HeraldAction extends Phobject { const STANDARD_NONE = 'standard.none'; const STANDARD_PHID_LIST = 'standard.phid.list'; const STANDARD_TEXT = 'standard.text'; + const STANDARD_REMARKUP = 'standard.remarkup'; const DO_STANDARD_EMPTY = 'do.standard.empty'; const DO_STANDARD_NO_EFFECT = 'do.standard.no-effect'; @@ -60,6 +61,8 @@ abstract class HeraldAction extends Phobject { return new HeraldEmptyFieldValue(); case self::STANDARD_TEXT: return new HeraldTextFieldValue(); + case self::STANDARD_REMARKUP: + return new HeraldRemarkupFieldValue(); case self::STANDARD_PHID_LIST: $tokenizer = id(new HeraldTokenizerFieldValue()) ->setKey($this->getHeraldActionName()) diff --git a/src/applications/herald/action/HeraldCommentAction.php b/src/applications/herald/action/HeraldCommentAction.php new file mode 100644 index 0000000000..fa52ba1f5f --- /dev/null +++ b/src/applications/herald/action/HeraldCommentAction.php @@ -0,0 +1,79 @@ +getApplicationTransactionTemplate(); + try { + $comment = $xaction->getApplicationTransactionCommentObject(); + if (!$comment) { + return false; + } + } catch (PhutilMethodNotImplementedException $ex) { + return false; + } + + return true; + } + + public function supportsRuleType($rule_type) { + return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); + } + + public function applyEffect($object, HeraldEffect $effect) { + $adapter = $this->getAdapter(); + $comment_text = $effect->getTarget(); + + $xaction = $adapter->newTransaction() + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT); + + $comment = $xaction->getApplicationTransactionCommentObject() + ->setContent($comment_text); + + $xaction->attachComment($comment); + + $adapter->queueTransaction($xaction); + + $this->logEffect(self::DO_COMMENT, $comment_text); + } + + public function getHeraldActionStandardType() { + return self::STANDARD_REMARKUP; + } + + protected function getActionEffectMap() { + return array( + self::DO_COMMENT => array( + 'icon' => 'fa-comment', + 'color' => 'blue', + 'name' => pht('Added Comment'), + ), + ); + } + + public function renderActionDescription($value) { + $summary = PhabricatorMarkupEngine::summarize($value); + return pht('Add comment: %s', $summary); + } + + protected function renderActionEffectDescription($type, $data) { + $summary = PhabricatorMarkupEngine::summarize($data); + return pht('Added a comment: %s', $summary); + } + +} diff --git a/src/applications/herald/value/HeraldFieldValue.php b/src/applications/herald/value/HeraldFieldValue.php index b3f35968e2..c88776f53a 100644 --- a/src/applications/herald/value/HeraldFieldValue.php +++ b/src/applications/herald/value/HeraldFieldValue.php @@ -8,6 +8,7 @@ abstract class HeraldFieldValue extends Phobject { const CONTROL_TEXT = 'herald.control.text'; const CONTROL_SELECT = 'herald.control.select'; const CONTROL_TOKENIZER = 'herald.control.tokenizer'; + const CONTROL_REMARKUP = 'herald.control.remarkup'; abstract public function getFieldValueKey(); abstract public function getControlType(); diff --git a/src/applications/herald/value/HeraldRemarkupFieldValue.php b/src/applications/herald/value/HeraldRemarkupFieldValue.php new file mode 100644 index 0000000000..4740ec7342 --- /dev/null +++ b/src/applications/herald/value/HeraldRemarkupFieldValue.php @@ -0,0 +1,22 @@ + Date: Wed, 13 Dec 2017 05:35:56 -0800 Subject: [PATCH 2/7] Move the Git LFS gate to dedicated (non-prototype) config Summary: See PHI131. Ref T7789. Although this probably isn't 100% complete, there don't seem to be any actual, known, practical blocking issues remaining (everything is either heresay or not reproducible). Test Plan: Tried to push LFS locally, got blocked with a helpful message. Enabled setting, tried to push LFS locally, got a successful push. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T7789 Differential Revision: https://secure.phabricator.com/D18825 --- .../config/PhabricatorDiffusionConfigOptions.php | 15 ++++++++++++++- .../repository/storage/PhabricatorRepository.php | 3 +-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php index 50f761eabc..d15fb678d5 100644 --- a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php +++ b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php @@ -101,7 +101,7 @@ final class PhabricatorDiffusionConfigOptions ->setBoolOptions( array( pht('Allow HTTP Basic Auth'), - pht('Disable HTTP Basic Auth'), + pht('Disallow HTTP Basic Auth'), )) ->setSummary(pht('Enable HTTP Basic Auth for repositories.')) ->setDescription( @@ -115,6 +115,19 @@ final class PhabricatorDiffusionConfigOptions "barrier to attackers than SSH does.\n\n". "Consider using SSH for authenticated access to repositories ". "instead of HTTP.")), + $this->newOption('diffusion.allow-git-lfs', 'bool', false) + ->setBoolOptions( + array( + pht('Allow Git LFS'), + pht('Disallow Git LFS'), + )) + ->setLocked(true) + ->setSummary(pht('Allow Git Large File Storage (LFS).')) + ->setDescription( + pht( + 'Phabricator supports Git LFS, a Git extension for storing large '. + 'files alongside a repository. Activate this setting to allow '. + 'the extension to store file data in Phabricator.')), $this->newOption('diffusion.ssh-user', 'string', null) ->setLocked(true) ->setSummary(pht('Login username for SSH connections to repositories.')) diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 713ed92270..702bb42780 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1627,8 +1627,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return false; } - // TODO: Unprototype this feature. - if (!PhabricatorEnv::getEnvConfig('phabricator.show-prototypes')) { + if (!PhabricatorEnv::getEnvConfig('diffusion.allow-git-lfs')) { return false; } From 8e416474c06398857a990bb0bd6d4725ab17f100 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Dec 2017 05:52:39 -0800 Subject: [PATCH 3/7] Add a Herald pre-commit field for detecting LFS usage Summary: Depends on D18825. Ref T7789. See PHI131. Allows installs to selectively disable LFS by adding Herald rules to block commits that use LFS. Test Plan: - Wrote an LFS rule ("When commit uses git lfs, block commit"). - Pushed an LFS commit: rejected. - Pushed a non-lFS commit: success. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T7789 Differential Revision: https://secure.phabricator.com/D18827 --- src/__phutil_library_map__.php | 2 + ...iffusionPreCommitUsesGitLFSHeraldField.php | 41 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 src/applications/diffusion/herald/DiffusionPreCommitUsesGitLFSHeraldField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 924e81a5ff..f86b3a1d2d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -821,6 +821,7 @@ phutil_register_library_map(array( 'DiffusionPreCommitRefRepositoryHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefRepositoryHeraldField.php', 'DiffusionPreCommitRefRepositoryProjectsHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefRepositoryProjectsHeraldField.php', 'DiffusionPreCommitRefTypeHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefTypeHeraldField.php', + 'DiffusionPreCommitUsesGitLFSHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitUsesGitLFSHeraldField.php', 'DiffusionPullEventGarbageCollector' => 'applications/diffusion/garbagecollector/DiffusionPullEventGarbageCollector.php', 'DiffusionPushCapability' => 'applications/diffusion/capability/DiffusionPushCapability.php', 'DiffusionPushEventViewController' => 'applications/diffusion/controller/DiffusionPushEventViewController.php', @@ -5881,6 +5882,7 @@ phutil_register_library_map(array( 'DiffusionPreCommitRefRepositoryHeraldField' => 'DiffusionPreCommitRefHeraldField', 'DiffusionPreCommitRefRepositoryProjectsHeraldField' => 'DiffusionPreCommitRefHeraldField', 'DiffusionPreCommitRefTypeHeraldField' => 'DiffusionPreCommitRefHeraldField', + 'DiffusionPreCommitUsesGitLFSHeraldField' => 'DiffusionPreCommitContentHeraldField', 'DiffusionPullEventGarbageCollector' => 'PhabricatorGarbageCollector', 'DiffusionPushCapability' => 'PhabricatorPolicyCapability', 'DiffusionPushEventViewController' => 'DiffusionPushLogController', diff --git a/src/applications/diffusion/herald/DiffusionPreCommitUsesGitLFSHeraldField.php b/src/applications/diffusion/herald/DiffusionPreCommitUsesGitLFSHeraldField.php new file mode 100644 index 0000000000..3a34db777c --- /dev/null +++ b/src/applications/diffusion/herald/DiffusionPreCommitUsesGitLFSHeraldField.php @@ -0,0 +1,41 @@ +getAdapter()->getDiffContent('+'); + + // At the time of writing, all current Git LFS files begin with this + // line, verbatim: + // + // version https://git-lfs.github.com/spec/v1 + // + // ...but we don't try to match the specific version here, in the hopes + // that this might also detect future versions. + $pattern = '(^version\s*https://git-lfs.github.com/spec/)i'; + + foreach ($map as $path => $content) { + if (preg_match($pattern, $content)) { + return true; + } + } + + return false; + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_BOOL; + } + +} From 5295840a4cf9c1c75e317ace5597a254e3e6b27b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Dec 2017 06:02:09 -0800 Subject: [PATCH 4/7] Restore the "Download from Git LFS" UI button to Diffusion Summary: Depends on D18827. Ref T7789. See PHI204. See PHI131. This button got accidentally removed in Diffusion refactoring (`$data` is no longer used). Test Plan: {F5321459} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T7789 Differential Revision: https://secure.phabricator.com/D18828 --- .../diffusion/controller/DiffusionBrowseController.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 4985ac2bb7..77ec8e46ca 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -916,7 +916,8 @@ final class DiffusionBrowseController extends DiffusionController { ->setTag('a') ->setText($text) ->setHref($href) - ->setIcon($icon); + ->setIcon($icon) + ->setColor(PHUIButtonView::GREY); } private function buildDisplayRows( @@ -1924,7 +1925,7 @@ final class DiffusionBrowseController extends DiffusionController { try { $file = $this->loadGitLFSFile($ref); - $data = $this->renderGitLFSButton(); + $this->corpusButtons[] = $this->renderGitLFSButton(); } catch (Exception $ex) { $severity = PHUIInfoView::SEVERITY_ERROR; $messages[] = pht('The data for this file could not be loaded.'); From e1d6bad864d4faccba5a63261d2a2800a39fd377 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Dec 2017 06:28:11 -0800 Subject: [PATCH 5/7] Stop trying to assess the image dimensions of large files and file chunks Summary: Depends on D18828. Ref T7789. See . Currently, when you upload a large (>4MB) image, we may try to assess the dimensions for the image and for each individual chunk. At best, this is slow and not useful. At worst, it fatals or consumes a ton of memory and I/O we don't need to be using. Instead: - Don't try to assess dimensions for chunked files. - Don't try to assess dimensions for the chunks themselves. - Squelch errors for bad data, etc., that `gd` can't actually read, since we recover sensibly. Test Plan: - Created a 2048x2048 PNG in Photoshop using the "Random Noise" filter which weighs 8.5MB. - Uploaded it. - Before patch: got complaints in log about `imagecreatefromstring()` failing, although the actual upload went OK in my environment. - After patch: clean log, no attempt to detect the size of a big image. - Also uploaded a small image, got dimensions detected properly still. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T7789 Differential Revision: https://secure.phabricator.com/D18830 --- .../FileUploadChunkConduitAPIMethod.php | 1 + .../files/storage/PhabricatorFile.php | 31 +++++++++++++++++-- .../PhabricatorFileUploadSource.php | 1 + 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php b/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php index b4d1a23a3f..eab46c4992 100644 --- a/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php +++ b/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php @@ -64,6 +64,7 @@ final class FileUploadChunkConduitAPIMethod $params = array( 'name' => $file->getMonogram().'.chunk-'.$chunk->getID(), 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, + 'chunk' => true, ); if ($mime_type !== null) { diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index c87623c68c..d3dc5208d2 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -40,6 +40,7 @@ final class PhabricatorFile extends PhabricatorFileDAO const METADATA_PROFILE = 'profile'; const METADATA_STORAGE = 'storage'; const METADATA_INTEGRITY = 'integrity'; + const METADATA_CHUNK = 'chunk'; const STATUS_ACTIVE = 'active'; const STATUS_DELETED = 'deleted'; @@ -410,7 +411,7 @@ final class PhabricatorFile extends PhabricatorFileDAO try { $file->updateDimensions(false); } catch (Exception $ex) { - // Do nothing + // Do nothing. } $file->saveAndIndex(); @@ -1057,9 +1058,20 @@ final class PhabricatorFile extends PhabricatorFileDAO throw new Exception(pht('Cannot retrieve image information.')); } + if ($this->getIsChunk()) { + throw new Exception( + pht('Refusing to assess image dimensions of file chunk.')); + } + + $engine = $this->instantiateStorageEngine(); + if ($engine->isChunkEngine()) { + throw new Exception( + pht('Refusing to assess image dimensions of chunked file.')); + } + $data = $this->loadFileData(); - $img = imagecreatefromstring($data); + $img = @imagecreatefromstring($data); if ($img === false) { throw new Exception(pht('Error when decoding image.')); } @@ -1255,6 +1267,15 @@ final class PhabricatorFile extends PhabricatorFileDAO return $this; } + public function getIsChunk() { + return idx($this->metadata, self::METADATA_CHUNK); + } + + public function setIsChunk($value) { + $this->metadata[self::METADATA_CHUNK] = $value; + return $this; + } + public function setIntegrityHash($integrity_hash) { $this->metadata[self::METADATA_INTEGRITY] = $integrity_hash; return $this; @@ -1339,6 +1360,7 @@ final class PhabricatorFile extends PhabricatorFileDAO 'mime-type' => 'optional string', 'builtin' => 'optional string', 'storageEngines' => 'optional list', + 'chunk' => 'optional bool', )); $file_name = idx($params, 'name'); @@ -1416,6 +1438,11 @@ final class PhabricatorFile extends PhabricatorFileDAO $this->setMimeType($mime_type); } + $is_chunk = idx($params, 'chunk'); + if ($is_chunk) { + $this->setIsChunk(true); + } + return $this; } diff --git a/src/applications/files/uploadsource/PhabricatorFileUploadSource.php b/src/applications/files/uploadsource/PhabricatorFileUploadSource.php index e3242af329..bbd93a455a 100644 --- a/src/applications/files/uploadsource/PhabricatorFileUploadSource.php +++ b/src/applications/files/uploadsource/PhabricatorFileUploadSource.php @@ -189,6 +189,7 @@ abstract class PhabricatorFileUploadSource $params = array( 'name' => $file->getMonogram().'.chunk-'.$offset, 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, + 'chunk' => true, ); // If this isn't the initial chunk, provide a dummy MIME type so we do not From a1ad184ddd8cd7d788320e871c36b4c1a144fc2c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Dec 2017 07:36:06 -0800 Subject: [PATCH 6/7] Denormalize added and removed line counts for the current diff onto revisions Summary: See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts. I'd like to try a `[---+ ]` sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff. This just makes the data available without any subquerying and doesn't actually change the UI. Test Plan: Created a revision, saw detailed change information populate in the database. ``` mysql> select * from differential_revision where id = 292\G *************************** 1. row *************************** id: 292 title: WIP originalTitle: WIP phid: PHID-DREV-ux3cxptibn3l5pxsug3z status: draft summary: asdf testPlan: asdf authorPHID: PHID-USER-cvfydnwadpdj7vdon36z lastReviewerPHID: NULL lineCount: 41 dateCreated: 1513179418 dateModified: 1513179418 attached: [] mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz branchName: NULL viewPolicy: users editPolicy: users repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p properties: {"lines.added":40,"lines.removed":1} activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce 1 row in set (0.00 sec) ``` Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18832 --- .../editor/DifferentialTransactionEditor.php | 24 ++++++++++++++++++- .../storage/DifferentialRevision.php | 17 +++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index b2fc179002..e6dfed88a7 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -132,12 +132,14 @@ final class DifferentialTransactionEditor $diff = $this->requireDiff($xaction->getNewValue()); - $object->setLineCount($diff->getLineCount()); + $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; @@ -1645,5 +1647,25 @@ 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']); + } + } } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 73f22c91e4..2c82de164a 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -61,6 +61,8 @@ final class DifferentialRevision extends DifferentialDAO const PROPERTY_CLOSED_FROM_ACCEPTED = 'wasAcceptedBeforeClose'; const PROPERTY_DRAFT_HOLD = 'draft.hold'; const PROPERTY_HAS_BROADCAST = 'draft.broadcast'; + const PROPERTY_LINES_ADDED = 'lines.added'; + const PROPERTY_LINES_REMOVED = 'lines.removed'; public static function initializeNewRevision(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) @@ -728,6 +730,21 @@ final class DifferentialRevision extends DifferentialDAO return $this->setProperty(self::PROPERTY_HAS_BROADCAST, $has_broadcast); } + public function setAddedLineCount($count) { + return $this->setProperty(self::PROPERTY_LINES_ADDED, $count); + } + + public function getAddedLineCount() { + return $this->getProperty(self::PROPERTY_LINES_ADDED); + } + + public function setRemovedLineCount($count) { + return $this->setProperty(self::PROPERTY_LINES_REMOVED, $count); + } + + public function getRemovedLineCount() { + return $this->getProperty(self::PROPERTY_LINES_REMOVED); + } public function loadActiveBuilds(PhabricatorUser $viewer) { $diff = $this->getActiveDiff(); From 7cbbe2ccf793fe29d1cb7b6a7081f57da1f24795 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Dec 2017 07:05:07 -0800 Subject: [PATCH 7/7] When users browse to a submodule path in Diffusion explicitly, don't fatal Summary: Ref T13030. See PHI254. This behavior could be cleaner than I've made it, but it fixes the "this is totally broken" issue, replacing a fatal/exception with an informative (just not terribly useful) page. Test Plan: - Added a submodule to a repository. - In Diffusion, clicked some other file next to the submodule, then edited the URI to the submodule path instead. - Before patch: fatal. - After patch: relatively useful message about this being a submodule. Note that it's normally hard to hit this URI directly. In the browse view, submodules are marked up as directories and linked to a separate submodule resolution flow. {F5321524} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13030 Differential Revision: https://secure.phabricator.com/D18831 --- .../DiffusionBrowseQueryConduitAPIMethod.php | 30 +++++++++++++++++++ .../data/DiffusionBrowseResultSet.php | 1 + .../view/DiffusionEmptyResultView.php | 7 +++++ 3 files changed, 38 insertions(+) diff --git a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php index 7ff0f6725a..2e272d69a7 100644 --- a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php @@ -52,6 +52,36 @@ final class DiffusionBrowseQueryConduitAPIMethod $commit, $path); } catch (CommandException $e) { + // The "cat-file" command may fail if the path legitimately does not + // exist, but it may also fail if the path is a submodule. This can + // produce either "Not a valid object name" or "could not get object + // info". + + // To detect if we have a submodule, use `git ls-tree`. If the path + // is a submodule, we'll get a "160000" mode mask with type "commit". + + list($sub_err, $sub_stdout) = $repository->execLocalCommand( + 'ls-tree %s -- %s', + $commit, + $path); + if (!$sub_err) { + // If the path failed "cat-file" but "ls-tree" worked, we assume it + // must be a submodule. If it is, the output will look something + // like this: + // + // 160000 commit + // + // We make sure it has the 160000 mode mask to confirm that it's + // definitely a submodule. + $mode = (int)$sub_stdout; + if ($mode & 160000) { + $submodule_reason = DiffusionBrowseResultSet::REASON_IS_SUBMODULE; + $result + ->setReasonForEmptyResultSet($submodule_reason); + return $result; + } + } + $stderr = $e->getStderr(); if (preg_match('/^fatal: Not a valid object name/', $stderr)) { // Grab two logs, since the first one is when the object was deleted. diff --git a/src/applications/diffusion/data/DiffusionBrowseResultSet.php b/src/applications/diffusion/data/DiffusionBrowseResultSet.php index 2208aca505..22d9e08251 100644 --- a/src/applications/diffusion/data/DiffusionBrowseResultSet.php +++ b/src/applications/diffusion/data/DiffusionBrowseResultSet.php @@ -3,6 +3,7 @@ final class DiffusionBrowseResultSet extends Phobject { const REASON_IS_FILE = 'is-file'; + const REASON_IS_SUBMODULE = 'is-submodule'; const REASON_IS_DELETED = 'is-deleted'; const REASON_IS_NONEXISTENT = 'nonexistent'; const REASON_BAD_COMMIT = 'bad-commit'; diff --git a/src/applications/diffusion/view/DiffusionEmptyResultView.php b/src/applications/diffusion/view/DiffusionEmptyResultView.php index 12def83e76..c818267268 100644 --- a/src/applications/diffusion/view/DiffusionEmptyResultView.php +++ b/src/applications/diffusion/view/DiffusionEmptyResultView.php @@ -40,6 +40,13 @@ final class DiffusionEmptyResultView extends DiffusionView { $body = pht('This path was an empty directory at %s.', $commit); $severity = PHUIInfoView::SEVERITY_NOTICE; break; + case DiffusionBrowseResultSet::REASON_IS_SUBMODULE: + $title = pht('Submodule'); + // TODO: We could improve this, but it is normally difficult to + // reach this page for a submodule. + $body = pht('This path was a submodule at %s.', $commit); + $severity = PHUIInfoView::SEVERITY_NOTICE; + break; case DiffusionBrowseResultSet::REASON_IS_DELETED: $deleted = $this->browseResultSet->getDeletedAtCommit(); $existed = $this->browseResultSet->getExistedAtCommit();