Page MenuHomePhabricator

Differential display of revisions with "svn cp x@12345 x" is inaccurate
Open, LowPublic

Description

Ported from FB 470019.

epriestley> We should probably fix this at some point but nearly no one is impacted and the damage is very small.

There is an issue with, roughly, "svn rm x; svn commit; svn cp x@12345 y; touch x; svn add x; arc diff", creating a diff where a path was both a copy origin (at a different rev) and a new file.

I pushed this to fix the immediate issue (arc failing with an exception):

https://github.com/facebook/arcanist/commit/f071a552667f2d1d58582ff63c3f4e6c70e808ea

Differential does not display the diff completely correctly. The files should say "this file was not changed _click here to view contents_", not "This file was changed only by adding or removing trailing whitespace.". The data is present in the database so this is just a display problem.

Putting it at low-pri since it shouldn't really block anything too badly (reviewers can go look in Diffusion for file content; inelegant but doable) and this will be easiest to fix when I bring Differential over to open source.

Related Objects

Event Timeline

jungejason added a subscriber: jungejason.

I don't think so -- it's a Differential problem, not a Diffusion problem.

In Diffusion, we pass around revision information everywhere, but in Differential we don't, so a change to path "A.php" generates a "CHANGE" for file "A.php", but then if you "svn cp A.php@123" that generates a "MOVE_AWAY" for "A.php" even though they're different files.

The referenced commit hacks this up by actually generating a "MOVE_AWAY" for "A.php@123", but this would collide with modifying a file literally called A.php@123, and copying that file would then generate A.php@123@456, etc. A "fix" here would be to always put "@" after every name and follow it with metadata, then strip it later, but this would confuse a number of display layers right now.

The other problem referenced is at least partially caused by T181.

chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 4:35 AM