Re: svn commit: r1619452 - in /subversion/trunk/subversion: libsvn_client/diff.c tests/cmdline/diff_tests.py

Bert Huijben wrote:

> Julian Foad wrote:
>> URL: http://svn.apache.org/r1619452
>> Log:
>> Fix the copy-from revision number reported in a diff header. It previously
>> reported 'nonexistent' instead of the copy-from revision in some cases.
>>
>> This bug seems to be a regression; 1.8.x releases show the correct revision.
>
> 1.8.x just reported the original/left revision in a lot of cases where it
> didn't know the revision.

You're right -- 1.8.x inconsistently showed the copy-from revision in only some cases, and "(revision 0)" in some cases, and possibly other things in other cases, when diffing a copied node. For example (running trunk_at_head):

[[[
$ svn cp tools/hook-scripts/mailer_at_1619388 hs

  # remove the uninteresting mergeinfo addition
$ svn propdel svn:mergeinfo hs

$ svn propedit p-new hs hs/mailer.py

$ svn18 diff hs/
Index: hs/mailer.py
===================================================================
--- hs/mailer.py    (revision 1619388)
+++ hs/mailer.py    (working copy)

Property changes on: hs/mailer.py
___________________________________________________________________
Added: p-new
## -0,0 +1 ##
+v
Index: hs
===================================================================
--- hs    (revision 0)
+++ hs    (working copy)

Property changes on: hs
___________________________________________________________________
Added: svn:ignore
## -0,0 +1 ##
+mailer.conf
Added: p-new
## -0,0 +1 ##
+v
]]]

So I shouldn't describe this as a regression.

> Especially on directories where 1.8.x where the old internal diff callbacks
> didn't even report any revision... where the diff output just guessed the
> revision based on the revision of the complete diff (or whatever it expected it
> to be).
>
>
> I don't think we ever reported the copy from revision, as the actual
> left-src revision of the diff...

Yes we did -- at least in cases like 'mailer.py' in the example above.

> And I'm not sure if reporting copy from is really valid either, without
> reporting that it is a copy and probably more importantly where the copy is
> from.
>
> The copy can be from any repository path at the specified revision...
> So now you can no longer trust the revision to just specify that the path
> existed at the current path in that revision.

Well, yes... It would be better if the rev and the path matched each other.

It's all quite a mess. I think it now always shows the copy-from rev when diffing a copy (and 'nonexistent' when showing a copy as an add). It's now more consistent than it was, in that way.

It's stupid to show a diff whose header doesn't match the diff hunks -- a header that says 'this is a diff between trunk_at_10 and trunk_at_20' followed by a hunk actually containing a diff between branch_at_15 and trunk_at_20.

We should show header info (paths and revisions) that matches the diff hunks. So, when we are displaying diffs against copy-from, the 'left' header should show the copy-from. We haven't done this before but we should make it so.

Agree?

- Julian
Received on 2014-08-22 13:48:28 CEST