Page MenuHomePhabricator

New files in svn not properly sent to differential
Closed, ResolvedPublic

Description

when running arc diff to send a review, each time a new file is in the diff then the said fail it shown in differential as if the patch was applied twice

see https://phabric.freebsd.org/D282 for example

Event Timeline

bapt raised the priority of this task from to Needs Triage.
bapt updated the task description. (Show Details)
bapt added projects: Arcanist, Differential.
bapt added a subscriber: bapt.

I'm not sure I understand, that links looks fine to me. Which file is showing incorrectly?

the file devel/pkg-gmake/Makefile is wrong
if you have a look you see it appears twice (the numbering of line restart to 1 after line 29)

What can I provide more to help diagnose this, or I could probably fix it myself if one can give me a direction where to look in the code?

I can't reproduce this:

$ svn st
A       new-file.txt
$ arc diff --only --conduit-uri=https://secure.phabricator.com/
Created a new Differential diff:
        Diff URI: https://secure.phabricator.com/differential/diff/23679/

Included changes:
  A       new-file.txt

Produces this: https://secure.phabricator.com/differential/diff/23679/

Some things that might be helpful:

  • What version of SVN do you have?
  • Does this happen if you arc diff --only, too?
  • If you arc diff --only --trace, you'll see all the commands arc is running. In particular, there should be a line like this:

    >>> [6] <exec> $ svn diff --depth empty --diff-cmd '/INSECURE/devtools/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'new-file.txt'
  • If you run your local version of that command in isolation, does it produce a reasonable-looking diff? Can you show me a copy?
  • If you pipe the diff that command produces into arc diff --raw, does that produce a normal or broken diff?

So I'm using svn 1.8.9 and I can reproduce on every svn 1.8 version (I haven't tried earlier version)
arc diff --only is giving this result: https://phabric.freebsd.org/differential/diff/651/ which is broken
with --trace I got the following:

<<< [4] <exec> 766,773 us
>>> [6] <exec> $ svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/Makefile'
>>> [7] <exec> $ svn info '/home/bapt/dev/src/usr.bin/Makefile'@
>>> [8] <exec> $ svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/timeout'
>>> [9] <exec> $ svn info '/home/bapt/dev/src/usr.bin/timeout'@
>>> [10] <exec> $ svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/timeout/Makefile'
>>> [11] <exec> $ svn info '/home/bapt/dev/src/usr.bin/timeout/Makefile'@
>>> [12] <exec> $ svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/timeout/timeout.1'
>>> [13] <exec> $ svn info '/home/bapt/dev/src/usr.bin/timeout/timeout.1'@
<<< [7] <exec> 14,540 us
>>> [14] <exec> $ svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/timeout/timeout.c'
<<< [11] <exec> 16,535 us
>>> [15] <exec> $ svn info '/home/bapt/dev/src/usr.bin/timeout/timeout.c'@
<<< [13] <exec> 16,676 us
<<< [6] <exec> 23,517 us
<<< [9] <exec> 24,467 us
<<< [12] <exec> 22,453 us
<<< [10] <exec> 26,674 us
<<< [8] <exec> 28,347 us
<<< [14] <exec> 18,868 us
<<< [15] <exec> 15,341 us
>>> [16] <exec> $ svn info .
<<< [16] <exec> 9,011 us
>>> [17] <conduit> arcanist.projectinfo() <bytes = 181>
>>> [18] <http> https://phabric.freebsd.org/api/arcanist.projectinfo
<<< [18] <http> 235,660 us
<<< [17] <conduit> 235,788 us
>>> [19] <conduit> repository.query() <bytes = 192>
>>> [20] <http> https://phabric.freebsd.org/api/repository.query
<<< [20] <http> 240,972 us
<<< [19] <conduit> 241,080 us
>>> [21] <conduit> differential.creatediff() <bytes = 43916>
>>> [22] <http> https://phabric.freebsd.org/api/differential.creatediff
<<< [22] <http> 990,618 us
<<< [21] <conduit> 990,733 us
>>> [23] <event> diff.wasCreated <listeners = 0>
<<< [23] <event> 37 us
Created a new Differential diff:
        Diff URI: https://phabric.freebsd.org/differential/diff/652/

Included changes:
  M       usr.bin/Makefile
  A (dir) usr.bin/timeout
  A       usr.bin/timeout/Makefile
  A       usr.bin/timeout/timeout.1
  A       usr.bin/timeout/timeout.c

This following command is giving me a normal diff
svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/timeout/timeout.c'

this is also getting a normal diff:
svn diff --depth empty --diff-cmd '/usr/local/lib/php/arcanist/scripts/repository/binary_safe_diff.sh' -x -U32767 'usr.bin/timeout/timeout.c' | arc diff --raw
see https://phabric.freebsd.org/differential/diff/653/
The following is also giving me a normal diff
arc diff --only usr.bin/timeout/timeout.c
see https://phabric.freebsd.org/differential/diff/654/

I also get a normal diff with:

arc diff --only usr.bin/timeout/timeout.c usr.bin/timeout/timeout.1
see https://phabric.freebsd.org/differential/diff/655/

but arc diff --only usr.bin/timeout/
gives me a wrong diff
https://phabric.freebsd.org/differential/diff/656/

So this seems to be related to when a new directory is added

We pass --depth empty to prevent svn diff subdir/ from including changes to files in the subdirectory, but apparently this doesn't do anything if the directory is new:

http://mail-archives.apache.org/mod_mbox/subversion-dev/201311.mbox/%3C528A6925.10402@wandisco.com%3E

D9921 should fix this.

I can confirm it fixes it, thank you very much!

epriestley claimed this task.