Skip to content

Commit 5310f1c

Browse files
author
epriestley
committed
Remove all whitespace options/configuration everywhere
Summary: Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely. I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction. Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T3498 Differential Revision: https://secure.phabricator.com/D20185
1 parent 661c758 commit 5310f1c

23 files changed

+33
-375
lines changed

‎resources/celerity/map.php

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
'core.pkg.css' => '261ee8cf',
1313
'core.pkg.js' => '5ace8a1e',
1414
'differential.pkg.css' => 'c3f15714',
15-
'differential.pkg.js' => '67c9ea4c',
15+
'differential.pkg.js' => 'be031567',
1616
'diffusion.pkg.css' => '42c75c37',
1717
'diffusion.pkg.js' => '91192d85',
1818
'maniphest.pkg.css' => '35995d6d',
@@ -374,7 +374,7 @@
374374
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '076bd092',
375375
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
376376
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '9b1cbd76',
377-
'rsrc/js/application/diff/DiffChangeset.js' => 'e7cf10d6',
377+
'rsrc/js/application/diff/DiffChangeset.js' => 'd0a85a85',
378378
'rsrc/js/application/diff/DiffChangesetList.js' => 'b91204e9',
379379
'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94',
380380
'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
@@ -753,7 +753,7 @@
753753
'phabricator-darklog' => '3b869402',
754754
'phabricator-darkmessage' => '26cd4b73',
755755
'phabricator-dashboard-css' => '4267d6c6',
756-
'phabricator-diff-changeset' => 'e7cf10d6',
756+
'phabricator-diff-changeset' => 'd0a85a85',
757757
'phabricator-diff-changeset-list' => 'b91204e9',
758758
'phabricator-diff-inline' => 'a4a14a94',
759759
'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1973,6 +1973,17 @@
19731973
'javelin-stratcom',
19741974
'javelin-util',
19751975
),
1976+
'd0a85a85' => array(
1977+
'javelin-dom',
1978+
'javelin-util',
1979+
'javelin-stratcom',
1980+
'javelin-install',
1981+
'javelin-workflow',
1982+
'javelin-router',
1983+
'javelin-behavior-device',
1984+
'javelin-vector',
1985+
'phabricator-diff-inline',
1986+
),
19761987
'd12d214f' => array(
19771988
'javelin-install',
19781989
'javelin-dom',
@@ -2038,17 +2049,6 @@
20382049
'javelin-dom',
20392050
'phabricator-draggable-list',
20402051
),
2041-
'e7cf10d6' => array(
2042-
'javelin-dom',
2043-
'javelin-util',
2044-
'javelin-stratcom',
2045-
'javelin-install',
2046-
'javelin-workflow',
2047-
'javelin-router',
2048-
'javelin-behavior-device',
2049-
'javelin-vector',
2050-
'phabricator-diff-inline',
2051-
),
20522052
'e8240b50' => array(
20532053
'javelin-behavior',
20542054
'javelin-stratcom',

‎src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,8 @@ public static function getAncientConfig() {
533533
'This ancient extension point has been replaced with other '.
534534
'mechanisms, including "AphrontSite".'),
535535

536+
'differential.whitespace-matters' => pht(
537+
'Whitespace rendering is now handled automatically.'),
536538
);
537539

538540
return $ancient_config;

‎src/applications/differential/__tests__/DifferentialParseRenderTestCase.php

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ public function testParseRender() {
3131
foreach (array('one', 'two') as $type) {
3232
$this->runParser($type, $data, $file, 'expect');
3333
$this->runParser($type, $data, $file, 'unshielded');
34-
$this->runParser($type, $data, $file, 'whitespace');
3534
}
3635
}
3736
}
@@ -44,25 +43,20 @@ private function runParser($type, $data, $file, $extension) {
4443
}
4544

4645
$unshielded = false;
47-
$whitespace = false;
4846
switch ($extension) {
4947
case 'unshielded':
5048
$unshielded = true;
5149
break;
52-
case 'whitespace';
53-
$unshielded = true;
54-
$whitespace = true;
55-
break;
5650
}
5751

5852
$parsers = $this->buildChangesetParsers($type, $data, $file);
59-
$actual = $this->renderParsers($parsers, $unshielded, $whitespace);
53+
$actual = $this->renderParsers($parsers, $unshielded);
6054
$expect = Filesystem::readFile($test_file);
6155

6256
$this->assertEqual($expect, $actual, basename($test_file));
6357
}
6458

65-
private function renderParsers(array $parsers, $unshield, $whitespace) {
59+
private function renderParsers(array $parsers, $unshield) {
6660
$result = array();
6761
foreach ($parsers as $parser) {
6862
if ($unshield) {
@@ -73,11 +67,6 @@ private function renderParsers(array $parsers, $unshield, $whitespace) {
7367
$e_range = null;
7468
}
7569

76-
if ($whitespace) {
77-
$parser->setWhitespaceMode(
78-
DifferentialChangesetParser::WHITESPACE_SHOW_ALL);
79-
}
80-
8170
$result[] = $parser->render($s_range, $e_range, array());
8271
}
8372
return implode(str_repeat('~', 80)."\n", $result);

‎src/applications/differential/__tests__/data/generated.diff.one.unshielded

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
N 1 . @generated\n~
2-
O 2 - \n~
2+
N 2 . \n~
33
O 3 - This is a generated file.\n~
4-
N 2 + \n~
54
N 3 + This is a generated file{(, full of generated code)}.\n~
65
N 4 . \n~

‎src/applications/differential/__tests__/data/generated.diff.two.unshielded

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
O 1 . @generated\n~
22
N 1 . @generated\n~
3-
O 2 - \n~
4-
N 2 + \n~
3+
O 2 . \n~
4+
N 2 . \n~
55
O 3 - This is a generated file.\n~
66
N 3 + This is a generated file{(, full of generated code)}.\n~
77
O 4 . \n~

‎src/applications/differential/__tests__/data/whitespace.diff.one.expect

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@ CTYPE 2 1 (unforced)
22
WHITESPACE
33
WHITESPACE
44
-
5-
SHIELD (whitespace) This file was changed only by adding or removing whitespace.
5+
O 1 - -=[-Rocket-Ship>\n~
6+
N 1 + {> )}-=[-Rocket-Ship>\n~

‎src/applications/differential/__tests__/data/whitespace.diff.two.expect

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@ CTYPE 2 1 (unforced)
22
WHITESPACE
33
WHITESPACE
44
-
5-
SHIELD (whitespace) This file was changed only by adding or removing whitespace.
5+
O 1 - -=[-Rocket-Ship>\n~
6+
N 1 + {> )}-=[-Rocket-Ship>\n~

‎src/applications/differential/config/PhabricatorDifferentialConfigOptions.php

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -80,18 +80,6 @@ public function getOptions() {
8080
"Select and reorder revision fields.\n\n".
8181
"NOTE: This feature is under active development and subject ".
8282
"to change.")),
83-
$this->newOption(
84-
'differential.whitespace-matters',
85-
'list<regex>',
86-
array(
87-
'/\.py$/',
88-
'/\.l?hs$/',
89-
'/\.ya?ml$/',
90-
))
91-
->setDescription(
92-
pht(
93-
"List of file regexps where whitespace is meaningful and should ".
94-
"not use 'ignore-all' by default")),
9583
$this->newOption('differential.require-test-plan-field', 'bool', true)
9684
->setBoolOptions(
9785
array(

‎src/applications/differential/controller/DifferentialRevisionViewController.php

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -305,10 +305,6 @@ public function handleRequest(AphrontRequest $request) {
305305
$details = $this->buildDetails($revision, $field_list);
306306
$curtain = $this->buildCurtain($revision);
307307

308-
$whitespace = $request->getStr(
309-
'whitespace',
310-
DifferentialChangesetParser::WHITESPACE_IGNORE_MOST);
311-
312308
$repository = $revision->getRepository();
313309
if ($repository) {
314310
$symbol_indexes = $this->buildSymbolIndexes(
@@ -383,7 +379,6 @@ public function handleRequest(AphrontRequest $request) {
383379
->setDiff($target)
384380
->setRenderingReferences($rendering_references)
385381
->setVsMap($vs_map)
386-
->setWhitespace($whitespace)
387382
->setSymbolIndexes($symbol_indexes)
388383
->setTitle(pht('Diff %s', $target->getID()))
389384
->setBackground(PHUIObjectBoxView::BLUE_PROPERTY);
@@ -412,7 +407,6 @@ public function handleRequest(AphrontRequest $request) {
412407
->setDiffUnitStatuses($broken_diffs)
413408
->setSelectedVersusDiffID($diff_vs)
414409
->setSelectedDiffID($target->getID())
415-
->setSelectedWhitespace($whitespace)
416410
->setCommitsForLinks($commits_for_links);
417411

418412
$local_table = id(new DifferentialLocalCommitsView())
@@ -1095,7 +1089,7 @@ private function buildRawDiffResponse(
10951089
// this ends up being something like
10961090
// D123.diff
10971091
// or the verbose
1098-
// D123.vs123.id123.whitespaceignore-all.diff
1092+
// D123.vs123.id123.highlightjs.diff
10991093
// lame but nice to include these options
11001094
$file_name = ltrim($request_uri->getPath(), '/').'.';
11011095
foreach ($request_uri->getQueryParamsAsPairList() as $pair) {

‎src/applications/differential/parser/DifferentialChangesetParser.php

Lines changed: 2 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ final class DifferentialChangesetParser extends Phobject {
1919
protected $specialAttributes = array();
2020

2121
protected $changeset;
22-
protected $whitespaceMode = null;
2322

2423
protected $renderCacheKey = null;
2524

@@ -163,7 +162,6 @@ public static function getDefaultRendererForViewer(PhabricatorUser $viewer) {
163162
}
164163

165164
public function readParametersFromRequest(AphrontRequest $request) {
166-
$this->setWhitespaceMode($request->getStr('whitespace'));
167165
$this->setCharacterEncoding($request->getStr('encoding'));
168166
$this->setHighlightAs($request->getStr('highlight'));
169167

@@ -191,20 +189,14 @@ public function readParametersFromRequest(AphrontRequest $request) {
191189
return $this;
192190
}
193191

194-
const CACHE_VERSION = 12;
192+
const CACHE_VERSION = 13;
195193
const CACHE_MAX_SIZE = 8e6;
196194

197195
const ATTR_GENERATED = 'attr:generated';
198196
const ATTR_DELETED = 'attr:deleted';
199197
const ATTR_UNCHANGED = 'attr:unchanged';
200-
const ATTR_WHITELINES = 'attr:white';
201198
const ATTR_MOVEAWAY = 'attr:moveaway';
202199

203-
const WHITESPACE_SHOW_ALL = 'show-all';
204-
const WHITESPACE_IGNORE_TRAILING = 'ignore-trailing';
205-
const WHITESPACE_IGNORE_MOST = 'ignore-most';
206-
const WHITESPACE_IGNORE_ALL = 'ignore-all';
207-
208200
public function setOldLines(array $lines) {
209201
$this->old = $lines;
210202
return $this;
@@ -336,11 +328,6 @@ public function setChangeset(DifferentialChangeset $changeset) {
336328
return $this;
337329
}
338330

339-
public function setWhitespaceMode($whitespace_mode) {
340-
$this->whitespaceMode = $whitespace_mode;
341-
return $this;
342-
}
343-
344331
public function setRenderingReference($ref) {
345332
$this->renderingReference = $ref;
346333
return $this;
@@ -574,10 +561,6 @@ public function isUnchanged() {
574561
return idx($this->specialAttributes, self::ATTR_UNCHANGED, false);
575562
}
576563

577-
public function isWhitespaceOnly() {
578-
return idx($this->specialAttributes, self::ATTR_WHITELINES, false);
579-
}
580-
581564
public function isMoveAway() {
582565
return idx($this->specialAttributes, self::ATTR_MOVEAWAY, false);
583566
}
@@ -624,18 +607,8 @@ protected function processHighlightedSource($data, $result) {
624607
}
625608

626609
private function tryCacheStuff() {
627-
$whitespace_mode = $this->whitespaceMode;
628-
switch ($whitespace_mode) {
629-
case self::WHITESPACE_SHOW_ALL:
630-
case self::WHITESPACE_IGNORE_TRAILING:
631-
case self::WHITESPACE_IGNORE_ALL:
632-
break;
633-
default:
634-
$whitespace_mode = self::WHITESPACE_IGNORE_MOST;
635-
break;
636-
}
610+
$skip_cache = false;
637611

638-
$skip_cache = ($whitespace_mode != self::WHITESPACE_IGNORE_MOST);
639612
if ($this->disableCache) {
640613
$skip_cache = true;
641614
}
@@ -648,8 +621,6 @@ private function tryCacheStuff() {
648621
$skip_cache = true;
649622
}
650623

651-
$this->whitespaceMode = $whitespace_mode;
652-
653624
$changeset = $this->changeset;
654625

655626
if ($changeset->getFileType() != DifferentialChangeType::FILE_TEXT &&
@@ -668,71 +639,10 @@ private function tryCacheStuff() {
668639
}
669640

670641
private function process() {
671-
$whitespace_mode = $this->whitespaceMode;
672642
$changeset = $this->changeset;
673643

674-
$ignore_all = (($whitespace_mode == self::WHITESPACE_IGNORE_MOST) ||
675-
($whitespace_mode == self::WHITESPACE_IGNORE_ALL));
676-
677-
$force_ignore = ($whitespace_mode == self::WHITESPACE_IGNORE_ALL);
678-
679-
if (!$force_ignore) {
680-
if ($ignore_all && $changeset->getWhitespaceMatters()) {
681-
$ignore_all = false;
682-
}
683-
}
684-
685-
// The "ignore all whitespace" algorithm depends on rediffing the
686-
// files, and we currently need complete representations of both
687-
// files to do anything reasonable. If we only have parts of the files,
688-
// don't use the "ignore all" algorithm.
689-
if ($ignore_all) {
690-
$hunks = $changeset->getHunks();
691-
if (count($hunks) !== 1) {
692-
$ignore_all = false;
693-
} else {
694-
$first_hunk = reset($hunks);
695-
if ($first_hunk->getOldOffset() != 1 ||
696-
$first_hunk->getNewOffset() != 1) {
697-
$ignore_all = false;
698-
}
699-
}
700-
}
701-
702-
if ($ignore_all) {
703-
$old_file = $changeset->makeOldFile();
704-
$new_file = $changeset->makeNewFile();
705-
if ($old_file == $new_file) {
706-
// If the old and new files are exactly identical, the synthetic
707-
// diff below will give us nonsense and whitespace modes are
708-
// irrelevant anyway. This occurs when you, e.g., copy a file onto
709-
// itself in Subversion (see T271).
710-
$ignore_all = false;
711-
}
712-
}
713-
714644
$hunk_parser = new DifferentialHunkParser();
715-
$hunk_parser->setWhitespaceMode($whitespace_mode);
716645
$hunk_parser->parseHunksForLineData($changeset->getHunks());
717-
718-
// Depending on the whitespace mode, we may need to compute a different
719-
// set of changes than the set of changes in the hunk data (specifically,
720-
// we might want to consider changed lines which have only whitespace
721-
// changes as unchanged).
722-
if ($ignore_all) {
723-
$engine = new PhabricatorDifferenceEngine();
724-
$engine->setIgnoreWhitespace(true);
725-
$no_whitespace_changeset = $engine->generateChangesetFromFileContent(
726-
$old_file,
727-
$new_file);
728-
729-
$type_parser = new DifferentialHunkParser();
730-
$type_parser->parseHunksForLineData($no_whitespace_changeset->getHunks());
731-
732-
$hunk_parser->setOldLineTypeMap($type_parser->getOldLineTypeMap());
733-
$hunk_parser->setNewLineTypeMap($type_parser->getNewLineTypeMap());
734-
}
735-
736646
$hunk_parser->reparseHunksForSpecialAttributes();
737647

738648
$unchanged = false;
@@ -753,7 +663,6 @@ private function process() {
753663
$this->setSpecialAttributes(array(
754664
self::ATTR_UNCHANGED => $unchanged,
755665
self::ATTR_DELETED => $hunk_parser->getIsDeleted(),
756-
self::ATTR_WHITELINES => !$hunk_parser->getHasTextChanges(),
757666
self::ATTR_MOVEAWAY => $moveaway,
758667
));
759668

@@ -971,10 +880,6 @@ public function render(
971880
pht('The contents of this file were not changed.'),
972881
$type);
973882
}
974-
} else if ($this->isWhitespaceOnly()) {
975-
$shield = $renderer->renderShield(
976-
pht('This file was changed only by adding or removing whitespace.'),
977-
'whitespace');
978883
} else if ($this->isDeleted()) {
979884
$shield = $renderer->renderShield(
980885
pht('This file was completely deleted.'));

0 commit comments

Comments
 (0)