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

cp: copy to readonly file should fail on mac #5257

Closed
tertsdiepraam opened this issue Sep 7, 2023 · 2 comments · Fixed by #5261
Closed

cp: copy to readonly file should fail on mac #5257

tertsdiepraam opened this issue Sep 7, 2023 · 2 comments · Fixed by #5261
Labels

Comments

@tertsdiepraam
Copy link
Member

Found here: nushell/nushell#10097 (comment)

This test should probably fail on mac too (it fails correctly on all other platforms). It is a port from a nushell test.

#[test]
fn dest_no_permissions() {
    let ts = TestScenario::new(util_name!());
    let at = &ts.fixtures;

    at.touch("valid.txt");
    at.touch("invalid_perms.txt");
    at.set_readonly("invalid_perms.txt");

    ts.ucmd()
        .args(&["valid.txt", "invalid_perms.txt"])
        .fails()
        .stderr_contains("invalid_perms.txt")
        .stderr_contains("denied");
}
@tertsdiepraam
Copy link
Member Author

I have a theory for this failure:

  • We use clonefile on mac to do a copy-on-write copy.
  • According to comments in the code clonefile fails if the file already exists
  • If it does we remove the file and try again.

Instead we should probably check the permissions before we remove the file.

However, I can't test that theory because I don't have a mac. If anyone wants to try this out, please do!

@PGIII
Copy link
Contributor

PGIII commented Sep 7, 2023

Checked on my mac and this test does fail. I added a check if the dest file is read only and that does fix it. I will open a PR with the changes i made. Also it seems test_cp::test_cp_preserve_xattr is failing on my mac not sure if theres an open issue for that or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

3 participants