You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2007/04/05 14:04:38 UTC
[PATCH] remove redundant code
Hi All,
In merge-cmd.c for rev_range_syntax style of merges we populate
peg_revision with default values.
This causes merge of the kind,
svn merge -r5:6 file:///path/to/repo/path1/path2 to always call
svn_client_merge_peg3 with a peg_revision which is either
'svn_opt_revision_head' or 'svn_opt_revision_working'.
This causes subversion/libsvn_client/merge.c:do_merge to make a
redundant call to resolve the same via 'svn_client__repos_locations'.
<snip from do_merge>
if (peg_revision->kind != svn_opt_revision_unspecified)
{
SVN_ERR(svn_client__repos_locations(&URL1, &revision1,
&URL2, &revision2,
NULL,
initial_path2 ? initial_path2
: initial_URL2,
peg_revision,
initial_revision1,
initial_revision2,
ctx, pool));
merge_b->url = URL2;
path1 = NULL;
path2 = NULL;
merge_b->path = NULL;
}
</snip>
We can avoid this redundant call with this patch.
I ran testsuite with and without this patch over ra_local/ra_svn/ra_dav
it works fine.
With regards
Kamesh Jayachandran
Re: [PATCH] remove redundant code
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Kamesh Jayachandran
> Sent: Friday, April 06, 2007 3:30 AM
> To: Paul Burba; Malcolm Rowe
> Cc: SVN Dev
> Subject: RE: Re: [PATCH] remove redundant code
>
> Nice point Paul.
>
> Yes this patch breaks the existing behaviour.
>
> But won't it be good to move this snippet of code(with a
> better comment) from svn binary to libsvn_client(as suggested
> by Malcom) so that other client implementation would be free
> from having this code.
Hi Kamesh,
Sorry, I didn't mean to imply I disagreed with Malcolm. As long as the
problem with the default URL peg revision doesn't occur in your
subsequent patch I see no problem.
Paul B.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove redundant code
Posted by Kamesh Jayachandran <ka...@collab.net>.
>>
>>> Was another patch ever submitted?
>>>
>>
>> Not that I could find.
>>
> Totally forgot about this.
>
> I will post one after taking care of the cases Paul mentioned.
>
> With regards
> Kamesh Jayachandran
Now I remember.
I was working on making libsvn_client/merge.c(erstwhile diff.c) not to
call 'svn_client__repos_locations' unless user provides a peg URL as
suggested by Malcom.
Then I got a mail from Paul
http://svn.haxx.se/dev/archive-2007-04/0292.shtml.
Then I realised the need for 'svn_client__repos_locations' in all the
cases as it ensures correct nodes to be merged.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove redundant code
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> Moving this code to libsvn_client/merge.c seems like a good
>> idea. I still see it in merge-cmd.c.
>>
>
> I have no objections to this part of the change, excepting what I noted
> here:
>
> http://svn.haxx.se/dev/archive-2007-04/0292.shtml
> http://svn.haxx.se/dev/archive-2007-04/0355.shtml
>
> (Maybe that isn't even a problem?)
>
> I'd even do this myself, but I don't quite see where exactly in
> libsvn_client to make the change.
>
>
>> Was another patch ever submitted?
>>
>
> Not that I could find.
>
Totally forgot about this.
I will post one after taking care of the cases Paul mentioned.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] remove redundant code
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Monday, June 11, 2007 4:56 PM
> To: Kamesh Jayachandran
> Cc: Paul Burba; Malcolm Rowe; SVN Dev
> Subject: Re: [PATCH] remove redundant code
>
> On Fri, 06 Apr 2007, Kamesh Jayachandran wrote:
>
> > Nice point Paul.
> >
> > Yes this patch breaks the existing behaviour.
> >
> > But won't it be good to move this snippet of code(with a
> > better comment) from svn binary to libsvn_client(as suggested
> > by Malcom) so that other client implementation would be free
> > from having this code.
>
> Moving this code to libsvn_client/merge.c seems like a good
> idea. I still see it in merge-cmd.c.
I have no objections to this part of the change, excepting what I noted
here:
http://svn.haxx.se/dev/archive-2007-04/0292.shtml
http://svn.haxx.se/dev/archive-2007-04/0355.shtml
(Maybe that isn't even a problem?)
I'd even do this myself, but I don't quite see where exactly in
libsvn_client to make the change.
> Was another patch ever submitted?
Not that I could find.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove redundant code
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 06 Apr 2007, Kamesh Jayachandran wrote:
> Nice point Paul.
>
> Yes this patch breaks the existing behaviour.
>
> But won't it be good to move this snippet of code(with a better comment) from svn binary to libsvn_client(as suggested by Malcom) so that other client implementation would be free from having this code.
Moving this code to libsvn_client/merge.c seems like a good idea. I
still see it in merge-cmd.c. Was another patch ever submitted?
RE: Re: [PATCH] remove redundant code
Posted by Kamesh Jayachandran <ka...@collab.net>.
Nice point Paul.
Yes this patch breaks the existing behaviour.
But won't it be good to move this snippet of code(with a better comment) from svn binary to libsvn_client(as suggested by Malcom) so that other client implementation would be free from having this code.
Thanks
With regards
Kamesh Jayachandran
RE: Re: [PATCH] remove redundant code
Posted by Paul Burba <pb...@collab.net>.
Hi Kamesh,
Unrelated to Malcolm's concerns I see a problem with your patch -- it
appears to cause the default URL peg revision for a URL merge source to
change. For example, starting with a vanilla greek tree:
# Make a branch
---------------
>set
URL=file:///C:/SVN/src-trunk/Release/subversion/tests/cmdline/svn-test-w
ork/repositories/merge_tests-1
>svn copy %URL%/A %URL%/A_COPY -m ""
Committed revision 2.
>svn up
A A_COPY
A A_COPY\B
A A_COPY\B\lambda
A A_COPY\B\E
A A_COPY\B\E\alpha
A A_COPY\B\E\beta
A A_COPY\B\F
A A_COPY\mu
A A_COPY\C
A A_COPY\D
A A_COPY\D\gamma
A A_COPY\D\G
A A_COPY\D\G\pi
A A_COPY\D\G\rho
A A_COPY\D\G\tau
A A_COPY\D\H
A A_COPY\D\H\chi
A A_COPY\D\H\omega
A A_COPY\D\H\psi
Updated to revision 2.
# Make a mod in the branch source as r3
---------------------------------------
>echo "new content" > A\B\E\alpha
>svn ci -m ""
Sending A\B\E\alpha
Transmitting file data .
Committed revision 3.
# Move the branch source in r4
------------------------------
>svn move %URL%/A %URL%/A_MOVED -m ""
Committed revision 4.
>svn up
D A
A A_MOVED
A A_MOVED\B
A A_MOVED\B\lambda
A A_MOVED\B\E
A A_MOVED\B\E\alpha
A A_MOVED\B\E\beta
A A_MOVED\B\F
A A_MOVED\mu
A A_MOVED\C
A A_MOVED\D
A A_MOVED\D\gamma
A A_MOVED\D\G
A A_MOVED\D\G\pi
A A_MOVED\D\G\rho
A A_MOVED\D\G\tau
A A_MOVED\D\H
A A_MOVED\D\H\chi
A A_MOVED\D\H\omega
A A_MOVED\D\H\psi
Updated to revision 4.
# Merge from URL which doesn't exist
# at implied peg rev of HEAD (r4).
# Without your patch we get the
# expected error...
------------------------------------
>svn merge %URL%/A/B A_COPY\B -c3
svn: File not found: revision 4, path '/A/B'
# ...But with your patch the
# merge "works" when it shouldn't no?
-------------------------------------
>svn merge %URL%/A/B A_COPY\B -c3
U A_COPY\B\E\alpha
It gets even stranger if %URL%/A/B exists at HEAD but doesn't share
history with %URL%/A/B@2...
Paul B.
> -----Original Message-----
> From: Malcolm Rowe [mailto:malcolm-svn-dev@farside.org.uk]
> Sent: Thursday, April 05, 2007 12:09 PM
> To: Kamesh Jayachandran
> Cc: SVN Dev
> Subject: Re: [PATCH] remove redundant code
>
> On Thu, Apr 05, 2007 at 09:28:08PM +0530, Kamesh Jayachandran wrote:
> >
> > >It would make more sense to fix libsvn_client to not do
> the redundant
> > >resolution in the first place rather than work around the
> problem in
> > >the 'svn' binary.
> > >
> > >Regards,
> > >Malcolm
> > >
> >
> > I think you misunderstood. The code in libsvn_client/merge.c is
> > correct it exists to handle the cases like svn merge -r5:6
> > file:///path/to/repo/path1/path2@{2007-03-08}
> >
> > The fix is to avoid libsvn_client/merge.c doing redundant
> work because
> > of unwanted input svn gives all the time.
> >
>
> Yes, and I'm suggesting that libsvn_client should be clever
> enough to realise that a merge of file:///path@HEAD or
> path@BASE shouldn't require an extra resolution step to get
> the peg revision.
>
> In other words, I'd like to fix the problem for all clients,
> not just one.
>
> Regards,
> Malcolm
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove redundant code
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Apr 05, 2007 at 09:28:08PM +0530, Kamesh Jayachandran wrote:
>
> >It would make more sense to fix libsvn_client to not do the redundant
> >resolution in the first place rather than work around the problem in the
> >'svn' binary.
> >
> >Regards,
> >Malcolm
> >
>
> I think you misunderstood. The code in libsvn_client/merge.c is correct
> it exists to handle the cases like svn merge -r5:6
> file:///path/to/repo/path1/path2@{2007-03-08}
>
> The fix is to avoid libsvn_client/merge.c doing redundant work because
> of unwanted input svn gives all the time.
>
Yes, and I'm suggesting that libsvn_client should be clever enough to
realise that a merge of file:///path@HEAD or path@BASE shouldn't require
an extra resolution step to get the peg revision.
In other words, I'd like to fix the problem for all clients, not just
one.
Regards,
Malcolm
Re: [PATCH] remove redundant code
Posted by Kamesh Jayachandran <ka...@collab.net>.
> It would make more sense to fix libsvn_client to not do the redundant
> resolution in the first place rather than work around the problem in the
> 'svn' binary.
>
> Regards,
> Malcolm
>
I think you misunderstood. The code in libsvn_client/merge.c is correct
it exists to handle the cases like svn merge -r5:6
file:///path/to/repo/path1/path2@{2007-03-08}
The fix is to avoid libsvn_client/merge.c doing redundant work because
of unwanted input svn gives all the time.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove redundant code
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Apr 05, 2007 at 07:34:38PM +0530, Kamesh Jayachandran wrote:
> Hi All,
> In merge-cmd.c for rev_range_syntax style of merges we populate
> peg_revision with default values.
>
>
> This causes subversion/libsvn_client/merge.c:do_merge to make a
> redundant call to resolve the same via 'svn_client__repos_locations'.
>
> --- subversion/svn/merge-cmd.c (revision 24447)
> +++ subversion/svn/merge-cmd.c (working copy)
> @@ -77,11 +77,6 @@
> pool));
> sourcepath2 = sourcepath1;
>
> - /* Set the default peg revision if one was not specified. */
> - if (peg_revision.kind == svn_opt_revision_unspecified)
> - peg_revision.kind = svn_path_is_url(sourcepath1)
> - ? svn_opt_revision_head : svn_opt_revision_working;
> -
It would make more sense to fix libsvn_client to not do the redundant
resolution in the first place rather than work around the problem in the
'svn' binary.
Regards,
Malcolm