Remove all whitespace options/configuration everywhere
ClosedPublic
Actions

Authored by epriestley on Feb 16 2019, 2:16 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T3498: Allow ignore whitespace in Audit/Diffusion diffs
T13161: Plans: Improve Differential diff display behaviors

Commits

rP5310f1cdd90e: 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.

Diff Detail

Repository

rP Phabricator

Branch

xspace2

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 22039
Build 30106: Run Core Tests
Build 30105: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	57 ms	DifferentialParseRenderTestCase::Unknown Unit Message ("") Assertion failed, expected values to be equal (at DifferentialParseRenderTestCase.php:56): whitespace.diff.one.expect Expected vs Actual Output Diff --- Old Value
	1 ms	AlmanacNamesTestCase::Unknown Unit Message ("") 30 assertions passed.
	0 ms	AlmanacServiceTypeTestCase::Unknown Unit Message ("") 1 assertion passed.
	0 ms	AphrontHTTPSinkTestCase::Unknown Unit Message ("") 2 assertions passed.
	1 ms	AphrontHTTPSinkTestCase::Unknown Unit Message ("") 1 assertion passed.
		View Full Test Results (1 Failed · 398 Passed)

Event Timeline

epriestley created this revision.Feb 16 2019, 2:16 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 16 2019, 2:18 PM

Harbormaster failed remote builds in B22039: Diff 48192!

Tests depend on D20182.

epriestley added inline comments.Feb 16 2019, 2:49 PM

src/applications/differential/parser/DifferentialChangesetParser.php
718–734	See T13161. This is basically the piece I'm going to restore in the next diff to improve line alignment when lines changes only by changing whitespace, so this is coming back in some form soon.
src/applications/differential/parser/DifferentialHunkParser.php
318–331	This piece may need to come back, too.

epriestley added a child revision: D20187: Put some whitespace behaviors back, but only for "diff alignment", not display.Feb 16 2019, 4:31 PM

amckinley accepted this revision.Feb 19 2019, 8:21 PM

amckinley added inline comments.

src/applications/differential/__tests__/data/generated.diff.one.unshielded
1–5	This isn't really a generated file; it's just designed to appear as one, right? Presumably there isn't a very good way to actually test this stuff without also incidentally adding a "This is a generated file which doesn't need to be reviewed" message when reviewing revisions that touch these files.

This revision is now accepted and ready to land.Feb 19 2019, 8:21 PM

epriestley added inline comments.Feb 19 2019, 8:38 PM

src/applications/differential/__tests__/data/generated.diff.one.unshielded
1–5	Right, this is testing the rendering of generated files. We could work around this with some mangling in the test harness, but it's such a weird special case that it doesn't seem terribly valuable.