Page MenuHomePhabricator

Phabricator can't process commits which delete files from a relocated directory
Open, Needs TriagePublic

Description

PhabricatorRepositorySvnCommitChangeParserWorker can't process commits which delete a file from a relocate directory (if the directory also didn't exist in the previous commit). The problem is that the relocated directory path doesn't get added to the $add_paths variable so phabricator tries to look up the deleted from in the same location from the previous commit rather than in the location from which it was moved from.

To get an example of this is quite tricky so I've set up a demo svn repo here: https://subversion.assembla.com/svn/phabricator-example/

The basic steps to create your own are:

svn co $some_repo
mkdir ./trunk/foo
svn add ./trunk/foo
svn commit -m "Created directory."
svn rm ./trunk/foo
svn commit -m "Deleted directory."
mkdir ./trunk/foo
mkdir ./trunk/foo/bar
touch ./trunk/foo/bar/bad-file.txt
svn add ./trunk/foo
svn commit -m "Recreated directory."
svn rm ./trunk/foo/bar/bad-file.txt
svn commit -m "Deleted bad file."

mkdir ./branches/new-branch
svn add ./branches/new-branch
svn commit -m "Created a new branch."
cd ./branches/new-branch
svn merge -c 2 $some_repo/trunk
svn commit -m "Merge commit 2."
svn merge -c 3,4,5 $some_repo/trunk
svn commit -m "Bad commit: merge commits 3, 4 and 5."

Event Timeline

datr raised the priority of this task from to Needs Triage.
datr updated the task description. (Show Details)
datr added a project: Phabricator.
datr added a subscriber: datr.

Sorry, super busy for the last couple of days. I expect to be able to take a look at this tomorrow. Thanks for all the work in building a repro case!

I found a (possibly related?) type of change which still doesn't parse for me, even with this patch:

mkdir q_dir
touch q_dir/a
touch q_dir/b
touch q_file
svn commit -m "commit 1"
svn rm q_file
svn mv q_dir q_file
svn rm q_file/b
svn commit -m "commit 2"

Or, technically, this parses, but the web UI errors when trying to pull patches for it.

I'm going to pull D7432 since I think it improves things without breaking anything, but I'm going to leave this task open until we can get some actual test coverage on this stuff and fix the case above. Coverage is conceptually simple and we nearly have the infrastructure for it, and I think it will go a long way toward making this parsing robust.

After T4327 we have coverage here, but fixing this seemed fairly tricky. This can be attacked now, at least.