The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file at all but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- Restricted Maniphest Task
T4599: Incorrect "only adding or removing whitespace" shield being applied to origin of moved and modified files. - Commits
- Restricted Diffusion Commit
rPdc1106a9b910: Differential - stop showing the shield for "move away" operations
made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/differential/parser/DifferentialChangesetParser.php | ||
|---|---|---|
| 86 | i could purge all caches but thought that was excessive? probably should | |
Comment Actions
I think leaving the caches is reasonable. This is pretty rare and this is the most expensive cache to rebuild.
I wonder if we should always shield these with a message like "This file was moved elsewhere." or "This file was moved to <destination link>"? We'll basically always show the whole file in red now, right? And show it again elsewhere in the diff? Or am I getting the behavior wrong?
This is less confusing/incorrect and strictly an improvement, in any case.
Comment Actions
Here's a very mature screenshot of the new behavior. If I hadn't edited the file in the new location, there would be a shield there to show the contents as they were unchanged.