You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2008/01/30 20:41:13 UTC
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Of interest (to me, at least) is how we managed to get ahold of the
non-canonicalized path that we currently write to the entries file. I mean,
the first thing our command-line client does with the paths provided to
subcommands is canonicalize them, I thought. What gives?
Augie Fackler wrote:
> I have a simple one-line patch that fixes the issue for me. It appears
> the problem occurred when the repo root was also the server root the
> repo root was declared in entries to be "http://svn.example.com/" but
> should have been "http://svn.example.com" according to
> svn_path_canonicalize(). I just tested this against svn.perian.org and
> the checkout worked as expected from there.
> Peace,
> Augie
>
> [[[
> Fix a regression from 1.4 where corrupt WC entries would be generated if
> the
> server root and the repository root were the same.
>
> * subversion/libsvn_wc/entries.c
> (write_entry): Call svn_path_canonicalize() on the repository root
> path before
> writing to entries.
>
> Patch by: Augie Fackler <du...@gmail.com>
> ]]]
>
> Index: subversion/libsvn_wc/entries.c
> ===================================================================
> --- subversion/libsvn_wc/entries.c (revision 29086)
> +++ subversion/libsvn_wc/entries.c (working copy)
> @@ -1538,9 +1539,9 @@
> || (this_dir->repos == NULL
> || (entry->repos
> && strcmp(this_dir->repos, entry->repos) != 0))))
> - valuestr = entry->repos;
> + valuestr = svn_path_canonicalize(entry->repos, pool);
> else
> valuestr = NULL;
> write_str(buf, valuestr, pool);
>
> /* Schedule. */
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by Augie Fackler <du...@gmail.com>.
On Jan 30, 2008, at 3:01 PM, David Glasser wrote:
> On Jan 30, 2008 12:57 PM, C. Michael Pilato <cm...@collab.net>
> wrote:
>> Augie Fackler wrote:
>>>
>>> On Jan 30, 2008, at 2:41 PM, C. Michael Pilato wrote:
>>>
>>>> Of interest (to me, at least) is how we managed to get ahold of the
>>>> non-canonicalized path that we currently write to the entries
>>>> file. I
>>>> mean, the first thing our command-line client does with the paths
>>>> provided to subcommands is canonicalize them, I thought. What
>>>> gives?
>>>
>>> The path is the computed repository root - I haven't figured out
>>> in the
>>> code where that happens yet, but it looks to my eye like the
>>> repository
>>> root is being computed with the hostname and the path separate, so
>>> that
>>> when the path is just / it isn't stored in a canonical form, but
>>> if it
>>> was at /foo/ it would get stored as /foo
>>>
>>> I was seeing the bug when I was checking out
>>> http://svn.perian.org/trunk, which isn't the repo root, so svn
>>> must be
>>> computing the repo root of http://svn.perian.org/ on its own
>>> somewhere...
>>
>> Oh, yes, right -- forgot that you were probably checking out
>> something
>> deeper than the root. Yes, each RA implementation does this sort
>> of root
>> calculation. Your solution will work, but still leaves us with
>> non-canonicalized paths being passed around the libraries in memory
>> during
>> the checkout. I would suggest making the four RA layers do this
>> canonicalization at the moment they discern the repository root.
>
> +1
>
> Augie, do you think you'll get a chance to do this?
Working on it now. Should be done fairly soon (unless my apartment
details start moving again, in which case it might get delayed until
this evening).
Augie
> It's a shame that our test infrastructure doesn't provide an easy way
> to write a test for this condition.
>
> --dave
>
> --
> David Glasser | glasser@davidglasser.net | http://
> www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by David Glasser <gl...@davidglasser.net>.
On Jan 30, 2008 12:57 PM, C. Michael Pilato <cm...@collab.net> wrote:
> Augie Fackler wrote:
> >
> > On Jan 30, 2008, at 2:41 PM, C. Michael Pilato wrote:
> >
> >> Of interest (to me, at least) is how we managed to get ahold of the
> >> non-canonicalized path that we currently write to the entries file. I
> >> mean, the first thing our command-line client does with the paths
> >> provided to subcommands is canonicalize them, I thought. What gives?
> >
> > The path is the computed repository root - I haven't figured out in the
> > code where that happens yet, but it looks to my eye like the repository
> > root is being computed with the hostname and the path separate, so that
> > when the path is just / it isn't stored in a canonical form, but if it
> > was at /foo/ it would get stored as /foo
> >
> > I was seeing the bug when I was checking out
> > http://svn.perian.org/trunk, which isn't the repo root, so svn must be
> > computing the repo root of http://svn.perian.org/ on its own somewhere...
>
> Oh, yes, right -- forgot that you were probably checking out something
> deeper than the root. Yes, each RA implementation does this sort of root
> calculation. Your solution will work, but still leaves us with
> non-canonicalized paths being passed around the libraries in memory during
> the checkout. I would suggest making the four RA layers do this
> canonicalization at the moment they discern the repository root.
+1
Augie, do you think you'll get a chance to do this?
It's a shame that our test infrastructure doesn't provide an easy way
to write a test for this condition.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by David Glasser <gl...@davidglasser.net>.
On Jan 30, 2008 2:03 PM, Augie Fackler <du...@gmail.com> wrote:
>
> On Jan 30, 2008, at 3:17 PM, Augie Fackler wrote:
>
> >
> > On Jan 30, 2008, at 3:12 PM, C. Michael Pilato wrote:
> >
> >> Augie Fackler wrote:
> >>> Ah, that information comes from the RA layer (I was just trying to
> >>> figure this out) - I'll go inspect them all and make sure they
> >>> DTRT for this case. Should I keep the original part of the patch
> >>> and include it with the work on the RA layer?
> >>
> >> We shouldn't need the original patch, but I won't complain if we
> >> include that code. Should make us more resilient in the case of
> >> some other unseen programmer error.
> >
> > Acutally, in looking more - it's a bug in libsvn_ra_serf anyway. If
> > I switch my RA layer to neon and re-run the checkout without my
> > patch, it works. I'm working on a fix for serf now.
>
> This is the simplest fix I can come up with for serf:
> Index: subversion/libsvn_ra_serf/serf.c
> ===================================================================
> --- subversion/libsvn_ra_serf/serf.c (revision 29079)
> +++ subversion/libsvn_ra_serf/serf.c (working copy)
> @@ -992,6 +992,7 @@
> }
>
> *url = session->repos_root_str;
> + *url = svn_path_canonicalize(*url, pool);
> return SVN_NO_ERROR;
> }
>
> Anything else will require a little fiddling around inside serf to
> make some of the code in update.c work correctly.
So I guess the alternative here is to make session->repos_root_str be
set correctly in svn_ra_serf__discover_root, but this might cause
weirdness in the other code that looks at that field in update.c?
Lieven or Justin, any opinions?
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a bug in ra_serf
Posted by "C. Michael Pilato" <cm...@collab.net>.
C. Michael Pilato wrote:
> I'll field this.
>
>
> Augie Fackler wrote:
>> Here's a lower-level patch. My adhoc testing against svn's repo and
>> perian's indicates this works, but I'd appreciate it if someone could
>> run it through automated tests (which I don't have set up for dav
>> tests) before accepting the patch.
>>
>> Peace,
>> Augie
>>
>> [[[
>> Fix a bug in ra_serf where the computed repository root was not
>> canonical,
>> which caused working copy corruption.
>>
>> * subversion/libsvn_ra_serf/util.c
>> (svn_ra_serf__discover_root): Canonicalize repos_root_str when
>> discovering the
>> repository root.
>>
>> Patch by: Augie Fackler: <du...@gmail.com>
>> ]]]
Committed (tweaked) in r29148.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Fix a bug in ra_serf
Posted by "C. Michael Pilato" <cm...@collab.net>.
I'll field this.
Augie Fackler wrote:
> Here's a lower-level patch. My adhoc testing against svn's repo and
> perian's indicates this works, but I'd appreciate it if someone could
> run it through automated tests (which I don't have set up for dav tests)
> before accepting the patch.
>
> Peace,
> Augie
>
> [[[
> Fix a bug in ra_serf where the computed repository root was not canonical,
> which caused working copy corruption.
>
> * subversion/libsvn_ra_serf/util.c
> (svn_ra_serf__discover_root): Canonicalize repos_root_str when
> discovering the
> repository root.
>
> Patch by: Augie Fackler: <du...@gmail.com>
> ]]]
>
> Index: subversion/libsvn_ra_serf/util.c
> ===================================================================
> --- subversion/libsvn_ra_serf/util.c (revision 29127)
> +++ subversion/libsvn_ra_serf/util.c (working copy)
> @@ -1218,6 +1218,9 @@
> session->repos_root.path = apr_pstrdup(session->pool,
> url_buf->data);
> session->repos_root_str = apr_uri_unparse(session->pool,
> &session->repos_root, 0);
> + /* Canonicalize the string, otherwise we create corrupt WC
> entries */
> + session->repos_root_str =
> svn_path_canonicalize(session->repos_root_str,
> + pool);
> }
>
> if (rel_path)
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Fix a bug in ra_serf (was: Fix a regression from 1.4 in trunk and branch)
Posted by Augie Fackler <du...@gmail.com>.
Here's a lower-level patch. My adhoc testing against svn's repo and
perian's indicates this works, but I'd appreciate it if someone could
run it through automated tests (which I don't have set up for dav
tests) before accepting the patch.
Peace,
Augie
[[[
Fix a bug in ra_serf where the computed repository root was not
canonical,
which caused working copy corruption.
* subversion/libsvn_ra_serf/util.c
(svn_ra_serf__discover_root): Canonicalize repos_root_str when
discovering the
repository root.
Patch by: Augie Fackler: <du...@gmail.com>
]]]
Index: subversion/libsvn_ra_serf/util.c
===================================================================
--- subversion/libsvn_ra_serf/util.c (revision 29127)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -1218,6 +1218,9 @@
session->repos_root.path = apr_pstrdup(session->pool, url_buf-
>data);
session->repos_root_str = apr_uri_unparse(session->pool,
&session-
>repos_root, 0);
+ /* Canonicalize the string, otherwise we create corrupt WC
entries */
+ session->repos_root_str = svn_path_canonicalize(session-
>repos_root_str,
+ pool);
}
if (rel_path)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Fix a bug in ra_serf (was: Fix a regression from 1.4 in trunk and branch)
Posted by Augie Fackler <du...@gmail.com>.
Per IRC, sending this patch with a log message and so forth. Patch and
log message after the conversation.
On Jan 30, 2008, at 4:03 PM, Augie Fackler wrote:
>
> On Jan 30, 2008, at 3:17 PM, Augie Fackler wrote:
>
>>
>> On Jan 30, 2008, at 3:12 PM, C. Michael Pilato wrote:
>>
>>> Augie Fackler wrote:
>>>> Ah, that information comes from the RA layer (I was just trying
>>>> to figure this out) - I'll go inspect them all and make sure they
>>>> DTRT for this case. Should I keep the original part of the patch
>>>> and include it with the work on the RA layer?
>>>
>>> We shouldn't need the original patch, but I won't complain if we
>>> include that code. Should make us more resilient in the case of
>>> some other unseen programmer error.
>>
>> Acutally, in looking more - it's a bug in libsvn_ra_serf anyway. If
>> I switch my RA layer to neon and re-run the checkout without my
>> patch, it works. I'm working on a fix for serf now.
>
> This is the simplest fix I can come up with for serf:
> Index: subversion/libsvn_ra_serf/serf.c
> ===================================================================
> --- subversion/libsvn_ra_serf/serf.c (revision 29079)
> +++ subversion/libsvn_ra_serf/serf.c (working copy)
> @@ -992,6 +992,7 @@
> }
>
> *url = session->repos_root_str;
> + *url = svn_path_canonicalize(*url, pool);
> return SVN_NO_ERROR;
> }
>
> Anything else will require a little fiddling around inside serf to
> make some of the code in update.c work correctly.
>
> Augie
[[[
Fix a bug in ra_serf where the computed repo root was not canonical.
* subversion/libsvn_ra_serf/serf.c
(svn_ra_serf__get_repos_root): Canonicalize the repo root URL before
returning.
]]]
Index: subversion/libsvn_ra_serf/serf.c
===================================================================
--- subversion/libsvn_ra_serf/serf.c (revision 29079)
+++ subversion/libsvn_ra_serf/serf.c (working copy)
@@ -992,6 +992,7 @@
}
*url = session->repos_root_str;
+ *url = svn_path_canonicalize(*url, pool);
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by Augie Fackler <du...@gmail.com>.
On Jan 30, 2008, at 3:17 PM, Augie Fackler wrote:
>
> On Jan 30, 2008, at 3:12 PM, C. Michael Pilato wrote:
>
>> Augie Fackler wrote:
>>> Ah, that information comes from the RA layer (I was just trying to
>>> figure this out) - I'll go inspect them all and make sure they
>>> DTRT for this case. Should I keep the original part of the patch
>>> and include it with the work on the RA layer?
>>
>> We shouldn't need the original patch, but I won't complain if we
>> include that code. Should make us more resilient in the case of
>> some other unseen programmer error.
>
> Acutally, in looking more - it's a bug in libsvn_ra_serf anyway. If
> I switch my RA layer to neon and re-run the checkout without my
> patch, it works. I'm working on a fix for serf now.
This is the simplest fix I can come up with for serf:
Index: subversion/libsvn_ra_serf/serf.c
===================================================================
--- subversion/libsvn_ra_serf/serf.c (revision 29079)
+++ subversion/libsvn_ra_serf/serf.c (working copy)
@@ -992,6 +992,7 @@
}
*url = session->repos_root_str;
+ *url = svn_path_canonicalize(*url, pool);
return SVN_NO_ERROR;
}
Anything else will require a little fiddling around inside serf to
make some of the code in update.c work correctly.
Augie
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by Augie Fackler <du...@gmail.com>.
On Jan 30, 2008, at 3:12 PM, C. Michael Pilato wrote:
> Augie Fackler wrote:
>> Ah, that information comes from the RA layer (I was just trying to
>> figure this out) - I'll go inspect them all and make sure they DTRT
>> for this case. Should I keep the original part of the patch and
>> include it with the work on the RA layer?
>
> We shouldn't need the original patch, but I won't complain if we
> include that code. Should make us more resilient in the case of
> some other unseen programmer error.
Acutally, in looking more - it's a bug in libsvn_ra_serf anyway. If I
switch my RA layer to neon and re-run the checkout without my patch,
it works. I'm working on a fix for serf now.
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On
> Demand
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by "C. Michael Pilato" <cm...@collab.net>.
Augie Fackler wrote:
> Ah, that information comes from the RA layer (I was just trying to
> figure this out) - I'll go inspect them all and make sure they DTRT for
> this case. Should I keep the original part of the patch and include it
> with the work on the RA layer?
We shouldn't need the original patch, but I won't complain if we include
that code. Should make us more resilient in the case of some other unseen
programmer error.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by Augie Fackler <du...@gmail.com>.
On Jan 30, 2008, at 2:57 PM, C. Michael Pilato wrote:
> Augie Fackler wrote:
>> On Jan 30, 2008, at 2:41 PM, C. Michael Pilato wrote:
>>> Of interest (to me, at least) is how we managed to get ahold of
>>> the non-canonicalized path that we currently write to the entries
>>> file. I mean, the first thing our command-line client does with
>>> the paths provided to subcommands is canonicalize them, I
>>> thought. What gives?
>> The path is the computed repository root - I haven't figured out in
>> the code where that happens yet, but it looks to my eye like the
>> repository root is being computed with the hostname and the path
>> separate, so that when the path is just / it isn't stored in a
>> canonical form, but if it was at /foo/ it would get stored as /foo
>> I was seeing the bug when I was checking out http://svn.perian.org/trunk
>> , which isn't the repo root, so svn must be computing the repo root
>> of http://svn.perian.org/ on its own somewhere...
>
> Oh, yes, right -- forgot that you were probably checking out
> something deeper than the root. Yes, each RA implementation does
> this sort of root calculation. Your solution will work, but still
> leaves us with non-canonicalized paths being passed around the
> libraries in memory during the checkout. I would suggest making the
> four RA layers do this canonicalization at the moment they discern
> the repository root.
Ah, that information comes from the RA layer (I was just trying to
figure this out) - I'll go inspect them all and make sure they DTRT
for this case. Should I keep the original part of the patch and
include it with the work on the RA layer?
>
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On
> Demand
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by "C. Michael Pilato" <cm...@collab.net>.
Augie Fackler wrote:
>
> On Jan 30, 2008, at 2:41 PM, C. Michael Pilato wrote:
>
>> Of interest (to me, at least) is how we managed to get ahold of the
>> non-canonicalized path that we currently write to the entries file. I
>> mean, the first thing our command-line client does with the paths
>> provided to subcommands is canonicalize them, I thought. What gives?
>
> The path is the computed repository root - I haven't figured out in the
> code where that happens yet, but it looks to my eye like the repository
> root is being computed with the hostname and the path separate, so that
> when the path is just / it isn't stored in a canonical form, but if it
> was at /foo/ it would get stored as /foo
>
> I was seeing the bug when I was checking out
> http://svn.perian.org/trunk, which isn't the repo root, so svn must be
> computing the repo root of http://svn.perian.org/ on its own somewhere...
Oh, yes, right -- forgot that you were probably checking out something
deeper than the root. Yes, each RA implementation does this sort of root
calculation. Your solution will work, but still leaves us with
non-canonicalized paths being passed around the libraries in memory during
the checkout. I would suggest making the four RA layers do this
canonicalization at the moment they discern the repository root.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Fix a regression from 1.4 in trunk and branch
Posted by Augie Fackler <du...@gmail.com>.
On Jan 30, 2008, at 2:41 PM, C. Michael Pilato wrote:
> Of interest (to me, at least) is how we managed to get ahold of the
> non-canonicalized path that we currently write to the entries file.
> I mean, the first thing our command-line client does with the paths
> provided to subcommands is canonicalize them, I thought. What gives?
The path is the computed repository root - I haven't figured out in
the code where that happens yet, but it looks to my eye like the
repository root is being computed with the hostname and the path
separate, so that when the path is just / it isn't stored in a
canonical form, but if it was at /foo/ it would get stored as /foo
I was seeing the bug when I was checking out http://svn.perian.org/
trunk, which isn't the repo root, so svn must be computing the repo
root of http://svn.perian.org/ on its own somewhere...
>
>
>
> Augie Fackler wrote:
>> I have a simple one-line patch that fixes the issue for me. It
>> appears the problem occurred when the repo root was also the server
>> root the repo root was declared in entries to be "http://svn.example.com/
>> " but should have been "http://svn.example.com" according to
>> svn_path_canonicalize(). I just tested this against svn.perian.org
>> and the checkout worked as expected from there.
>> Peace,
>> Augie
>> [[[
>> Fix a regression from 1.4 where corrupt WC entries would be
>> generated if the
>> server root and the repository root were the same.
>> * subversion/libsvn_wc/entries.c
>> (write_entry): Call svn_path_canonicalize() on the repository root
>> path before
>> writing to entries.
>> Patch by: Augie Fackler <du...@gmail.com>
>> ]]]
>> Index: subversion/libsvn_wc/entries.c
>> ===================================================================
>> --- subversion/libsvn_wc/entries.c (revision 29086)
>> +++ subversion/libsvn_wc/entries.c (working copy)
>> @@ -1538,9 +1539,9 @@
>> || (this_dir->repos == NULL
>> || (entry->repos
>> && strcmp(this_dir->repos, entry->repos) != 0))))
>> - valuestr = entry->repos;
>> + valuestr = svn_path_canonicalize(entry->repos, pool);
>> else
>> valuestr = NULL;
>> write_str(buf, valuestr, pool);
>> /* Schedule. */
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On
> Demand
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org