You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/08/21 18:50:01 UTC
RE: svn commit: r1619452 - in /subversion/trunk/subversion: libsvn_client/diff.c tests/cmdline/diff_tests.py
> -----Original Message-----
> From: julianfoad@apache.org [mailto:julianfoad@apache.org]
> Sent: donderdag 21 augustus 2014 17:48
> To: commits@subversion.apache.org
> Subject: svn commit: r1619452 - in /subversion/trunk/subversion:
> libsvn_client/diff.c tests/cmdline/diff_tests.py
>
> Author: julianfoad
> Date: Thu Aug 21 15:47:37 2014
> New Revision: 1619452
>
> 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.
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...
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.
Bert
RE: svn commit: r1619452 - in /subversion/trunk/subversion: libsvn_client/diff.c tests/cmdline/diff_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: vrijdag 22 augustus 2014 13:48
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: 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@head):
>
> [[[
> $ svn cp tools/hook-scripts/mailer@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.
+1
The interesting thing is how we should improve it further.
>
> > 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@10 and trunk@20' followed
by
> a hunk actually containing a diff between branch@15 and trunk@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?
Agree.
What we currently show in the headers is very inconsistent.
For 1.9 I tried to cleanup the code that creates these paths, but we update
the values all over the place in the diff code, that calls the diff drivers.
(That is why we have those stupid output arguments and if I remember
correctly even a callback in the runner code now... I tried to refactor
everything out, but kept finding dependencies that need updating)
Bert
>
> - Julian
Re: svn commit: r1619452 - in /subversion/trunk/subversion: libsvn_client/diff.c tests/cmdline/diff_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
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@head):
[[[
$ svn cp tools/hook-scripts/mailer@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@10 and trunk@20' followed by a hunk actually containing a diff between branch@15 and trunk@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