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.

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

Fix #20925, cp not preserving permissions #27295

Merged
merged 3 commits into from
Jun 2, 2018
Merged

Fix #20925, cp not preserving permissions #27295

merged 3 commits into from
Jun 2, 2018

Conversation

staticfloat
Copy link
Sponsor Member

Rebasing @tkelman's branch here, as I can confirm that it fixes #26907 (comment)

@ararslan ararslan added kind:bugfix This change fixes an existing bug domain:filesystem Underlying file system and functions that use it labels May 28, 2018
@staticfloat
Copy link
Sponsor Member Author

@vtjnash reading your comment, it appears you were asking for a mode argument. Something like this:

+"""
+    sendfile(src, dst; mode = nothing)
+
+Copy file `src` to path `dst`, creating `dst` with the given `mode`.  If `mode`
+is left at the default value of `nothing`, the mode of the `src` file is used.
+"""
+function sendfile(src::AbstractString, dst::AbstractString;
+                  mode::Union{Unsigned,Nothing}=nothing)
     src_open = false
     dst_open = false
     local src_file, dst_file
     try
         src_file = open(src, JL_O_RDONLY)
         src_open = true
-        dst_file = open(dst, JL_O_CREAT | JL_O_TRUNC | JL_O_WRONLY, filemode(src_file))
+        if mode == nothing
+            mode = filemode(src_file)
+        end
+        dst_file = open(dst, JL_O_CREAT | JL_O_TRUNC | JL_O_WRONLY, mode)
         dst_open = true

To be honest, I don't see why you would want to do this. For this to be useful, we would then add on mode kwargs to cp() and mv(), but it doesn't make sense to me why we would do that and not simply require users to chmod() after a cp() or an mv(); the fusion of these two operations doesn't make much sense to me unless you are thinking about things from the perspective of "I know that a mv from one filesystem to another is going to require an open() anyway, so I can save some work by doing it this way" in which case I would argue you should just write your own function instead of using these ones.

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 28, 2018

Mainly that open with mask typically gives a different result than chmod with mask. But I think what you're doing with this PR handles that.

@staticfloat
Copy link
Sponsor Member Author

Great. Then I'll assume this is an easy review for you and request one. :)

@staticfloat staticfloat requested a review from vtjnash May 29, 2018 00:56
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Seems like a safe enough change. In the future, we may even want to use the new, high-performance, libuv wrapper uv_fs_copyfile

@staticfloat
Copy link
Sponsor Member Author

Great, will merge once it's green then.

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 29, 2018

Refs nodejs/node#15394

@KristofferC
Copy link
Sponsor Member

KristofferC commented May 31, 2018

https://travis-ci.org/JuliaLang/julia/jobs/384922807

Error in testset file:
Test Failed at /tmp/julia/share/julia/test/file.jl:91
  Expression: filemode(tmpfile) == filemode(tmpfile2)
   Evaluated: 0x000081ff == 0x000081fd
Error in testset file:
Test Failed at /tmp/julia/share/julia/test/file.jl:95
  Expression: filemode(tmpfile) == filemode(tmpfile2)
   Evaluated: 0x000081c7 == 0x000081c5
ERROR: LoadError: Test run finished with errors
in expression starting at /tmp/julia/share/julia/test/runtests.jl:59

@staticfloat
Copy link
Sponsor Member Author

Thanks Kristoffer, I've added umask-aware tests.

@staticfloat
Copy link
Sponsor Member Author

Windows failures unrelated, and we passed the file test on the windows builds anyway, so I'm calling this one good.

@staticfloat staticfloat merged commit c8ce43a into master Jun 2, 2018
@martinholters martinholters deleted the sf/fix20925 branch June 2, 2018 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:filesystem Underlying file system and functions that use it kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mv loses file permissions on some linux systems
5 participants