@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Fix whitespace and unchanged file shields
Summary:
Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems:
- The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy).
- When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link.
- When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode.
- We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it.
- When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content.
Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T181, T2009
Differential Revision: https://secure.phabricator.com/D4407
+88
-34
+19
-12
src/applications/differential/parser/DifferentialChangesetParser.php
+19
-12
src/applications/differential/parser/DifferentialChangesetParser.php
···
690
690
$shield = $renderer->renderShield(
691
691
pht(
692
692
'This file contains generated code, which does not normally '.
693
-
'need to be reviewed.'),
694
-
true);
693
+
'need to be reviewed.'));
695
694
} else if ($this->isUnchanged()) {
696
-
$shield = $renderer->renderShield(
697
-
pht("The contents of this file were not changed."),
698
-
false);
695
+
$type = 'text';
696
+
if (!$rows) {
697
+
// NOTE: Normally, diffs which don't change files do not include
698
+
// file content (for example, if you "chmod +x" a file and then
699
+
// run "git show", the file content is not available). Similarly,
700
+
// if you move a file from A to B without changing it, diffs normally
701
+
// do not show the file content. In some cases `arc` is able to
702
+
// synthetically generate content for these diffs, but for raw diffs
703
+
// we'll never have it so we need to be prepared to not render a link.
704
+
$type = 'none';
705
+
}
706
+
$shield = $renderer->renderShield(
707
+
pht('The contents of this file were not changed.'),
708
+
$type);
699
709
} else if ($this->isWhitespaceOnly()) {
700
710
$shield = $renderer->renderShield(
701
-
pht(
702
-
'This file was changed only by adding or removing whitespace.'),
703
-
false);
711
+
pht('This file was changed only by adding or removing whitespace.'),
712
+
'whitespace');
704
713
} else if ($this->isDeleted()) {
705
714
$shield = $renderer->renderShield(
706
-
pht("This file was completely deleted."),
707
-
true);
715
+
pht('This file was completely deleted.'));
708
716
} else if ($this->changeset->getAffectedLineCount() > 2500) {
709
717
$lines = number_format($this->changeset->getAffectedLineCount());
710
718
$shield = $renderer->renderShield(
711
719
pht(
712
720
'This file has a very large number of changes ({%s} lines).',
713
-
$lines),
714
-
true);
721
+
$lines));
715
722
}
716
723
}
717
724
+2
src/applications/differential/parser/DifferentialHunkParser.php
+2
src/applications/differential/parser/DifferentialHunkParser.php
···
249
249
$n_type = null;
250
250
}
251
251
252
+
// This line does not exist in the new file.
252
253
if (($o_type != null) && ($n_type == null)) {
253
254
$rebuild_old[] = $old_line_data;
254
255
$rebuild_new[] = null;
···
258
259
continue;
259
260
}
260
261
262
+
// This line does not exist in the old file.
261
263
if (($n_type != null) && ($o_type == null)) {
262
264
$rebuild_old[] = null;
263
265
$rebuild_new[] = $new_line_data;
+62
-21
src/applications/differential/render/DifferentialChangesetRenderer.php
+62
-21
src/applications/differential/render/DifferentialChangesetRenderer.php
···
258
258
$vs = 0
259
259
);
260
260
261
-
public function renderShield($message, $more) {
261
+
/**
262
+
* Render a "shield" over the diff, with a message like "This file is
263
+
* generated and does not need to be reviewed." or "This file was completely
264
+
* deleted." This UI element hides unimportant text so the reviewer doesn't
265
+
* need to scroll past it.
266
+
*
267
+
* The shield includes a link to view the underlying content. This link
268
+
* may force certain rendering modes when the link is clicked:
269
+
*
270
+
* - `"default"`: Render the diff normally, as though it was not
271
+
* shielded. This is the default and appropriate if the underlying
272
+
* diff is a normal change, but was hidden for reasons of not being
273
+
* important (e.g., generated code).
274
+
* - `"text"`: Force the text to be shown. This is probably only relevant
275
+
* when a file is not changed.
276
+
* - `"whitespace"`: Force the text to be shown, and the diff to be
277
+
* rendered with all whitespace shown. This is probably only relevant
278
+
* when a file is changed only by altering whitespace.
279
+
* - `"none"`: Don't show the link (e.g., text not available).
280
+
*
281
+
* @param string Message explaining why the diff is hidden.
282
+
* @param string|null Force mode, see above.
283
+
* @return string Shield markup.
284
+
*/
285
+
public function renderShield($message, $force = 'default') {
286
+
287
+
$end = $this->getLineCount();
288
+
$reference = $this->getRenderingReference();
289
+
290
+
if ($force !== 'text' &&
291
+
$force !== 'whitespace' &&
292
+
$force !== 'none' &&
293
+
$force !== 'default') {
294
+
throw new Exception("Invalid 'force' parameter '{$force}'!");
295
+
}
296
+
297
+
$range = "0-{$end}";
298
+
if ($force == 'text') {
299
+
// If we're forcing text, force the whole file to be rendered.
300
+
$range = "{$range}/0-{$end}";
301
+
}
302
+
303
+
$meta = array(
304
+
'ref' => $reference,
305
+
'range' => $range,
306
+
);
307
+
308
+
if ($force == 'whitespace') {
309
+
$meta['whitespace'] = DifferentialChangesetParser::WHITESPACE_SHOW_ALL;
310
+
}
262
311
263
-
if ($more) {
264
-
$end = $this->getLineCount();
265
-
$reference = $this->getRenderingReference();
266
-
$more =
267
-
' '.
268
-
javelin_render_tag(
269
-
'a',
270
-
array(
271
-
'mustcapture' => true,
272
-
'sigil' => 'show-more',
273
-
'class' => 'complete',
274
-
'href' => '#',
275
-
'meta' => array(
276
-
'ref' => $reference,
277
-
'range' => "0-{$end}",
278
-
),
279
-
),
280
-
'Show File Contents');
281
-
} else {
282
-
$more = null;
312
+
$more = null;
313
+
if ($force !== 'none') {
314
+
$more = ' '.javelin_render_tag(
315
+
'a',
316
+
array(
317
+
'mustcapture' => true,
318
+
'sigil' => 'show-more',
319
+
'class' => 'complete',
320
+
'href' => '#',
321
+
'meta' => $meta,
322
+
),
323
+
'Show File Contents');
283
324
}
284
325
285
326
return javelin_render_tag(
+5
-1
webroot/rsrc/js/application/differential/behavior-show-more.js
+5
-1
webroot/rsrc/js/application/differential/behavior-show-more.js
···
39
39
var container = JX.DOM.scry(context, 'td')[0];
40
40
JX.DOM.setContent(container, 'Loading...');
41
41
JX.DOM.alterClass(context, 'differential-show-more-loading', true);
42
-
data['whitespace'] = config.whitespace;
42
+
43
+
if (!data['whitespace']) {
44
+
data['whitespace'] = config.whitespace;
45
+
}
46
+
43
47
new JX.Workflow(config.uri, data)
44
48
.setHandler(JX.bind(null, onresponse, context))
45
49
.start();