calling mkDirs with something like '/foo' will fail because of the split call.

Again it would be nice to switch to posixpath functions here.

Comment on attachment 745805 [details] [diff] [review] use posixpath Review of attachment 745805 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a couple little tweaks. ::: mozdevice/mozdevice/devicemanager.py @@ +235,4 @@ > WARNING: does not create last part of the path. For example, if asked to > create `/mnt/sdcard/foo/bar/baz`, it will only create `/mnt/sdcard/foo/bar` > """ > + (containing, _) = posixpath.split(filename) If you only care about the first part, you can just use posixpath.dirname(). @@ +240,1 @@ > parts = filename.split('/') So this could use posixpath, although admittedly there is no function to split a path into all its components. I'm okay with keeping split("/") in this case as long as the path goes through normpath() first.

I don't want to change that split because (as you said) there's no equivalent in posixpath and the function would grow unnecessarily.

Comment on attachment 748179 [details] [diff] [review] normalize path in mkdirs Review of attachment 748179 [details] [diff] [review]: ----------------------------------------------------------------- Great! The only thing this is missing is a test. Please modify sut_mkdir.py to test the creation of a directory off of root (make sure the test fails without this patch and passes with it).

I should say, please do the test as a separate patch and r? me. I don't think we can verify this on a real device (via sut_tests) because I believe / is often mounted read-only.

Comment on attachment 748181 [details] [diff] [review] improve dmsut.pushdir logic Review of attachment 748181 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozdevice/mozdevice/devicemanagerSUT.py @@ +375,2 @@ > > + if (basename == ""): This is not quite right. parts[1] is actually the root directory, e.g. for /a/b/c it would be 'a'. In fact, this breaks sut_tests/test_push2.py (please run *all* tests before submitting patches!). I'm not sure of the best way to fix this; perhaps this whole loop needs to be rethought.

I reverted to the split method, as this works, but I kept the cleaner join() method. All tests pass now.

Oh and I had to take off the starting slash because posixpath.join("/a/b", "/c") == "/c" :).

Fails without the patches, passes with.

Comment on attachment 757109 [details] [diff] [review] use posixpath in dmsut.pushdir Review of attachment 757109 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this makes the function clearer and it appears to work fine. Just a few suggestions to make it even more clear and Pythonic. :) No need for rereview if you just make those changes. ::: mozdevice/mozdevice/devicemanagerSUT.py @@ +369,5 @@ > existentDirectories = [] > for root, dirs, files in os.walk(localDir, followlinks=True): > + _, subpath = root.split(localDir) > + if subpath and subpath[0] == '/': > + subpath = subpath[1:] You can replace both those lines with subpath = subpath.lstrip('/') @@ +374,2 @@ > for f in files: > + remoteRoot = posixpath.join(remoteDir, subpath) This doesn't actually have to be done on each iteration; might as well move it above the for loop. @@ +377,2 @@ > > + if (subpath == ""): Since you're editing this line anyway, take out the extraneous parentheses.

Comment on attachment 748182 [details] [diff] [review] update sut_push test to use proper paths Review of attachment 748182 [details] [diff] [review]: ----------------------------------------------------------------- This works fine now with your latest fix to pushDir().

Comment on attachment 757131 [details] [diff] [review] test mkdirs for /foo Review of attachment 757131 [details] [diff] [review]: ----------------------------------------------------------------- Heh I keep forgetting that mkDirs() doesn't create the last path element, so mkDirs("/foo") is kind of pointless, but at least it doesn't error out now. :) Thanks!

(In reply to Mark Côté ( :mcote ) from comment #16) > Comment on attachment 757131 [details] [diff] [review] > test mkdirs for /foo > > Review of attachment 757131 [details] [diff] [review]: > ----------------------------------------------------------------- > > Heh I keep forgetting that mkDirs() doesn't create the last path element, so > mkDirs("/foo") is kind of pointless, but at least it doesn't error out now. > :) Thanks! Haha me too, I actually spent time "debugging" this!