path fixing when copying folders from windows #88

moscoquera · 2017-08-30T15:42:31Z

as window's file separator is '', when copying folders from windows the destination path get malformed, replacing '' with '/' is enough to solve it.

as window's file separator is '\', when copying folders from windows the destination path get malformed, replacing '\' with '/' is enough to solve it.

pkittenis · 2017-08-30T22:38:17Z

Hi there, thanks for the PR.

If I understand correctly, os.path.sep is \ on Windows but SFTP file paths always use /, therefore file copy with directory paths breaks on Windows?

In that case it would be more appropriate to not use os.path in the first place and always use / instead as that's what the protocol uses.

moscoquera · 2017-08-31T00:40:15Z

Hi.

imagine a windows folder called parent and with 2 files inside called childrenA.txt and childrenB.txt:

parent:
childrenA.txt
childrenB.txt

to access them python will use "parent\childrenA.txt" and "parent\childrenB.txt", if you are trying to copy parent to a remote sever, you will get 3 files:

a folder called "parent".
an two files called "parent\childrenA.txt" and "parent\childrenB.txt"

when the expected behavior will be a single folder called "parent" with 2 files inside.

pkittenis · 2017-09-01T16:11:14Z

Yes, I understand.

The change needed here is for remote_path (only remote path) to always use / for joining. Ie remote_path = '/'.join([remote_dir, file_name]) in both copy_file and copy_remote_file.

Replacing os.path, which is \ on Windows, with / after the path is generated does not make much sense. So if you'd like to make that change, I'd be happy to review it.

when copying files from or to a windows device, sub-directories structure is not being to be correctly replicated as linux doesn't use '\' as file separator. forcing the remote path to use '/' guarantees the paths are correctly replicated.

coveralls · 2017-09-19T02:21:14Z

Coverage remained the same at 88.914% when pulling a310fef on moscoquera:master into b53799d on ParallelSSH:master.

moscoquera · 2017-09-19T02:24:07Z

Hi again. sorry for the very late response, i have been busy upgrading a project. i just did the requested changes.

i'd be happy if u can review it.

coveralls · 2017-09-19T02:26:33Z

Coverage decreased (-3.2%) to 89.367% when pulling 008a45a on moscoquera:master into 52e7974 on ParallelSSH:master.

pkittenis

No worries, thanks for looking into this!

Reviewed, comments in line.

pkittenis · 2017-09-21T12:47:26Z

pssh/ssh_client.py

@@ -364,7 +364,7 @@ def _copy_dir(self, local_dir, remote_dir, sftp):
        file_list = os.listdir(local_dir)
        for file_name in file_list:
            local_path = os.path.join(local_dir, file_name)
-            remote_path = os.path.join(remote_dir, file_name)
+            remote_path = remote_dir+'/'+file_name


This is better written as '/'.join([remote_dir, file_name]).

String concatenation creates new strings on every operation (2 temporary + 1 final in the above case), which is a lot slower than a single join call. This is done for every sub-dir and for every client in host list, so adds up quickly.

but, isn't it creating a new array each time?

pkittenis · 2017-09-21T13:03:27Z

pssh/ssh_client.py

@@ -455,7 +455,7 @@ def copy_remote_file(self, remote_file, local_file, recurse=False,
    def _copy_remote_dir(self, file_list, remote_dir, local_dir, sftp):
        for file_name in file_list:
            remote_path = os.path.join(remote_dir, file_name)
-            local_path = os.path.join(local_dir, file_name)
+            local_path = local_dir+'/'+file_name


remote_path needs changing here, not local_path (local_path is local to client and platform specific). Looking at paramiko source confirms this.

Also same comment as above about '/'.join.

replacing + on string concatenation by join.

moscoquera · 2017-09-24T03:11:58Z

changes are made

pkittenis · 2017-09-26T17:02:38Z

pssh/ssh_client.py

@@ -364,7 +364,7 @@ def _copy_dir(self, local_dir, remote_dir, sftp):
        file_list = os.listdir(local_dir)
        for file_name in file_list:
            local_path = os.path.join(local_dir, file_name)
-            remote_path = os.path.join(remote_dir, file_name)
+            remote_path = '/'.join([remote_dir,file_name])


Flake errors: pssh/ssh_client.py:367:47: E231 missing whitespace after ','

Per https://travis-ci.org/ParallelSSH/parallel-ssh/jobs/279101695

pkittenis · 2017-09-26T17:02:57Z

pssh/ssh_client.py

-            remote_path = os.path.join(remote_dir, file_name)
-            local_path = os.path.join(local_dir, file_name)
+            remote_path = '/'.join([remote_dir, file_name])
+            local_path = os.path.join(local_dir,file_name)


Ditto - https://travis-ci.org/ParallelSSH/parallel-ssh/jobs/279101695

pkittenis · 2017-09-26T17:04:06Z

Thanks, changes look good. Only couple minor changes needed for the flake check to pass, see in line comments and CI job, looks ready otherwise.

Thanks again for the PR.

coveralls · 2017-09-26T17:56:33Z

Coverage increased (+0.7%) to 89.593% when pulling ba641ec on moscoquera:master into b366c60 on ParallelSSH:master.

coveralls · 2017-09-26T17:58:39Z

Coverage increased (+0.5%) to 89.367% when pulling ba641ec on moscoquera:master into b366c60 on ParallelSSH:master.

moscoquera · 2017-09-26T18:00:01Z

sure, no problem.

pkittenis · 2017-10-03T16:26:41Z

Thanks for making these changes :)

FYI, have found a couple more places where remote path needs changing for windows which are used when recurse is enabled. Those changes, along with this fix, will be in next release.

path fixing when copying folders from windows

af970de

as window's file separator is '\', when copying folders from windows the destination path get malformed, replacing '\' with '/' is enough to solve it.

Merge branch 'master' into master

008a45a

pkittenis requested changes Sep 22, 2017

View reviewed changes

_copy_remote_dir remote path correction

f50ea46

replacing + on string concatenation by join.

pkittenis reviewed Sep 26, 2017

View reviewed changes

moscoquera added 2 commits September 26, 2017 12:53

pep8 compilance

6238979

Merge branch 'master' into master

ba641ec

pkittenis approved these changes Sep 27, 2017

View reviewed changes

pkittenis merged commit 011dd0f into ParallelSSH:master Sep 27, 2017

path fixing when copying folders from windows #88

path fixing when copying folders from windows #88

Uh oh!

Conversation

moscoquera commented Aug 30, 2017

Uh oh!

pkittenis commented Aug 30, 2017

Uh oh!

moscoquera commented Aug 31, 2017

Uh oh!

pkittenis commented Sep 1, 2017

Uh oh!

coveralls commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moscoquera commented Sep 19, 2017

Uh oh!

coveralls commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkittenis left a comment

Choose a reason for hiding this comment

Uh oh!

pkittenis Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

moscoquera Sep 24, 2017

Choose a reason for hiding this comment

Uh oh!

pkittenis Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

moscoquera commented Sep 24, 2017

Uh oh!

pkittenis Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

pkittenis Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

pkittenis commented Sep 26, 2017

Uh oh!

coveralls commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moscoquera commented Sep 26, 2017

Uh oh!

pkittenis commented Oct 3, 2017

Uh oh!

Uh oh!

coveralls commented Sep 19, 2017 •

edited

Loading

coveralls commented Sep 19, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading