You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Senthil Kumaran S <se...@collab.net> on 2009/04/15 10:34:24 UTC
Question on fixing issue #3361 and #3294
Hi,
I was working on a patch (attached) to fix issue #3361, which also fixes issue
#3294. I found in the update editor API, there was a check to see if the
copyfrom_url and the wc entries url are same by checking the ancestral
relation, which will not work for urls with different schemes. I discussed
about it in IRC whose transcript is as follows:
<snip>
(02:12:08 PM) stylesen: can anyone tell me what is the use of copyfrom_url in wc?
(02:15:29 PM) ehu`: stylesen: sure.
(02:15:48 PM) ehu`: it's the url of the item which has an "A+" state.
(02:16:00 PM) ehu`: the source of it, that is
(02:31:57 PM) stylesen: ehu`: this copyfrom_url is recorded with the absolute
path in the wc entries? like "http://svn.collab.net/trunk/somedir"
(02:32:11 PM) ehu`: yes
(02:32:21 PM) stylesen: or the relative path from root is alone recorded
"trunk/somedir"
(02:32:30 PM) ehu`: although it should probably be "root relative"
(02:32:42 PM) ehu`: I think it's absolute.
(02:32:54 PM) gstein: it is absolute
(02:33:04 PM) ehu`: which is wrong.
(02:33:06 PM) ehu`: imo
(02:33:13 PM) ehu`: root relative is much better.
(02:33:17 PM) stylesen: yes me too feel the same
(02:33:31 PM) stylesen: we need to record only the relative path from the root
and not the absolute path
(02:33:51 PM) stylesen: this will help in mixing different schemes while
merging which am trying to solve
(02:34:02 PM) gstein: that is true if *and only if* the copyfrom can *only*
come from the same repository
(02:34:12 PM) gstein: in wc-ng, we allow copyfrom to specify arbitrary repositories
(02:35:00 PM) gstein: we specify it as a 4-tuple: repository root url,
repository uuid, relative path from root, and revision
(02:35:23 PM) ***ehu` was writing that's better
(02:35:23 PM) stylesen: in
"/subversion-dev/trunk/subversion/libsvn_wc/update_editor.c", line 5358I am
trying to change the if check, to check for uuid and not ancestral relation
(02:35:37 PM) stylesen: which fails on different url schemes
(02:36:21 PM) gstein: you do not have enough information at that point in the
code to fix that check
(02:36:52 PM) gstein: there is no copyfrom_uuid, so you will not be able to fix
that
(02:36:53 PM) stylesen: gstein: I can find the uuids of the two urls and check?
(02:37:08 PM) gstein: and you certainly do not want to hit the server to fetch
a uuid
(02:38:01 PM) stylesen: then I shall come up with "svn_wc_add_repos_file4"
which will take the uuids?
(02:38:24 PM) gstein: don't even bother with it
(02:39:13 PM) gstein: wait until the wc-ng is more complete
(02:39:32 PM) ehu`: stylesen: I don't think we should be fixing too many issues
in wc-1, given that we're switching to wc-ng in the "near" future.
(02:39:34 PM) stylesen: gstein: will wc-ng support different schemes?
(02:39:47 PM) gstein: it has a better mechanism for doing so, yes
(02:40:04 PM) gstein: and it *does* record uuids for all the repositories that
it knows about,
(02:40:14 PM) stylesen: am trying to fix issues 3361 and 3294
(02:40:18 PM) gstein: so it will be possible to see N different root URLs, but
know they all refer to the same repository
(02:40:47 PM) gstein: for example, it would know that svn.apache.org/repos/asf
and svn.eu.apache.org/repos/asf and https://svn.apache.org/repos/asf are ALL
the same repository
(02:40:52 PM) gstein: different scheme, different host, etc
(02:41:04 PM) stylesen: that sounds cool
(02:41:09 PM) gstein: you don't want to just allow different schemes,
(02:41:18 PM) stylesen: that should ultimately fix scheme matching
(02:41:20 PM) gstein: because the repository might have a different root url
(02:41:42 PM) gstein: it could be http://svn.example.com/root/ and
https://svn.example.com/protected/
(02:42:35 PM) stylesen: yes I get it, so we need not want this check in
libsvn_client/merge.c at line 1454, which I feel is not correct
(02:42:56 PM) stylesen: may be I shall fix this scheme checking for now in
merge.c code to use the uuid check
(02:43:32 PM) gstein: yes, that check is incorrect
(02:43:45 PM) stylesen: or just remove that check altogether, from merge.c
code, because we already know we are working on same_repos
(02:43:52 PM) gstein: urm. wait...
(02:44:04 PM) gstein: it is fine to compare that the two schemes *match*
(02:44:21 PM) gstein: but you cannot expect to replace the scheme and get the
same repository,
(02:44:48 PM) gstein: so yah: that check is actually fine. it ensures that you
use the same scheme.
(02:45:19 PM) gstein: stylesen: I don't know what that code is doing, so I
can't answer the question
(02:45:36 PM) stylesen: gstein: okie, let me have a closer look
(02:47:42 PM) gstein: ah. I see.
(02:47:56 PM) gstein: that check is necessary, but insufficient
(02:48:42 PM) gstein: I'd not worry about fixing it,
(02:48:55 PM) gstein: and assume it will be more easily fixed after some wc-ng
work completes
(02:49:45 PM) stylesen: gstein: can you elaborate on the insufficient part,
what are the other checks that needs to be done?
(02:50:53 PM) gstein: it appears that code constructs a new copyfrom_url,
(02:50:59 PM) gstein: but *only* checks the scheme,
(02:51:12 PM) gstein: when it should be checking the entire repository root url
(02:51:53 PM) gstein: i.e. it could allow http://svn.eu.apache.org/ when it
should *only* allow http://svn.apache.org/
(02:52:05 PM) gstein: ie. it isn't checking the hostname in this example
</snip>
The attached patch removes the check for scheme (of url) during a merge, which
IMHO is not required, since we already know we are operating with the same
repository and all the mergeinfo that is getting recorded is relative to the
root and not an absolute path. I may be missing something seriously here.
I would like to get more comments on this patch whether we can go ahead in
fixing this (which will benefit 1.6.x and previous versions) or wait for wc-ng
in order to get a cleaner fix for future and don't bother about previous releases.
Though this patch fixes these issues (and passes all tests), *it is not ready
to commit*, It poses a general idea in which I would like to fix these issues.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1725218
Re: Question on fixing issue #3361 and #3294
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Apr 16, 2009 at 09:51:21AM +0200, Bert Huijben wrote:
> Please, check whether the url is the the expected url based on the working
> copy before creating an RA session and only fall back when it is no match.
+1
And don't forget to ignore the URL scheme when comparing URLs :)
> Creating an RA session can be a huge performance hit if you are working on a
> repository that is thousands of miles away. (E.g. from Amsterdam to
> California, like my access of the Subversion repository). And this pain
> would be even more painful if the repository would use negotiation in the
> authentication phase.
>
> In that case just creating the RA session can be several web requests (and
> at that point you haven't got to the actual check).
Well, it looks like wc-ng is moving forward, so we won't have to
bother users with this for a long time, except for maybe some time
during the 1.6.x line.
Stefan
RE: Question on fixing issue #3361 and #3294
Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Senthil Kumaran S [mailto:senthil@collab.net]
> Sent: donderdag 16 april 2009 8:12
> To: dev@subversion.tigris.org
> Subject: Re: Question on fixing issue #3361 and #3294
>
> Hi Stefan,
>
> Stefan Sperling wrote:
> > On Wed, Apr 15, 2009 at 04:04:24PM +0530, Senthil Kumaran S wrote:
> >> (02:35:23 PM) stylesen: in
> >> "/subversion-dev/trunk/subversion/libsvn_wc/update_editor.c", line
5358I am
> >> trying to change the if check, to check for uuid and not ancestral
relation
> >> (02:35:37 PM) stylesen: which fails on different url schemes
> >> (02:36:21 PM) gstein: you do not have enough information at that point
in
> the
> >> code to fix that check
> >
> >> The attached patch removes the check for scheme (of url) during a
merge,
> which
> >> IMHO is not required, since we already know we are operating with the
same
> >> repository
> >
> > That statement seems to contradict what gstein said in the chat.
> > Which is correct?
>
> It checks whether the copyfrom URL is using the same scheme as the working
> copy
> repository url, just in order to record the correct copy from url in the
> entries, which has a redundant check in update editor too. Hence I think
this
> check is not necessary here, since we already do a check for same
repositories
> using the uuid, so it is safe to remove this check here.
>
> >> + /* ### TODO: Here we need to check copyfrom_url and ent->repos are
> from
> >> + ### the same repository by checking their UUIDs, which should
be
> >> + ### trivial in wc-ng and avoids opening an ra_session here. */
> >
> > Why is opening an RA session a problem?
> >
> > Sure, performance will suffer, but if we need that information to make
> > the check correct, we have to get it to make the check correct.
>
> For performance reasons only.
Please, check whether the url is the the expected url based on the working
copy before creating an RA session and only fall back when it is no match.
Creating an RA session can be a huge performance hit if you are working on a
repository that is thousands of miles away. (E.g. from Amsterdam to
California, like my access of the Subversion repository). And this pain
would be even more painful if the repository would use negotiation in the
authentication phase.
In that case just creating the RA session can be several web requests (and
at that point you haven't got to the actual check).
Bert
>
> > and forget about fixing the problem for wc-1. Otherwise, we go for the
> > RA session for now, and make the check use information available in
> > wc-ng as soon as possible, too.
>
> I shall try to create an ra session and make the commit.
>
> Thank You.
> --
> Senthil Kumaran S
> http://www.stylesen.org/
>
> ------------------------------------------------------
>
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=174
06
> 37
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1742370
Re: Question on fixing issue #3361 and #3294
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Stefan,
Stefan Sperling wrote:
> On Wed, Apr 15, 2009 at 04:04:24PM +0530, Senthil Kumaran S wrote:
>> (02:35:23 PM) stylesen: in
>> "/subversion-dev/trunk/subversion/libsvn_wc/update_editor.c", line 5358I am
>> trying to change the if check, to check for uuid and not ancestral relation
>> (02:35:37 PM) stylesen: which fails on different url schemes
>> (02:36:21 PM) gstein: you do not have enough information at that point in the
>> code to fix that check
>
>> The attached patch removes the check for scheme (of url) during a merge, which
>> IMHO is not required, since we already know we are operating with the same
>> repository
>
> That statement seems to contradict what gstein said in the chat.
> Which is correct?
It checks whether the copyfrom URL is using the same scheme as the working copy
repository url, just in order to record the correct copy from url in the
entries, which has a redundant check in update editor too. Hence I think this
check is not necessary here, since we already do a check for same repositories
using the uuid, so it is safe to remove this check here.
>> + /* ### TODO: Here we need to check copyfrom_url and ent->repos are from
>> + ### the same repository by checking their UUIDs, which should be
>> + ### trivial in wc-ng and avoids opening an ra_session here. */
>
> Why is opening an RA session a problem?
>
> Sure, performance will suffer, but if we need that information to make
> the check correct, we have to get it to make the check correct.
For performance reasons only.
> and forget about fixing the problem for wc-1. Otherwise, we go for the
> RA session for now, and make the check use information available in
> wc-ng as soon as possible, too.
I shall try to create an ra session and make the commit.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1740637
Re: Question on fixing issue #3361 and #3294
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 15, 2009 at 04:04:24PM +0530, Senthil Kumaran S wrote:
> (02:35:23 PM) stylesen: in
> "/subversion-dev/trunk/subversion/libsvn_wc/update_editor.c", line 5358I am
> trying to change the if check, to check for uuid and not ancestral relation
> (02:35:37 PM) stylesen: which fails on different url schemes
> (02:36:21 PM) gstein: you do not have enough information at that point in the
> code to fix that check
> The attached patch removes the check for scheme (of url) during a merge, which
> IMHO is not required, since we already know we are operating with the same
> repository
That statement seems to contradict what gstein said in the chat.
Which is correct?
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 37269)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -5355,12 +5355,9 @@
>
> new_URL = svn_path_url_add_component2(ent->url, base_name, pool);
>
> - if (copyfrom_url && ent->repos &&
> - ! svn_path_is_ancestor(ent->repos, copyfrom_url))
> - return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> - _("Copyfrom-url '%s' has different repository"
> - " root than '%s'"),
> - copyfrom_url, ent->repos);
> + /* ### TODO: Here we need to check copyfrom_url and ent->repos are from
> + ### the same repository by checking their UUIDs, which should be
> + ### trivial in wc-ng and avoids opening an ra_session here. */
Why is opening an RA session a problem?
Sure, performance will suffer, but if we need that information to make
the check correct, we have to get it to make the check correct.
Everyone already knows that wc-1 is slow, so the additional round trip
won't cause much surprises. It's the broken wc-1 design that is the
problem, and we can only try to work around it. That's all we can do.
> (02:39:32 PM) ehu`: stylesen: I don't think we should be fixing too many issues
> in wc-1, given that we're switching to wc-ng in the "near" future.
As Erik said, we have to stop chasing bugs in wc-1 at some point.
There are too many of them.
It does not make sense to waste too much time on it.
If there is some hard technical problem that prevents us from contacting
the repository during the check, I'd say we should make sure the check
is updated to use information available in wc-ng as soon as possible,
and forget about fixing the problem for wc-1. Otherwise, we go for the
RA session for now, and make the check use information available in
wc-ng as soon as possible, too.
Stefan