Arc diff misses files after svn merge for new versions of svn

Overview

arc diff does not show all files after I use svn merge. This bug was caused by some change in svn between version 1.6 and 1.14. I have done analysis and would like to discuss potential fixes.

Note that I sometimes also see this bug after svn merge. But I think they can be tracked separately.

Environment

Windows
svn version 1.14

Reproduction Instructions

# Create a workspace.
mkdir C:\tmp\repos
mkdir C:\tmp\workspace

# Set up the new repository.
svnadmin create C:\tmp\repos\new
svn mkdir -m "Create directory structure." ^
  file:///C:/tmp/repos/new/trunk ^
  file:///C:/tmp/repos/new/branches ^
  file:///C:/tmp/repos/new/tags 

# Add some nested content on a feature branch.
svn copy file:///C:/tmp/repos/new/trunk file:///C:/tmp/repos/new/branches/feature1 -m "create feature branch"
cd C:\tmp\workspace
svn checkout file:///C:/tmp/repos/new/branches/feature1 new/feature1
cd new/feature1
mkdir new_folder
echo "Hello, World!" > new_folder/test.txt
svn add *
svn commit -m "create content in branch"

# Merge new content to trunk (use svn revision when branch was created - get from svn log).
cd C:\tmp\workspace
svn checkout file:///C:/tmp/repos/new/trunk new/trunk
svn merge -r2:HEAD file:///C:/tmp/repos/new/branches/feature1 new/trunk

# Add a .arcconfig file from your own Phabricator install.
# Then try to use arc diff.
arc diff --only

It creates a diff (assuming you aren’t blocked by this bug). But the diff only includes the new_folder. It does not include the test.txt file.

Included changes:
M (dir) .
A (dir) new_folder

When I follow these same steps for svn version 1.6, the diff includes the nested file.

Analysis

Arcanist builds the list of changed files in src/repository/api/Arcanist/SubversionAPI.php. The getSVNStatus function calls some variation on svn status.

When I follow these same repro steps for svn version 1.6, the status includes nested files:

C:\tmp\workspace\old\trunk>svn status
M .
A + new_folder
A + new_folder\test.txt

But for svn version 1.14, it only includes the folder:

C:\tmp\workspace\new\trunk>svn status
M .
A + new_folder

We can get the required information by passing -v to the svn command. Here is the xml snippet for this scenario (arcanist uses the XML output):

        <entry path="new_folder">
            <wc-status props="none" copied="true" item="added">
                <commit revision="3">
                    <author>USER</author>
                    <date>2021-04-15T20:02:12.905461Z</date>
                </commit>
            </wc-status>
        </entry>
        <entry path="new_folder\test.txt">
            <wc-status item="normal" props="none" copied="true">
                <commit revision="3">
                    <author>USER</author>
                    <date>2021-04-15T20:02:12.905461Z</date>
                </commit>
            </wc-status>
        </entry>

Note the nested file has item="normal" and copied="true".

I was able to get it working by modifying the getSVNStatus function in ArcanistSubversionAPI.php.

First pass the -v flag when we call svn status at the beginning of the getSVNStatus function. (I would include a diff, but I’m only allowed one image per post as a new user.)

Next:

image

This “works” in the sense that I am able to generate a diff that includes the nested file(s). However, I do have a few concerns:

  • I think the -v flag tells svn to include all files in the status output, even those that have not changed. This could impact performance for big repositories. If that is a concern, perhaps this could be an optional command-line flag to arc diff? Then arc diff should ideally print a hint if it detects this scenario to help users.
  • The “fixed” diff reports the nested file as added. Is that correct? It matches the old behavior in svn 1.6, so I would assume so.

Phabricator/Arcanist Version

I downloaded the latest version from github on 4/15/2021.

( arc version fails for me due to some other unrelated issues, trying to resolve so I can provide a more satisfactory answer here.)