-
Notifications
You must be signed in to change notification settings - Fork 204
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 option to restrict rsync to one file system #1600
Conversation
Dear David, We need to discuss this in the team but I would say we wait until the upcoming release (round about 15th January) before we merge this. But it is just a (slow-down) feeling of myself. There is no strict technical reason. |
I excluded the Travis job for Python3.12 on a ppc64le machine because of an unfixed Issue on the site of TravisCI. I am in contact with their support. Open todos for the team:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the spelling consistent with the other options. Careful: Just editing here, I haven't had a chance to test this code.
Thanks for the review. Do you think we should wait until the (in two weeks) upcoming release?
I agree that consistence will improve code quality in most cases. But ( 😄 ) in this case my focus was more on resources. To the best of my recollection, the decision was not final, but we somehow agreed to stick to PEP8 as a minimal and in the Python world well known and accepted coding style. I would take this into account no matter that it will result in style-mixed code files. In the end I will stick to the teams opinion here. I am very flexible about that. ❤️ |
Thanks. As I said, I'm a newbie when it comes to python, so I'm not familiar with standards. I was copying existing code. |
You did a good job. No problem. It is usual that PRs are slightly modified by maintainers. |
OK, I updated to the latest upstream "dev". How do we decide about the naming style of the methods involved in this PR? Keep the old non-PEP8 naming convention (e.g. Any objects against merging this? |
I also prefer using underscores as word separator due to better readability even though we will have a mix for next years (or decades ;-) I guess. |
'that the user specified, and also the analogous recursion ' | ||
'on the receiving side during deletion. Also keep in mind ' | ||
'that rsync treats a "bind" mount to the same device as ' | ||
'being on the same filesystem.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
longest tooltip I ever saw ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are much more of this from-rsync-manpage-copy-and-pasted-tooltips. That is why I introduced the wrapping, to prevent translators from dealing with line breaks them selfs.
I don't like the idea of introducing a new naming scheme with for one function, so that it is different from all its siblings. But my objection is not very strong, so you may go ahead. |
Sounds reasonable to me. Also might confuse extern contributors. |
Now I set it back to camel case. 🤣 |
Added the ability to include the
--one-file-system
option on the rsync command.This will prevent rsync from going into file systems on other devices and systems.
Fix #1598