You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2005/10/31 11:03:03 UTC
[PATCH] fix broken svn_client_info
Hi,
The following command fails with 1.3.x, even though it shouldn't:
svn info
http://tortoisesvn.tigris.org/svn/tortoisesvn/branches/release-0.20/src/TortoiseSVNSetup/TortoiseSVNSetup.vdproj@330
-r330
The reason is that same_resource_in_head() errors out if the URL doesn't
exist anymore in HEAD. In that case, it should just treat the HEAD as
unrelated instead.
Patch attached.
If I may make a request: since svn_client_info() is very important to
TSVN (we use it in different places, usually before some other
svn_client_* function to get additional information on how to proceed
further) I'd really love it if this could be backported to 1.3.x really
soon. If it's not possible to get it into 1.3.0 anymore, then I hope for
1.3.1.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by Stefan Küng <to...@gmail.com>.
Stefan Küng wrote:
> Hi,
>
> The following command fails with 1.3.x, even though it shouldn't:
>
> svn info
> http://tortoisesvn.tigris.org/svn/tortoisesvn/branches/release-0.20/src/TortoiseSVNSetup/TortoiseSVNSetup.vdproj@330
> -r330
Just noticed: this also fixes the command
svn ls
http://tortoisesvn.tigris.org/svn/tortoisesvn/branches/release-0.20/src/TortoiseSVNSetup/TortoiseSVNSetup.vdproj@332
-r332
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 31 Oct 2005, Stefan Küng wrote:
> Peter N. Lundblad wrote:
> > On Mon, 31 Oct 2005, Malcolm Rowe wrote:
> >
> > *But*, we'll have to do something on the client, since this is a server
> > bug. I'm not sure if STefan's patch or hacking something into
> > svn_client_repos_locations is the best thing to do here.
>
> I think fixing this in svn_client_repos_locations is better. New patch
> attached.
>
The problem with this is that it will filter real NOT_FOUND errors (i.e.
where the path wasn't found at the peg revision), which isn't good either.
I'll apply your original patch. We'll have to accept (and document) that
this funct9on might return NOT_FOUND if the path doesn't exist in a future
revision. We should also fix the repository part of this long-term.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 1 Nov 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> >
> > I committer your original patch (with docstring fix and test case) in
> > r17123. Nominating for 1.3.x.
>
> Your log message doesn't mention the header file that you changed.
>
Fixed. Thanks for paying attention.
> About svn_client__repos_locations(), but not directly about this change:
>
Yes, it could probably be improved.
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
>
> I committer your original patch (with docstring fix and test case) in
> r17123. Nominating for 1.3.x.
Your log message doesn't mention the header file that you changed.
About svn_client__repos_locations(), but not directly about this change:
* The doc string doesn't indicate what the (*START_REVISION) and
(*END_REVISION) return values are for - it doesn't give any reason why they
might differ from START and END respectively.
* It also doesn't say what it does if the object didn't exist at START and/or
END in the past. (If it returns an error, it should say so, though it need not
specify which one.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 31 Oct 2005, Stefan Küng wrote:
> Peter N. Lundblad wrote:
> I think fixing this in svn_client_repos_locations is better. New patch
> attached.
>
I committer your original patch (with docstring fix and test case) in
r17123. Nominating for 1.3.x.
thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by Stefan Küng <to...@gmail.com>.
Peter N. Lundblad wrote:
> On Mon, 31 Oct 2005, Malcolm Rowe wrote:
>
>> On Mon, Oct 31, 2005 at 02:33:33PM +0100, Stefan Küng wrote:
>>> The function same_resource_in_head() calls svn_client__repos_locations()
>>> which returns the error SVN_ERR_FS_NOT_FOUND. And as I see it, this is
>>> correct if the path doesn't exist. But same_resource_in_head() is the
>>> function which should not pass that error on, because it's docstring says:
>> You're not running against a 1.0.x server, are you? Because in that case,
>> we call slow_locations(), which can return either SVN_ERR_FS_NOT_FOUND
>> or APR_EGENERAL if it failed to find some of the revisions.
>>
> Oh, I really like that APR_EGENERAL:-)
>
> I've tracked it down now. It is because of svn_fs_node_history called from
> check_ancestor_of_peg_path in libsvn_repos/rev_hunt.c. That will fail,
> which I think is inconsitent, becuasee if I interpret the code correct,
> revisions that predate the creation revision for path won't cause this
> error.
>
> *But*, we'll have to do something on the client, since this is a server
> bug. I'm not sure if STefan's patch or hacking something into
> svn_client_repos_locations is the best thing to do here.
I think fixing this in svn_client_repos_locations is better. New patch
attached.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 31 Oct 2005, Malcolm Rowe wrote:
> On Mon, Oct 31, 2005 at 02:33:33PM +0100, Stefan Küng wrote:
> > The function same_resource_in_head() calls svn_client__repos_locations()
> > which returns the error SVN_ERR_FS_NOT_FOUND. And as I see it, this is
> > correct if the path doesn't exist. But same_resource_in_head() is the
> > function which should not pass that error on, because it's docstring says:
>
> You're not running against a 1.0.x server, are you? Because in that case,
> we call slow_locations(), which can return either SVN_ERR_FS_NOT_FOUND
> or APR_EGENERAL if it failed to find some of the revisions.
>
Oh, I really like that APR_EGENERAL:-)
I've tracked it down now. It is because of svn_fs_node_history called from
check_ancestor_of_peg_path in libsvn_repos/rev_hunt.c. That will fail,
which I think is inconsitent, becuasee if I interpret the code correct,
revisions that predate the creation revision for path won't cause this
error.
*But*, we'll have to do something on the client, since this is a server
bug. I'm not sure if STefan's patch or hacking something into
svn_client_repos_locations is the best thing to do here.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by Stefan Küng <to...@gmail.com>.
Malcolm Rowe wrote:
> On Mon, Oct 31, 2005 at 02:33:33PM +0100, Stefan Küng wrote:
>> The function same_resource_in_head() calls svn_client__repos_locations()
>> which returns the error SVN_ERR_FS_NOT_FOUND. And as I see it, this is
>> correct if the path doesn't exist. But same_resource_in_head() is the
>> function which should not pass that error on, because it's docstring says:
>
> You're not running against a 1.0.x server, are you? Because in that case,
> we call slow_locations(), which can return either SVN_ERR_FS_NOT_FOUND
> or APR_EGENERAL if it failed to find some of the revisions.
tigris.org is running an 1.1.x server AFAIK. Not 1.0.x.
And it's not calling slow_locations(), because svn_ra_get_locations()
doesn't return SVN_ERR_RA_NOT_IMPLEMENTED but SVN_ERR_FS_NOT_FOUND.
So the question now is:
should the check for SVN_ERR_FS_NOT_FOUND be done in
svn_client__repos_locations(), or in same_resource_in_head().
After reading the docstring of svn_client__repos_locations(), I'd say
this is the function which the check should be done.
I also don't know if this happens over file:// or svn:// connections
too, or just for http://.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Oct 31, 2005 at 02:33:33PM +0100, Stefan Küng wrote:
> The function same_resource_in_head() calls svn_client__repos_locations()
> which returns the error SVN_ERR_FS_NOT_FOUND. And as I see it, this is
> correct if the path doesn't exist. But same_resource_in_head() is the
> function which should not pass that error on, because it's docstring says:
You're not running against a 1.0.x server, are you? Because in that case,
we call slow_locations(), which can return either SVN_ERR_FS_NOT_FOUND
or APR_EGENERAL if it failed to find some of the revisions.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 31 Oct 2005, Stefan Küng wrote:
> Peter N. Lundblad wrote:
> > On Mon, 31 Oct 2005, Stefan Küng wrote:
> > Thanks for the patch. I'm not sure that this is the correct layer to fix
> > the problem. Would you care to check if this happens in the other RA
> > layers as well. This might be a bug deeper down in libsvn_ra_dav or
> > libsvn_repos.
>
> I don't think I can. I tried to find the place *where* this apr_error is
> set and couldn't even find that! I thought I could find the error define
> that way ;)
>
> Also, I think this *is* the right place to fix this bug (but of course I
> don't know the internals that well, so chances that I'm wrong are very
> high).
>
> The function same_resource_in_head() calls svn_client__repos_locations()
> which returns the error SVN_ERR_FS_NOT_FOUND. And as I see it, this is
> correct if the path doesn't exist. But same_resource_in_head() is the
> function which should not pass that error on, because it's docstring says:
>
> /* Set *SAME_P to TRUE if URL exists in the head of the repository and
> refers to the same resource as it does in REV, using POOL for
> temporary allocations. RA_SESSION is an open RA session for URL. */
>
When I wrote same_resource_in_head, I looked at the docstring of
svn_client__repos_locations, which says that if start_rev or end_rev is
greater than peg_rev, it will check that the paths are related, and if
this check fails, return SVN_ERR_CLIENT_UNRELATED_RESOURCES. I don't
believe returning FS_NOT_FOUND is correct in this case.
> So *SAME_P* must not be TRUE if the URL does not exist in HEAD. Of
> course, it isn't true without my patch either, but it throws an error
> which it shouldn't in that case. IMHO same_resource_in_head() has to
> check SVN_ERR_FS_NOT_FOUND too, because that's just one more sign that
> not a 'real' error occurred but that the resources aren't the same.
> Other errors like auth failures for example are 'real' errors for that
> function too, but not SVN_ERR_FS_NOT_FOUND.
>
Yes, it shouldn't return that error.
> About other RA layers: this function is called from all RA layers, not
> just DAV.
>
Yeah, it will be called regardless of which RA layer is in use, but it
will *call* functions specific to the RA layers. What I want to know is fi
this is specific to ra_dav.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by Stefan Küng <to...@gmail.com>.
Peter N. Lundblad wrote:
> On Mon, 31 Oct 2005, Stefan Küng wrote:
>
>> The following command fails with 1.3.x, even though it shouldn't:
>>
>> svn info
>> http://tortoisesvn.tigris.org/svn/tortoisesvn/branches/release-0.20/src/TortoiseSVNSetup/TortoiseSVNSetup.vdproj@330
>> -r330
>>
>> The reason is that same_resource_in_head() errors out if the URL doesn't
>> exist anymore in HEAD. In that case, it should just treat the HEAD as
>> unrelated instead.
>>
> Stefan,
>
> Thanks for the patch. I'm not sure that this is the correct layer to fix
> the problem. Would you care to check if this happens in the other RA
> layers as well. This might be a bug deeper down in libsvn_ra_dav or
> libsvn_repos.
I don't think I can. I tried to find the place *where* this apr_error is
set and couldn't even find that! I thought I could find the error define
that way ;)
Also, I think this *is* the right place to fix this bug (but of course I
don't know the internals that well, so chances that I'm wrong are very
high).
The function same_resource_in_head() calls svn_client__repos_locations()
which returns the error SVN_ERR_FS_NOT_FOUND. And as I see it, this is
correct if the path doesn't exist. But same_resource_in_head() is the
function which should not pass that error on, because it's docstring says:
/* Set *SAME_P to TRUE if URL exists in the head of the repository and
refers to the same resource as it does in REV, using POOL for
temporary allocations. RA_SESSION is an open RA session for URL. */
So *SAME_P* must not be TRUE if the URL does not exist in HEAD. Of
course, it isn't true without my patch either, but it throws an error
which it shouldn't in that case. IMHO same_resource_in_head() has to
check SVN_ERR_FS_NOT_FOUND too, because that's just one more sign that
not a 'real' error occurred but that the resources aren't the same.
Other errors like auth failures for example are 'real' errors for that
function too, but not SVN_ERR_FS_NOT_FOUND.
About other RA layers: this function is called from all RA layers, not
just DAV.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix broken svn_client_info
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 31 Oct 2005, Stefan Küng wrote:
> The following command fails with 1.3.x, even though it shouldn't:
>
> svn info
> http://tortoisesvn.tigris.org/svn/tortoisesvn/branches/release-0.20/src/TortoiseSVNSetup/TortoiseSVNSetup.vdproj@330
> -r330
>
> The reason is that same_resource_in_head() errors out if the URL doesn't
> exist anymore in HEAD. In that case, it should just treat the HEAD as
> unrelated instead.
>
Stefan,
Thanks for the patch. I'm not sure that this is the correct layer to fix
the problem. Would you care to check if this happens in the other RA
layers as well. This might be a bug deeper down in libsvn_ra_dav or
libsvn_repos.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org