From c1eeacd8500b6f3e449b47d7f7bf8dfad7f95818 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Jul 2020 12:37:34 -0700 Subject: [PATCH 1/5] Move "Wait for Previous Commits to Build" out of prototype Summary: Although I'm not entirely thrilled about doing flow control like this (as an actual action in a build plan), I believe this build step works correctly and there's no fancy replacement mechanism on the immediate horizon, and this didn't send us down a slippery slope of Turing-complete builds encoded without real structure or context. Just kick it out of prototype. (Other approaches which might be better in the long run are things like "this is a top-level behavior on the build plan itself" and/or "build plans are written in a DSL, not a Javascript UI".) Test Plan: Added a new build step, saw this as an option in the "Flow Control" section. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D21432 --- .../step/HarbormasterWaitForPreviousBuildStepImplementation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php index eadf4c94f5..cec25c6ad3 100644 --- a/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php @@ -14,7 +14,7 @@ final class HarbormasterWaitForPreviousBuildStepImplementation } public function getBuildStepGroupKey() { - return HarbormasterPrototypeBuildStepGroup::GROUPKEY; + return HarbormasterControlBuildStepGroup::GROUPKEY; } public function execute( From 98e0440d459b7a64b02d7976372aa2fca499f8bd Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Aug 2020 09:46:51 -0700 Subject: [PATCH 2/5] In 1-up source diffs, retain the "No newline at end of file" on "\" lines Summary: See PHI1839. Currently, the "No newline at end of file" text is dropped in the 1-up diff view for changes that affect a file with no trailing newline. Track it through the construction of diff primitivies more carefully. Test Plan: {F7695760} Differential Revision: https://secure.phabricator.com/D21433 --- .../render/DifferentialChangesetOneUpRenderer.php | 8 ++++++-- .../differential/render/DifferentialChangesetRenderer.php | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 186924e359..bcdbf4dd46 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -83,7 +83,9 @@ final class DifferentialChangesetOneUpRenderer $cells = array(); if ($is_old) { if ($p['htype']) { - if (empty($p['oline'])) { + if ($p['htype'] === '\\') { + $class = 'comment'; + } else if (empty($p['oline'])) { $class = 'left old old-full'; } else { $class = 'left old'; @@ -129,7 +131,9 @@ final class DifferentialChangesetOneUpRenderer $cells[] = $no_coverage; } else { if ($p['htype']) { - if (empty($p['oline'])) { + if ($p['htype'] === '\\') { + $class = 'comment'; + } else if (empty($p['oline'])) { $class = 'right new new-full'; } else { $class = 'right new'; diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index d493c0e886..dcae3b979b 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -505,6 +505,8 @@ abstract class DifferentialChangesetRenderer extends Phobject { $ospec['htype'] = $old[$ii]['type']; if (isset($old_render[$ii])) { $ospec['render'] = $old_render[$ii]; + } else if ($ospec['htype'] === '\\') { + $ospec['render'] = $old[$ii]['text']; } } @@ -514,6 +516,8 @@ abstract class DifferentialChangesetRenderer extends Phobject { $nspec['htype'] = $new[$ii]['type']; if (isset($new_render[$ii])) { $nspec['render'] = $new_render[$ii]; + } else if ($nspec['htype'] === '\\') { + $nspec['render'] = $new[$ii]['text']; } } From 2db1955159f78ec731243b8532a1b3603eb28508 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Aug 2020 12:15:17 -0700 Subject: [PATCH 3/5] In Jupyter notebooks, read strings stored in the raw as either "string" or "list" more consistently Summary: Ref PHI1835. Generally, Jupyter notebooks in the wild may store source and markdown content as either a single string or a list of strings. Make the renderer read these formats more consistently. In particular, this fixes rendering of code blocks stored as a single string. This also fixes an issue where cell labels were double-rendered in diff views. Test Plan: Created a notebook with a code block represented on disk as a single string, rendered a diff from it. {F7696071} Differential Revision: https://secure.phabricator.com/D21434 --- .../PhabricatorJupyterDocumentEngine.php | 80 ++++++++----------- 1 file changed, 32 insertions(+), 48 deletions(-) diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index 753cdf3921..20474b047b 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -85,15 +85,8 @@ final class PhabricatorJupyterDocumentEngine if ($utype === $vtype) { switch ($utype) { case 'markdown': - $usource = idx($ucell, 'source'); - if (is_array($usource)) { - $usource = implode('', $usource); - } - - $vsource = idx($vcell, 'source'); - if (is_array($vsource)) { - $vsource = implode('', $vsource); - } + $usource = $this->readString($ucell, 'source'); + $vsource = $this->readString($vcell, 'source'); $diff = id(new PhutilProseDifferenceEngine()) ->getDiff($usource, $vsource); @@ -117,8 +110,6 @@ final class PhabricatorJupyterDocumentEngine $vsource = idx($vcell, 'raw'); $udisplay = idx($ucell, 'display'); $vdisplay = idx($vcell, 'display'); - $ulabel = idx($ucell, 'label'); - $vlabel = idx($vcell, 'label'); $intraline_segments = ArcanistDiffUtils::generateIntralineDiff( $usource, @@ -142,15 +133,15 @@ final class PhabricatorJupyterDocumentEngine $vdisplay, $v_segments); - $u_content = $this->newCodeLineCell($ucell, $usource); - $v_content = $this->newCodeLineCell($vcell, $vsource); + list($u_label, $u_content) = $this->newCodeLineCell($ucell, $usource); + list($v_label, $v_content) = $this->newCodeLineCell($vcell, $vsource); $classes = array( 'jupyter-cell-flush', ); - $u_content = $this->newJupyterCell($ulabel, $u_content, $classes); - $v_content = $this->newJupyterCell($vlabel, $v_content, $classes); + $u_content = $this->newJupyterCell($u_label, $u_content, $classes); + $v_content = $this->newJupyterCell($v_label, $v_content, $classes); $u_content = $this->newCellContainer($u_content); $v_content = $this->newCellContainer($v_content); @@ -259,10 +250,7 @@ final class PhabricatorJupyterDocumentEngine $hash_input = $cell['raw']; break; case 'markdown': - $hash_input = $cell['source']; - if (is_array($hash_input)) { - $hash_input = implode('', $cell['source']); - } + $hash_input = $this->readString($cell, 'source'); break; default: $hash_input = serialize($cell); @@ -334,7 +322,6 @@ final class PhabricatorJupyterDocumentEngine 'be rendered as a Jupyter notebook.')); } - $nbformat = idx($data, 'nbformat'); if (!strlen($nbformat)) { throw new Exception( @@ -376,10 +363,7 @@ final class PhabricatorJupyterDocumentEngine foreach ($cells as $cell) { $cell_type = idx($cell, 'cell_type'); if ($cell_type === 'markdown') { - $source = $cell['source']; - if (is_array($source)) { - $source = implode('', $source); - } + $source = $this->readString($cell, 'source'); // Attempt to split contiguous blocks of markdown into smaller // pieces. @@ -404,11 +388,7 @@ final class PhabricatorJupyterDocumentEngine $label = $this->newCellLabel($cell); - $lines = idx($cell, 'source'); - if (!is_array($lines)) { - $lines = array(); - } - + $lines = $this->readStringList($cell, 'source'); $content = $this->highlightLines($lines); $count = count($lines); @@ -526,10 +506,7 @@ final class PhabricatorJupyterDocumentEngine } private function newMarkdownCell(array $cell) { - $content = idx($cell, 'source'); - if (!is_array($content)) { - $content = array(); - } + $content = $this->readStringList($cell, 'source'); // TODO: This should ideally highlight as Markdown, but the "md" // highlighter in Pygments is painfully slow and not terribly useful. @@ -549,11 +526,7 @@ final class PhabricatorJupyterDocumentEngine private function newCodeCell(array $cell) { $label = $this->newCellLabel($cell); - $content = idx($cell, 'source'); - if (!is_array($content)) { - $content = array(); - } - + $content = $this->readStringList($cell, 'source'); $content = $this->highlightLines($content); $outputs = array(); @@ -660,11 +633,7 @@ final class PhabricatorJupyterDocumentEngine continue; } - $raw_data = $data[$image_format]; - if (!is_array($raw_data)) { - $raw_data = array($raw_data); - } - $raw_data = implode('', $raw_data); + $raw_data = $this->readString($data, $image_format); $content = phutil_tag( 'img', @@ -695,11 +664,7 @@ final class PhabricatorJupyterDocumentEngine break; case 'stream': default: - $content = idx($output, 'text'); - if (!is_array($content)) { - $content = array(); - } - $content = implode('', $content); + $content = $this->readString($output, 'text'); break; } @@ -761,4 +726,23 @@ final class PhabricatorJupyterDocumentEngine return true; } + private function readString(array $src, $key) { + $list = $this->readStringList($src, $key); + return implode('', $list); + } + + private function readStringList(array $src, $key) { + $list = idx($src, $key); + + if (is_array($list)) { + $list = $list; + } else if (is_string($list)) { + $list = array($list); + } else { + $list = array(); + } + + return $list; + } + } From dbdfac1e072ef7d77513bf41cac2ee1a7d8678f3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Aug 2020 12:56:04 -0700 Subject: [PATCH 4/5] Recover inline comments which are "adjusted" off the end of a diff Summary: See PHI1834. Currently, the inline adjustment engine can sometime "adjust" an inline off the end of a diff. If it does, we lay it out on an invalid display line here and never render it. Instead, make sure that layout never puts a comment on an invalid line, so the UI is robust against questionable decisions by the adjustment engine: no adjustment should be able to accidentally discard an inline. Test Plan: - Created a two diff revision, where Diffs 1 and 2 have "alphabet.txt" with A-Z on one line each. The file is unchanged across diffs; some other file is changed. - Added a comment to lines P-Z of Diff 1. - Before: comment is adjusted out of range on Diff 2 and not shown in the UI. - After: comment is still adjusted out of range internally, but now corrected into the display range and shown. Differential Revision: https://secure.phabricator.com/D21435 --- .../parser/DifferentialChangesetParser.php | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 3652d18a9a..ad62b2a1ef 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1053,14 +1053,30 @@ final class DifferentialChangesetParser extends Phobject { $this->comments = id(new PHUIDiffInlineThreader()) ->reorderAndThreadCommments($this->comments); + $old_max_display = 1; + foreach ($this->old as $old) { + if (isset($old['line'])) { + $old_max_display = $old['line']; + } + } + + $new_max_display = 1; + foreach ($this->new as $new) { + if (isset($new['line'])) { + $new_max_display = $new['line']; + } + } + foreach ($this->comments as $comment) { - $final = $comment->getLineNumber() + - $comment->getLineLength(); - $final = max(1, $final); + $display_line = $comment->getLineNumber() + $comment->getLineLength(); + $display_line = max(1, $display_line); + if ($this->isCommentOnRightSideWhenDisplayed($comment)) { - $new_comments[$final][] = $comment; + $display_line = min($new_max_display, $display_line); + $new_comments[$display_line][] = $comment; } else { - $old_comments[$final][] = $comment; + $display_line = min($old_max_display, $display_line); + $old_comments[$display_line][] = $comment; } } } From ce0dc9a2ba2ca86e68f0e726df58294c4716b23d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Aug 2020 13:06:57 -0700 Subject: [PATCH 5/5] Correct an apparent off-by-one error when adjusting inlines across revision changes Summary: See PHI1834. It's not obvious why this "+1" is present in the code, but it causes inlines to be adjusted incorrectly when a file is not modified across changes. See D21435. Remove it, which appears to produce accurate adjustment behavior. Test Plan: - See D21435 for instructions to build a change, where a file with lines "A-Z" is unmodified across Diff 1 and Diff 2. - Left inlines on lines 14, 17-19, and 16-26 (end of the file) on Diff 1. - Before: saw inlines incorrectly adjusted to lines 15, 18, and 17 on Diff 2. Before D21435, the last inline was culled by the rendering engine. - After: saw inlines correctly adjusted to lines 14, 17, and 16 (the same lines as the original), render properly, and highlight the correct lines when hovered. Differential Revision: https://secure.phabricator.com/D21436 --- .../diff/engine/PhabricatorInlineCommentAdjustmentEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php b/src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php index 095a44c221..f1888cc363 100644 --- a/src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php +++ b/src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php @@ -358,7 +358,7 @@ final class PhabricatorInlineCommentAdjustmentEngine list($tail_deleted, $tail_offset, $tail_line) = $tail_info; if ($head_offset !== false) { - $inline->setLineNumber($head_line + 1 + $head_offset); + $inline->setLineNumber($head_line + $head_offset); } else { $inline->setLineNumber($head_line); $inline->setLineLength($tail_line - $head_line);