Re: 1.6 vs. 1.8: strange behavior of 'svn diff -cN WC-FILE' if the file was created in rev N by copying

On Thu, Jun 27, 2013 at 9:38 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Johan Corveleyn [mailto:jcorvel_at_gmail.com]
>> Sent: woensdag 26 juni 2013 23:53
>> To: Johan Corveleyn; Ben Reser; Tobias Bading; Peter Samuelson; Subversion
>> Development
>> Subject: Re: 1.6 vs. 1.8: strange behavior of 'svn diff -cN WC-FILE' if
> the file
>> was created in rev N by copying
>>
>> On Wed, Jun 26, 2013 at 3:01 PM, Stefan Sperling <stsp_at_elego.de> wrote:
>> > On Wed, Jun 26, 2013 at 01:39:59PM +0200, Johan Corveleyn wrote:
>> >> As Tobias pointed out, 'svn diff' does indeed do the diff against the
>> >> copy source, but *only if you're lucky*. I would consider this erratic
>> >> behavior definitely a bug, because it makes the output unpredictable,
>> >> and thus unusable in general.
>> >
>> > OK, you've both convinced me now. I agree that diff -c could trace
>> > copyfrom even if the copfrom rev is outside the operative revision
>> > range, and display an 'added' file only with --show-copies-as-adds.
>> >
>> > Now, the problem is that the repos->repos diff case doesn't support
>> > --show-copies-as-adds yet (see do_diff() in libsvn_client/diff.c).
>> > So it's not a trivial fix and might require some work to get done.
>>
>> Perhaps we should regard that as a separate problem. Isn't the fact
>> that --show-copies-as-adds doesn't work for repos->repos diff,
>> orthogonal to fixing the default behavior (non-show-copies-as-adds)
>> regarding moved files?
>>
>> Or do you feel that the change in behavior ("fix") of diff -c is
>> dependent on a good working --show-copies-as-adds?
>
> We can't just redefine the behavior of '-c'
> The 'svn' commandline client explicitly implements '-c' as '-r N-1:N' (or -r
> N:N-1 when passing -c -N)

Maybe I gave the wrong impression, but I'm not trying to completely
redefine the behavior of 'diff -c'. However the current behavior, when
looking at a moved file, is unpredictable: it depends on whether or
not the move source's revision was exactly 1 revision before the
move-commit. That unpredictability is not good.

Either we always show the diff relative to it's copy source, or we
always show it as a complete add. Not sometimes this and sometimes
that.

If we resolve this inconsistency by always diffing against the copy
source, this happens to solve the problem of the OP. If we resolve it
the other way, that's fine too, and then I'll suggest adding an option
--diff-copy-from (cf. 'svnlook diff') to get the other behavior (and
solve the problem of the OP).

> This behavior is tied to more commands than diff and the internal APIs just
> see the revision numbers.
>
> Besides
> $ svn diff -c 12 FILE
> has many use cases
>
> what would you expect after:
> $ svn rm FILE
> $ svn cp OTHER-FILE_at_6 FILE
> $ svn ci -m "r12"
> (copy file with some history)

Interestingly, this has the same problem. If you do the copy from
OTHER-FILE_at_11 the result is currently different from if you do the
copy from OTHER-FILE_at_6.

Two minimal recipes to demonstrate (find the difference):

1) Copy from N-1

    svnadmin create repos
    svn co file:///c:/temp/svntest5/repos wc
    cd wc
    echo other line 1 > other.txt
    svn add other.txt
    svn ci -m "r1"
    echo line 1 > file.txt
    svn add file.txt
    svn ci -m "r2"
    svn up
    svn rm file.txt
    svn cp other.txt file.txt
    echo other line 2 >> file.txt
    svn ci -m "r3"

2) Copy from N-2

    svnadmin create repos
    svn co file:///c:/temp/svntest6/repos wc
    cd wc
    echo other line 1 > other.txt
    svn add other.txt
    svn ci -m "r1"
    echo line 1 > file.txt
    svn add file.txt
    svn ci -m "r2"
    svn rm file.txt
    svn cp other.txt file.txt
    echo other line 2 >> file.txt
    svn ci -m "r3"

In scenario 1 (copy from N-1) you'll get the diff against the copy
source with 'svn diff -c3 file.txt' (but only if you give 'file.txt'
as explicit target --- if you give no explicit target, the copyfrom
isn't taken into account ... even more confusing :-) --- perhaps this
difference is intentional ... I don't know).

[[[
C:\Temp\svntest5\wc>svn diff -c3 file.txt
Index: other.txt
===================================================================
--- other.txt (.../other.txt) (revision 2)
+++ other.txt (.../file.txt) (revision 3)
@@ -1 +1,2 @@
 other line 1
+other line 2

C:\Temp\svntest5\wc>svn diff -c3
Index: file.txt
===================================================================
--- file.txt (revision 2)
+++ file.txt (revision 3)
@@ -1 +1,2 @@
-line 1
+other line 1
+other line 2

C:\Temp\svntest5\wc>svn diff -c3 --notice-ancestry
Index: file.txt
===================================================================
--- file.txt (revision 2)
+++ file.txt (revision 3)
@@ -1 +0,0 @@
-line 1
Index: file.txt
===================================================================
--- file.txt (revision 0)
+++ file.txt (revision 3)
@@ -0,0 +1,2 @@
+other line 1
+other line 2
]]]

In scenario 2 (copy from N-2) the output of 'svn diff -c 3 file.txt'
is different (the other outputs remain the same):

[[[
C:\Temp\svntest6\wc>svn diff -c 3 file.txt
Index: file.txt
===================================================================
--- file.txt (revision 2)
+++ file.txt (revision 3)
@@ -1 +1,2 @@
-line 1
+other line 1
+other line 2
]]]

> or after:
> $ svn rm FILE
> echo "QQQ" > FILE
> $ svn add FILE
> $ svn ci -m "r12"
> (add new file)
>
> $ echo new line >> FILE
> $ svn ci -m "r12"
> (text change)
>
> [There are even more variants possible if you replace an ancestor instead of
> just the node itself]
>
> We can't handle all these use cases with a simple -r N-1:N. We really need
> more flags to capture all these cases.
>
> 'svn diff' is already implemented as layer over layer over layer of use
> cases.
>
> E.g. by default diff doesn't describe the ancestry of nodes: A file replaced
> by a file or a directory by a directory is described as a simple content
> change unless you pass --notice-ancestry (or --git). Which is nice if you
> want to use 'svn diff' as input for GNU patch, but not if you want it to
> describe the local changes.
>
>
>
> Another interesting case is that our merge is also build on top of diff, so
> users would expect that
> svn diff ^/trunk -c 1234
>
> Would be the same as what you merge with
> svn merge ^/trunk -c 1234

Okay, now I wonder how merge will handle the unpredictable output as
shown above. I hope the result will be the same in both scenarios
(possibly the built-in conflict resolution of merge will absorb the
difference). No time to dig into that right now though.

> (And then of course there is that problem of backwards compatibility)

Okay, but that only goes so far when we're walking the thin line
between feature and bug.

> If you really want to make your head explode ...

Not right now, thanks :-).

> Redefining any of this requires taking into account all these other ugly
> corner cases... and our backwards compatibility guarantees...
>
>
> (Small steps forward...)

Okay, gotcha. It may be (vastly) more difficult than I thought to
change any of this. But I'd like to at least find consensus that the
existing (erratic) behavior can / should be improved, regardless of
how hard/risky it would be to change it. If we keep the scope of the
problem we're trying to solve very limited, I think it should be
doable without breaking things.

(note: this is not a personal itch of mine, I have no direct impact of
this issue ... I'm just trying to mediate in this discussion (and I
got involved because I noted the different behavior of 'svnlook diff'
and its --diff-copy-from option))

--
Johan