Page MenuHomePhabricator

`arc patch`: Directory move in SVN patch workflow is implemented erroneously
Open, Needs TriagePublic

Description

SVN patch workflow outline is the following:

  1. create directories for copy destinations, patches, additions
  2. copy stuff
  3. patch stuff
  4. add stuff
  5. delete stuff
  6. ...

The issue comes if directories are moved, e.g. refactoring of packages in Java. The premature creation in step 1 will result in erroneous directory structure due to the behavior of svn cp:

From: http://kera.name/articles/2012/08/why-caution-is-advised-when-svn-copying-directories/

I did a bit of research in the list archive, and came across this snippet,
posted by someone basically saying this was reasonable behaviour because
that's how *nix does it:

cp path/A path/B --> if path/B doesn't exist, creates path/B

if path/B does    exist, creates path/B/A

I case of directory move src/com/myname/p1 -> src/com/myname/p2, the above steps will produce this:

  1. create src/com/myname/p2
  2. svn cp src/com/myname/p1 src/com/myname/p2 (which results src/com/myname/p2/p1 because p2 already exists at the destination)

Proposed process:

  1. collect directory and file copies in different collections
  2. execute dir copies with svn cp --parents $src $dst
  3. create dirs for file copies, patches, additions
  4. execute file copies with same routine as the dirs
  5. ...
  6. add the --force flag to svn add to avoid false warnings that a given path is already under version control

I already have implementation for this.

References:
http://svn.haxx.se/users/archive-2007-04/1035.shtml
http://kera.name/articles/2012/08/why-caution-is-advised-when-svn-copying-directories/

Event Timeline

andrask updated the task description. (Show Details)
andrask added a project: Differential.
andrask edited projects, added Arcanist; removed Differential.
andrask added a subscriber: andrask.
avivey renamed this task from Directory move in SVN patch workflow is implemented erroneously to `arc patch`: Directory move in SVN patch workflow is implemented erroneously.Nov 14 2015, 9:05 PM
avivey added a project: Subversion.

Is there anything I could do to facilitate discussion or acception of this proposal?

@andrask: I couldn't quite understand what exactly is going on, and what is the expected behavior.

Can you create a diff against rSVNTEST that behaves wrongly when applied with arc patch?

@avivey I'm not sure how I can create a patch for that repo as it has no valid .arcconfig and I don't have the config for the whole Phabricator project. So I just created an svn diff that you can apply locally and then create the patch with your setup. I uploaded it here: https://secure.phabricator.com/F989152

I moved dir2 under dir1. After you create the arc diff, revert everything locally. Then call arc patch.

You'll see that dir2 will be at dir1/dir2/dir2 instead of dir1/dir2. And the patch will not apply cleanly, even if it should.

If you repeat the same with arcanist patched with my modifications, everything will apply cleanly.

@joshuaspence what do you think about this issue?

I know very little about Subversion, so unfortunately I don't have any input here.