diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index c8b9ccfe63..8111be2fad 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -690,28 +690,35 @@ final class DifferentialChangesetParser { $shield = $renderer->renderShield( pht( 'This file contains generated code, which does not normally '. - 'need to be reviewed.'), - true); + 'need to be reviewed.')); } else if ($this->isUnchanged()) { - $shield = $renderer->renderShield( - pht("The contents of this file were not changed."), - false); + $type = 'text'; + if (!$rows) { + // NOTE: Normally, diffs which don't change files do not include + // file content (for example, if you "chmod +x" a file and then + // run "git show", the file content is not available). Similarly, + // if you move a file from A to B without changing it, diffs normally + // do not show the file content. In some cases `arc` is able to + // synthetically generate content for these diffs, but for raw diffs + // we'll never have it so we need to be prepared to not render a link. + $type = 'none'; + } + $shield = $renderer->renderShield( + pht('The contents of this file were not changed.'), + $type); } else if ($this->isWhitespaceOnly()) { $shield = $renderer->renderShield( - pht( - 'This file was changed only by adding or removing whitespace.'), - false); + pht('This file was changed only by adding or removing whitespace.'), + 'whitespace'); } else if ($this->isDeleted()) { $shield = $renderer->renderShield( - pht("This file was completely deleted."), - true); + pht('This file was completely deleted.')); } else if ($this->changeset->getAffectedLineCount() > 2500) { $lines = number_format($this->changeset->getAffectedLineCount()); $shield = $renderer->renderShield( pht( 'This file has a very large number of changes ({%s} lines).', - $lines), - true); + $lines)); } } diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index 424c6cfbf0..5d2f9a6222 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -249,6 +249,7 @@ final class DifferentialHunkParser { $n_type = null; } + // This line does not exist in the new file. if (($o_type != null) && ($n_type == null)) { $rebuild_old[] = $old_line_data; $rebuild_new[] = null; @@ -258,6 +259,7 @@ final class DifferentialHunkParser { continue; } + // This line does not exist in the old file. if (($n_type != null) && ($o_type == null)) { $rebuild_old[] = null; $rebuild_new[] = $new_line_data; diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 9d7671bbc7..157aff5f97 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -258,28 +258,69 @@ abstract class DifferentialChangesetRenderer { $vs = 0 ); - public function renderShield($message, $more) { + /** + * Render a "shield" over the diff, with a message like "This file is + * generated and does not need to be reviewed." or "This file was completely + * deleted." This UI element hides unimportant text so the reviewer doesn't + * need to scroll past it. + * + * The shield includes a link to view the underlying content. This link + * may force certain rendering modes when the link is clicked: + * + * - `"default"`: Render the diff normally, as though it was not + * shielded. This is the default and appropriate if the underlying + * diff is a normal change, but was hidden for reasons of not being + * important (e.g., generated code). + * - `"text"`: Force the text to be shown. This is probably only relevant + * when a file is not changed. + * - `"whitespace"`: Force the text to be shown, and the diff to be + * rendered with all whitespace shown. This is probably only relevant + * when a file is changed only by altering whitespace. + * - `"none"`: Don't show the link (e.g., text not available). + * + * @param string Message explaining why the diff is hidden. + * @param string|null Force mode, see above. + * @return string Shield markup. + */ + public function renderShield($message, $force = 'default') { - if ($more) { - $end = $this->getLineCount(); - $reference = $this->getRenderingReference(); - $more = - ' '. - javelin_render_tag( - 'a', - array( - 'mustcapture' => true, - 'sigil' => 'show-more', - 'class' => 'complete', - 'href' => '#', - 'meta' => array( - 'ref' => $reference, - 'range' => "0-{$end}", - ), - ), - 'Show File Contents'); - } else { - $more = null; + $end = $this->getLineCount(); + $reference = $this->getRenderingReference(); + + if ($force !== 'text' && + $force !== 'whitespace' && + $force !== 'none' && + $force !== 'default') { + throw new Exception("Invalid 'force' parameter '{$force}'!"); + } + + $range = "0-{$end}"; + if ($force == 'text') { + // If we're forcing text, force the whole file to be rendered. + $range = "{$range}/0-{$end}"; + } + + $meta = array( + 'ref' => $reference, + 'range' => $range, + ); + + if ($force == 'whitespace') { + $meta['whitespace'] = DifferentialChangesetParser::WHITESPACE_SHOW_ALL; + } + + $more = null; + if ($force !== 'none') { + $more = ' '.javelin_render_tag( + 'a', + array( + 'mustcapture' => true, + 'sigil' => 'show-more', + 'class' => 'complete', + 'href' => '#', + 'meta' => $meta, + ), + 'Show File Contents'); } return javelin_render_tag( diff --git a/webroot/rsrc/js/application/differential/behavior-show-more.js b/webroot/rsrc/js/application/differential/behavior-show-more.js index b38f5062b7..2c44b798c3 100644 --- a/webroot/rsrc/js/application/differential/behavior-show-more.js +++ b/webroot/rsrc/js/application/differential/behavior-show-more.js @@ -39,7 +39,11 @@ JX.behavior('differential-show-more', function(config) { var container = JX.DOM.scry(context, 'td')[0]; JX.DOM.setContent(container, 'Loading...'); JX.DOM.alterClass(context, 'differential-show-more-loading', true); - data['whitespace'] = config.whitespace; + + if (!data['whitespace']) { + data['whitespace'] = config.whitespace; + } + new JX.Workflow(config.uri, data) .setHandler(JX.bind(null, onresponse, context)) .start();