RFR: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Daniel Fuchs
Hi,

Please find below a patch for:
8189953: FileHandler constructor throws NoSuchFileException
          with absolute path
https://bugs.openjdk.java.net/browse/JDK-8189953

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.00/

This is a windows only bug.

The issue is caused by a change that happen somewhen between
8 and 9, where new File(new File("C:"),"Workspace") was fixed
to return "C:Workspace" and not "C:\\Workspace".
This uncovered a bug in the FileHandler::generate algorithm.

The FileHandler::generate algorithm could arguably be improved
by being entirely rewritten but I choose to keep the changes
as minimal as I could.

best regards,

-- daniel
Reply | Threaded
Open this post in threaded view
|

Re: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Jason Mehrens
Hi Daniel,

For more background on this issue I think you should link to this issue to the following:

https://bugs.openjdk.java.net/browse/JDK-8153250
https://bugs.openjdk.java.net/browse/JDK-8130462
https://bugs.openjdk.java.net/browse/JDK-8189862

Thanks,

Jason
________________________________________
From: core-libs-dev <[hidden email]> on behalf of Daniel Fuchs <[hidden email]>
Sent: Thursday, November 2, 2017 11:47 AM
To: core-libs-dev
Subject: RFR: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Hi,

Please find below a patch for:
8189953: FileHandler constructor throws NoSuchFileException
          with absolute path
https://bugs.openjdk.java.net/browse/JDK-8189953

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.00/

This is a windows only bug.

The issue is caused by a change that happen somewhen between
8 and 9, where new File(new File("C:"),"Workspace") was fixed
to return "C:Workspace" and not "C:\\Workspace".
This uncovered a bug in the FileHandler::generate algorithm.

The FileHandler::generate algorithm could arguably be improved
by being entirely rewritten but I choose to keep the changes
as minimal as I could.

best regards,

-- daniel
Reply | Threaded
Open this post in threaded view
|

Re: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Daniel Fuchs
On 02/11/2017 17:55, Jason Mehrens wrote:
> Hi Daniel,
>
> For more background on this issue I think you should link to this issue to the following:
>
> https://bugs.openjdk.java.net/browse/JDK-8153250
> https://bugs.openjdk.java.net/browse/JDK-8130462
> https://bugs.openjdk.java.net/browse/JDK-8189862

Oh, thanks Jason!
That is great :-)

I have linked the issues.

best regards,

-- daniel

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Mandy Chung
In reply to this post by Daniel Fuchs
Hi Daniel,

On 11/2/17 9:47 AM, Daniel Fuchs wrote:

> Hi,
>
> Please find below a patch for:
> 8189953: FileHandler constructor throws NoSuchFileException
>          with absolute path
> https://bugs.openjdk.java.net/browse/JDK-8189953
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.00/
>
This looks okay.

A minor comment: I think line 695-709 can be made cleaner and easier to
read and

e.g.

if (word.length() >0) {
     String n = word.toString();
     if (result ==null) {
         result = prev !=null ? prev.resolveSibling(n) : Paths.get(n);
     }else {
         Path p = prev !=null ? prev.resolveSibling(n) : Paths.get(n);
         result = result.resolve(p);
     }
}else if (result ==null) {
     result = Paths.get("");
}
if (path.getRoot() ==null)
     return result.toFile();
else return path.getRoot().resolve(result).toFile();


Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Daniel Fuchs
Hi Mandy,

Thanks for the review - and thanks for reminding me about
@modules java.logging/java.util.logging:open

I have a new webrev that incorporates your suggestions.
I have also tried to reduce the long lines (I recently
switched to a new IDE whose default config make them more
difficult to spot).

http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.01

best regards,

-- daniel

On 09/11/2017 17:41, mandy chung wrote:

> Hi Daniel,
>
> On 11/2/17 9:47 AM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a patch for:
>> 8189953: FileHandler constructor throws NoSuchFileException
>>          with absolute path
>> https://bugs.openjdk.java.net/browse/JDK-8189953
>>
>> webrev:
>> http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.00/
>>
> This looks okay.
>
> A minor comment: I think line 695-709 can be made cleaner and easier to
> read and
>
> e.g.
>
> if (word.length() >0) {
>      String n = word.toString();
>      if (result ==null) {
>          result = prev !=null ? prev.resolveSibling(n) : Paths.get(n);
>      }else {
>          Path p = prev !=null ? prev.resolveSibling(n) : Paths.get(n);
>          result = result.resolve(p);
>      }
> }else if (result ==null) {
>      result = Paths.get("");
> }
> if (path.getRoot() ==null)
>      return result.toFile();
> else return path.getRoot().resolve(result).toFile();
>
>
> Mandy

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

Mandy Chung


On 11/9/17 11:20 AM, Daniel Fuchs wrote:

> Hi Mandy,
>
> Thanks for the review - and thanks for reminding me about
> @modules java.logging/java.util.logging:open
>
> I have a new webrev that incorporates your suggestions.
> I have also tried to reduce the long lines (I recently
> switched to a new IDE whose default config make them more
> difficult to spot).
>
> http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.01

Looks good.

thanks
Mandy