Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #28528 #31630

Merged
merged 1 commit into from
Feb 14, 2016
Merged

Fixes #28528 #31630

merged 1 commit into from
Feb 14, 2016

Conversation

pitdicker
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@pitdicker
Copy link
Contributor Author

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Feb 13, 2016
alexcrichton
alexcrichton reviewed Feb 13, 2016
View reviewed changes
src/libstd/sys/windows/fs.rs
(*info).SubstituteNameOffset / 2,
(*info).SubstituteNameLength / 2,
(*info).Flags & c::SYMLINK_FLAG_RELATIVE != 0)
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically we don't indent over the match body an extra indent

@alexcrichton
Copy link
Member

Thanks @pitdicker! Just a few stylistic nits, but otherwise looks good to me.

Does all metadata on Windows go through File::metadata now? I think that wasn't true at some point but it now looks to be the case? We could change the layout of fs::Metadata if that's the case to just be exactly that return value.

@alexcrichton
Copy link
Member

Ah and also can you expand the commit message to explain what's going on as well?

@pitdicker
Copy link
Contributor Author

I am not completely sure what you mean about metadata. There are now only two points that call into WinAPI for metadata: File::file_attr and in DirEntry. But what could be changed to be some return variable?

@pitdicker
Fixes #28528
6403f91 
Fix `read_link` to also be able to read the target of junctions on Windows.
Also the path returned should not include a NT namespace, and there were
some problems with permissions.
@alexcrichton
Copy link
Member

Ah yeah I guess we still have two entry points, perhaps we can refactor later!

@bors: r+ 6403f91

Thanks!

@pitdicker
Copy link
Contributor Author

Thanks for the review!

An other question. Many tests and private functions now have Unix names like mkdir and unlink, instead of create_dir and delete. Would it make sense to rename them?
It would make it a very little bit easier to find the function of a failing test, and to lift code up/down from sys::*::fs to fs.

@pitdicker
Copy link
Contributor Author

I don't think there is much refactoring we can do for metadata, but will think about it.

@bors
Copy link
Contributor

bors commented Feb 14, 2016

⌛ Testing commit 6403f91 with merge ebfe867...

@bors
Copy link
Contributor

bors commented Feb 14, 2016

💔 Test failed - auto-win-msvc-32-opt

@pitdicker
Copy link
Contributor Author

This looks like a timeout, the other Windows builds succeeded

@alexcrichton
Copy link
Member

@bors: retry

On Sun, Feb 14, 2016 at 7:41 AM, pitdicker [email protected] wrote:

This looks like a timeout, the other Windows builds succeeded

—
Reply to this email directly or view it on GitHub
#31630 (comment).

bors added a commit that referenced this pull request Feb 14, 2016
@bors
Auto merge of #31630 - pitdicker:read_link, r=alexcrichton
9b367d9
@bors
Copy link
Contributor

bors commented Feb 14, 2016

⌛ Testing commit 6403f91 with merge 9b367d9...

@bors
Copy link
Contributor

bors commented Feb 14, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit 6403f91 into rust-lang:master Feb 14, 2016
@llogiq llogiq mentioned this pull request Feb 14, 2016
Closed
@pitdicker pitdicker deleted the read_link branch February 28, 2016 07:57
Ryman added a commit to Ryman/walkdir that referenced this pull request Oct 31, 2016
@Ryman
Remove reference to a bug which has been fixed
da8c02d 
Since the minimum supported rustc version has this fix[1] we can
remove the reference.

[1] rust-lang/rust#31630
@Ryman Ryman mentioned this pull request Oct 31, 2016
Merged
@eddyb eddyb mentioned this pull request Jan 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pitdicker @rust-highfive @alexcrichton @bors @brson