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

Add force flag to delete-tree #120

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

jlesquembre
Copy link
Contributor

Closes #119

Please answer the following questions and leave the below in as part of your PR.

@borkdude
Copy link
Contributor

The posix stuff only works on linux/mac, perhaps there is another way which is more OS-agnostic.
Also, no need to set the file permission when the file already has the right permission, perhaps?

@jlesquembre
Copy link
Contributor Author

The windows part, sure, I have to fix it, any tips on how to test it without a windows computer?

Also, no need to set the file permission when the file already has the right permission, perhaps?

If the directory already have the right permissions (OWNER_WRITE and OWNER_EXECUTE) it doesn't change the permissions, or am I missing something?

@borkdude
Copy link
Contributor

you're right

@borkdude
Copy link
Contributor

Maybe File.setWriteable etc will suffice on Windows?

@jlesquembre
Copy link
Contributor Author

Maybe File.setWriteable etc will suffice on Windows?

Looks like that :-) :

https://github.com/jlesquembre/fs/actions/runs/7268334832

@borkdude
Copy link
Contributor

  • Would there be any benefit of not changing the file permission on windows if the file is already writable? Is there a situation where this might throw an exception where the delete would have happened successfully, without first changing file permissions?
  • Why not unify the code for windows and linux?

@jlesquembre
Copy link
Contributor Author

jlesquembre commented Dec 20, 2023

Would there be any benefit of not changing the file permission on windows if the file is already writable?

I don't think so, File.setWriteable seems idempotent

Is there a situation where this might throw an exception where the delete would have happened successfully, without first changing file permissions?

The only case where we touch the permissions, and a delete would have happened successfully is on windows if a file is already writable. We call .setWriteable again, but it doesn't seem to make any difference.

Why not unify the code for windows and linux?

I think mean here, right? https://github.com/jlesquembre/fs/blob/0468ac76bef9f054400de9da5030203c2351c15a/src/babashka/fs.cljc#L583-L584

The issue is that I don't want to touch the permissions if not necessary (with the exception of already writable files on windows, since it makes sense to me to just enforce that is writable and avoid calling .canWrite).

But the rules are different on posix vs windows:

  • posix:
    • Read-only files can be deleted
    • Files can be deleted only if the parent directory has the "rwx" permissions
  • windows:
    • Read-only files cannot be deleted
    • Parent directory permissions don't affect the delete operation of the child file.

I wrote some tests on my fork to test those assumptions, I can add it to the PR if it makes sense:

jlesquembre@c4d4ccd

https://github.com/jlesquembre/fs/actions/runs/7281870406

@borkdude
Copy link
Contributor

OK, let's add those tests and I'll merge

@borkdude
Copy link
Contributor

Btw, this issue might also be relevant to the above:

borkdude/deps.clj#120 (comment)

@jlesquembre
Copy link
Contributor Author

I added the tests, and a comment point out that the implementation is based on those assumptions.

@borkdude borkdude merged commit da1ace0 into babashka:master Dec 21, 2023
7 checks passed
@borkdude
Copy link
Contributor

Thanks!

@jlesquembre jlesquembre deleted the jl/delete-tree-force branch December 21, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

with-temp-dir can raise a AccessDeniedException exception
2 participants