-
Notifications
You must be signed in to change notification settings - Fork 149
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 symlink handling #764
base: main
Are you sure you want to change the base?
Fix symlink handling #764
Conversation
There is another bug in here when dealing with symlink targets in .git if the target file exists. I erroneously assumed edit: fixed |
Previously a sub-path of an ignored directory was not considered to be ignored, e.g. is_ignored returned False for a file build/hello.py even if _all_ignored_files contained Path("build"). Instead of just checking if the path is in the ignored files we also have to check if it is inside of an ignored directory. Additionally this commit fixes a bug that surfaced: when building the _all_ignored_files set all_files contains an empty string as its last element. This leads to Path(".") being an element in _all_ignored_files, i.e. ignore everything. Using all but the last element from all_files fixes that.
These tests assume the interpretation of the REUSE specification that: 1. a symlink pointing to a covered file is considered to be the same file as the covered file and can therefore be ignored. 2. a symlink pointing to a file that is not a covered file is itself considered to be a covered file and should not be ignored, unless the symlink itself is ignored by other means.
9ae09f6
to
fbb5ed2
Compare
fbb5ed2
to
13e4fa7
Compare
@wtraylor Feel free to try this out on a real-world project if you have one available. This was less straight-forward to implement than I thought it would be and there might still be some edge-cases I didn't consider. I tried to implement the symlink handling in a way I thought to be intuitive, but that might also differ from your expectations. So some testing would be great :) |
Hi @matrss This PR looks qualitatively really good. We'll need some time to review it, though. It will also need a change to the specification. Although this version of the spec is not yet live, the current development version says that symlinks are ignored. |
Many thanks for implementing this feature, @matrss !
The implementation appears to assume that all symlinks require dot-license files. For example:
I guess it makes sense to require dot-license files for symlinks because the symlink targets may not be available (i.e., symlink is broken, for example if annexed content is not in the working copy). However, I suggest to make this more transparent to the user—for example with these messages:
Point 3 stems from my experience that it can easily happen to annex a file by accident. Then you might end up with an annexed Finally, there is the question of how to deal with symlink folders. This does not fall into the use case of
I think this behavior is good. It doesn’t make sense for |
This PR implements the heuristics for handling symlinks I've described in more detail in #627 (comment) and #627 (comment).
Basically, this is based on an interpretation of the REUSE specification that:
This fixes #627 while not breaking #202 for the described situation, since it would fall into category 1 above.