You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Mark Phippard <ma...@gmail.com> on 2007/04/11 19:06:48 UTC
[PATCH] Fix JavaHL testBasicProperties on Windows
The JavaHL properties functions were rewritten to use a new callback
API. When that API is called it passes in a path in a format like
C:/folder1/folder2/file.ext. This value is stored as a key in a Map.
When the getProperties method is later called it is passed a String in
the format C:\folder1\folder2\file.ext so it fails to find a value in
the Map.
The attached patch fixes the problem by normalizing the path in the
get and set methods. I am not convinced this is the best solution and
I do not like that we are fixing it in our Impl method as it means
other people that implement the interface have to deal with the same
problem.
I also fixed a potential NullPointerError that this test failure revealed.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: [PATCH] Fix JavaHL testBasicProperties on Windows
Posted by Mark Phippard <ma...@gmail.com>.
On 4/11/07, Nicolás Lichtmaier <ni...@reloco.com.ar> wrote:
>
> > The JavaHL properties functions were rewritten to use a new callback
> > API. When that API is called it passes in a path in a format like
> > C:/folder1/folder2/file.ext. This value is stored as a key in a Map.
> > When the getProperties method is later called it is passed a String in
> > the format C:\folder1\folder2\file.ext so it fails to find a value in
> > the Map.
>
> Couldn't you just use File (if these are local files) or java.net.URI
> (if these may be URLs)?
I think all we know for sure is that we have a String. BTW, the
normalizePath method should have a null check added to it.
I kind of think this should just be handled differently. I did not
change the code to use this technique in the first place, so I am not
sure what all the options are the might be available. For example,
maybe when the callback method is fired, the path it receives will not
have been converted in any way.
The changes do seem like they have made this more fragile with lots of
potential for corner cases. For example C:/ and c:/ would not match
each other either.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix JavaHL testBasicProperties on Windows
Posted by Nicolás Lichtmaier <ni...@reloco.com.ar>.
> The JavaHL properties functions were rewritten to use a new callback
> API. When that API is called it passes in a path in a format like
> C:/folder1/folder2/file.ext. This value is stored as a key in a Map.
> When the getProperties method is later called it is passed a String in
> the format C:\folder1\folder2\file.ext so it fails to find a value in
> the Map.
Couldn't you just use File (if these are local files) or java.net.URI
(if these may be URLs)?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Documenting use of canonical paths in external APIs
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 11 Apr 2007, Hyrum K. Wright wrote:
> Daniel Rall wrote:
> > On Wed, 11 Apr 2007, Hyrum K. Wright wrote:
> >
> >> Daniel Rall wrote:
> > ...
> >>> I asked Erik where we document this important API usage fact, but he
> >>> wasn't sure. Do we document this anywhere? If not, I suggest
> >>> documenting it in hacking.html.
> >> Would documenting it in the header files also be useful? After all, it
> >> is something which the API consumers should be aware of. The header
> >> files are what outlines our API specification, and this seems part of
> >> that spec.
> >
> > I first thought of the header files (I'd personally like to see it
> > documented there). I suppose a sweep through svn_client.h would be a
> > sufficient, combined with a note in hacking.html?
>
> Or even just a note at the top of svn_client.h. Something similar to
> "you must call apr_initialize() before using any of this stuff." Come
> to think of it, that's not documented in svn_client.h either.
>
> I agree that it should also be mentioned in hacking.html.
I took care of hacking.html in r24575. Can people take a look?
If it looks okay, I'll try something similar in svn_client.h.
> -Hyrum (who has often thought it might be nice to have a separate
> "hacking with Subversion" document, in addition to the existing "hacking
> on Subversion" document)
+1
Re: Documenting use of canonical paths in external APIs
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Daniel Rall wrote:
> On Wed, 11 Apr 2007, Hyrum K. Wright wrote:
>
>> Daniel Rall wrote:
> ...
>>> I asked Erik where we document this important API usage fact, but he
>>> wasn't sure. Do we document this anywhere? If not, I suggest
>>> documenting it in hacking.html.
>> Would documenting it in the header files also be useful? After all, it
>> is something which the API consumers should be aware of. The header
>> files are what outlines our API specification, and this seems part of
>> that spec.
>
> I first thought of the header files (I'd personally like to see it
> documented there). I suppose a sweep through svn_client.h would be a
> sufficient, combined with a note in hacking.html?
Or even just a note at the top of svn_client.h. Something similar to
"you must call apr_initialize() before using any of this stuff." Come
to think of it, that's not documented in svn_client.h either.
I agree that it should also be mentioned in hacking.html.
-Hyrum (who has often thought it might be nice to have a separate
"hacking with Subversion" document, in addition to the existing "hacking
on Subversion" document)
Documenting use of canonical paths in external APIs
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 11 Apr 2007, Hyrum K. Wright wrote:
> Daniel Rall wrote:
...
> > I asked Erik where we document this important API usage fact, but he
> > wasn't sure. Do we document this anywhere? If not, I suggest
> > documenting it in hacking.html.
>
> Would documenting it in the header files also be useful? After all, it
> is something which the API consumers should be aware of. The header
> files are what outlines our API specification, and this seems part of
> that spec.
I first thought of the header files (I'd personally like to see it
documented there). I suppose a sweep through svn_client.h would be a
sufficient, combined with a note in hacking.html?
Re: [PATCH] Fix JavaHL testBasicProperties on Windows
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Daniel Rall wrote:
> On Wed, 11 Apr 2007, Mark Phippard wrote:
...
>> Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/ProplistCallbackImpl.java^M
>> ===================================================================^M
>> --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/ProplistCallbackImpl.java (revision 24532)^M
>> +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/ProplistCallbackImpl.java (working copy)^M
>> @@ -31,11 +31,16 @@^M
>>
>> public void singlePath(String path, Map props)
>> {
>> - propMap.put(path, props);
>> + propMap.put(normalizePath(path), props);
>> }
>>
>> public Map getProperties(String path)
>> {
>> - return (Map) propMap.get(path);
>> + return (Map) propMap.get(normalizePath(path));
>> }
>> +
>> + private String normalizePath(String path)
>> + {
>> + return path.replace('\\', '/');
>> + }
>> }
>
> We talked this over on IRC. The problem here boils down to the fact
> that svn_proplist_receiver_t is passed a canonical path, rather than a
> path in local style. Erik indicates that this is expected behavior
> for our APIs. Hyrum will be adding a call to svn_path_local_style()
> to JavaHL's C++ code to handle the conversion.
Done in r24536.
> I asked Erik where we document this important API usage fact, but he
> wasn't sure. Do we document this anywhere? If not, I suggest
> documenting it in hacking.html.
Would documenting it in the header files also be useful? After all, it
is something which the API consumers should be aware of. The header
files are what outlines our API specification, and this seems part of
that spec.
-Hyrum
Re: [PATCH] Fix JavaHL testBasicProperties on Windows
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Daniel Rall wrote:
> On Wed, 11 Apr 2007, Mark Phippard wrote:
>
>> The JavaHL properties functions were rewritten to use a new callback
>> API. When that API is called it passes in a path in a format like
>> C:/folder1/folder2/file.ext. This value is stored as a key in a Map.
>> When the getProperties method is later called it is passed a String in
>> the format C:\folder1\folder2\file.ext so it fails to find a value in
>> the Map.
>>
>> The attached patch fixes the problem by normalizing the path in the
>> get and set methods. I am not convinced this is the best solution and
>> I do not like that we are fixing it in our Impl method as it means
>> other people that implement the interface have to deal with the same
>> problem.
>>
>> I also fixed a potential NullPointerError that this test failure revealed.
>>
>> Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java^M
>> ===================================================================^M
>> --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (revision 24532)^M
>> +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (working copy)^M
>> @@ -1253,6 +1253,8 @@^M
>> properties(path, revision, pegRevision, false, callback);
>>
>> Map propMap = callback.getProperties(path);
>> + if (propMap == null)
>> + return new PropertyData[0];
>> PropertyData[] props = new PropertyData[propMap.size()];
>>
>> Iterator it = propMap.keySet().iterator();
>
> Please commit this portion of the patch.
Done, at Mark's request, in r24537.
-Hyrum
Re: [PATCH] Fix JavaHL testBasicProperties on Windows
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 11 Apr 2007, Mark Phippard wrote:
> The JavaHL properties functions were rewritten to use a new callback
> API. When that API is called it passes in a path in a format like
> C:/folder1/folder2/file.ext. This value is stored as a key in a Map.
> When the getProperties method is later called it is passed a String in
> the format C:\folder1\folder2\file.ext so it fails to find a value in
> the Map.
>
> The attached patch fixes the problem by normalizing the path in the
> get and set methods. I am not convinced this is the best solution and
> I do not like that we are fixing it in our Impl method as it means
> other people that implement the interface have to deal with the same
> problem.
>
> I also fixed a potential NullPointerError that this test failure revealed.
>
> Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java^M
> ===================================================================^M
> --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (revision 24532)^M
> +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/SVNClient.java (working copy)^M
> @@ -1253,6 +1253,8 @@^M
> properties(path, revision, pegRevision, false, callback);
>
> Map propMap = callback.getProperties(path);
> + if (propMap == null)
> + return new PropertyData[0];
> PropertyData[] props = new PropertyData[propMap.size()];
>
> Iterator it = propMap.keySet().iterator();
Please commit this portion of the patch.
> Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/ProplistCallbackImpl.java^M
> ===================================================================^M
> --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/ProplistCallbackImpl.java (revision 24532)^M
> +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/ProplistCallbackImpl.java (working copy)^M
> @@ -31,11 +31,16 @@^M
>
> public void singlePath(String path, Map props)
> {
> - propMap.put(path, props);
> + propMap.put(normalizePath(path), props);
> }
>
> public Map getProperties(String path)
> {
> - return (Map) propMap.get(path);
> + return (Map) propMap.get(normalizePath(path));
> }
> +
> + private String normalizePath(String path)
> + {
> + return path.replace('\\', '/');
> + }
> }
We talked this over on IRC. The problem here boils down to the fact
that svn_proplist_receiver_t is passed a canonical path, rather than a
path in local style. Erik indicates that this is expected behavior
for our APIs. Hyrum will be adding a call to svn_path_local_style()
to JavaHL's C++ code to handle the conversion.
I asked Erik where we document this important API usage fact, but he
wasn't sure. Do we document this anywhere? If not, I suggest
documenting it in hacking.html.
- Dan