Skip to content

Fix recursion when listing directories #20

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

Merged
merged 4 commits into from
Mar 11, 2014
Merged

Conversation

woiwa
Copy link
Contributor

@woiwa woiwa commented May 25, 2013

Sftp::scanDirectory() always recursively scans a whole directory tree even if no recursion is wanted. This patch uses scandir instead of the recursive method-call to avoid this.

Because this patch uses scandir() there might be the same problems described in an issue and pull request.

This is my first pull request ever, so feel free to point out things I made wrong.

Sftp::scanDirectory() always recursively scans a whole directory tree even if no recursion is wanted. This patch uses scandir instead of the recursive method-call to avoid this.
@ghost
Copy link

ghost commented Oct 18, 2013

Can this please be merged?

@h4cc
Copy link
Collaborator

h4cc commented Mar 4, 2014

@woiwa Thanks for your contribution.
I cant merge this because:

  • Using the silence operator is inappropriate. Try using the "is_dir()" function to it.
  • Have you updated the tests? please try to make them work on your fork + travis first. (Travis config has been fixed.)

@woiwa
Copy link
Contributor Author

woiwa commented Mar 4, 2014

@h4cc
Yes, of course using @scandir just to check if something is a directory isn't nice. But it isn't my invention - I have taken it from the existing code. (see Ssh\Sftp::scanDirectory, starting at line 250). In the comment there is an explanation why is_dir() isn't working.

The only thing I changed is the arrangement of some commands because the original code walks down recursively to check for directories... even if the $recursive argument is set to false. So listing a directory with many subdirectories will last very long, even with $recursive = false.

I didn't update any tests - and you're right, providing a test would have been better. But there aren't Testcases for the Sftp-Class in the original code - and I didn't change any functionality. The problem was fixed within a short time, and I thought setting up a brand new test for "Check if scanDirectory() scans recursively even if $recursive is set to false" isn't worth it. :-)

The test will be a bit difficult to write, because the original method does the recursion because it hits a loop before processing the $recursive argument - but the return value is correct and the same value as in my patch. And it is difficult (I don't know, maybe impossible) to test the "internals" of scanDirectory() without some bigger changes in the class. (Maybe I am wrong).

I will update my fork and look what travis says. I don't have used travis until now, so that's a good opportunity to check it out. :-)

@woiwa
Copy link
Contributor Author

woiwa commented Mar 5, 2014

@h4cc
Thank you for your hint! The build passed now.
My .travis.yml contains your suggested line, which isn't present in the original repo as far as I can see. I don't know if this matters.

h4cc added a commit that referenced this pull request Mar 11, 2014
Fix recursion when listing directories.
@h4cc h4cc merged commit a78aff7 into Herzult:master Mar 11, 2014
@h4cc
Copy link
Collaborator

h4cc commented Mar 11, 2014

Thanks for your effort.

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