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

Stop following symlinks 216 #224

Merged
merged 6 commits into from
Feb 13, 2020

Conversation

docoleman
Copy link
Contributor

Add a check for symlinks when we check if a path is a directory and treat the symlink as a normal
file. Gitless will NOT follow the symlinks, because it could pull in the .git/ directory, files
outside of the repository or, in the worst case, the entire filesystem. This follows the behavior of
git. Git will commit symlinks into the repository and recreate the links on filesystems that support
it when the file is downloaded.

Add a check for symlinks when we check if a path is a directory and treat the symlink as a normal
file. Gitless will NOT follow the symlinks, because it could pull in the .git/ directory, files
outside of the repository or, in the worst case, the entire filesystem. This follows the behavior of
git. Git will commit symlinks into the repository and recreate the links on filesystems that support
it when the file is downloaded.
@@ -82,6 +82,16 @@ def onerror(func, path, unused_exc_info): # error handler for rmtree
logging.debug('Removed dir {0}'.format(path))


def symlink(src, dst, target_is_directory=False, *, dir_fd=None):
Copy link
Member

Choose a reason for hiding this comment

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

is the * useful for something in this case? It doesn't seem like you are using it and it breaks the python 2 tests

@docoleman
Copy link
Contributor Author

Huge combination of OS and Python version support... I need a matrix. Here's the best explanation I have: https://docs.python.org/3.5/library/os.html#os.symlink Python 3 supports os.symlink() on all platforms, but might throw exceptions in the implementation. In python 2, there is no method os.symlink() in the os module for Windows (https://docs.python.org/2.7/library/os.html#os.symlink). Hopefully my error handling catches all the cases now.

Catch AttributeError for os module missing symlink method in Python 2
implementations on Windows. All the flavors of error that could happen
in a symlink call should be handled now.
@docoleman
Copy link
Contributor Author

Huge combination of OS and Python version support... I need a matrix. Here's the best explanation I have: https://docs.python.org/3.5/library/os.html#os.symlink Python 3 supports os.symlink() on all platforms, but might throw exceptions in the implementation. In python 2, there is no method os.symlink() in the os module for Windows (https://docs.python.org/2.7/library/os.html#os.symlink). Hopefully my error handling catches all the cases now.

Tests & CI are not running? Or I can't see any results. Anything I need to do specifically to kick those off?

@@ -82,6 +82,22 @@ def onerror(func, path, unused_exc_info): # error handler for rmtree
logging.debug('Removed dir {0}'.format(path))


def symlink(src, dst, target_is_directory=False, dir_fd=None):
try:
os.symlink(src, dst, target_is_directory, dir_fd=dir_fd)
Copy link
Member

Choose a reason for hiding this comment

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

In python 2 the symlink method has a different signature. It doesn't look like you are using target_is_directory and dir_fd. Perhaps we can remove those parameters? Then tests should pass on python 2 and 3

@spderosso
Copy link
Member

Tests & CI are not running? Or I can't see any results. Anything I need to do specifically to kick those off?

Not sure what's going on. Looks like Travis is getting confused, but I can trigger builds manually.

I changed except foo, bar, baz to except (foo, bar, baz) (commit) because that was throwing an invalid syntax error when I triggered the build manually.

See the comment I made on the symlink signature. If we change the symlink call then tests should pass on both python 2 and python 3

Make the os.symlink method call signature the same as python 2. The
arguments and parameters for symlinking to directories and for the file
descriptor of the directory were removed. Those were added in python 3
	causing tests to fail in python 2.
@docoleman
Copy link
Contributor Author

Looks like tests are passing in both now. Next time I'll get them set up and running before I commit...

@spderosso spderosso merged commit eccad4a into gitless-vcs:master Feb 13, 2020
@spderosso
Copy link
Member

Thank you for fixing this bug!

@spderosso
Copy link
Member

Fixes #216

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.

2 participants