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
Allow quoted commands to use relative paths #1900
Conversation
So this PR replaces #1800 ? |
Yeah sorry if it creates confusion. I wasn't sure how to bring it up to date with the current master. But I think I figured iut how to do a rebase.
John
|
No problems at all! |
Hi Daniel I see that tests 1208 and 608 fail in certain runs. I simply did make test to run the test and it passed all the tests. But apparently there are different ways to run the tests. How can I reproduce these errors on my system? I see one is a memory leak so I will try to hunt that down. But need a means to reproduce the error. |
When do changes normally get merged? Is there anything else I have to do to get this merged into the project? |
I'd like to see at least one test case that uses the new functionality The new function Wouldn't it be possible to let |
dbb835e
to
d377ee1
Compare
Changed Curl_get_pathname to accept /~/ to specify relative path.
Daniel I didn't know how to create a generic test case. I couldn't get to the unit test home directory. But the test case would involve a post quote renaming of a file with a relative path to the home directory. The relative path would be preceded by /~/ and the file would have to exist in the path relative to the unit test home directory. |
d377ee1
to
7d24f67
Compare
Changed Curl_get_pathname to accept /~/ to specify relative path. ssh-libssh.c also uses same function now.
7d24f67
to
c218dfd
Compare
Changed Curl_get_pathname to accept /~/ to specify relative path. ssh-libssh.c also uses same function now.
Test case: It solves this user's problem: |
Let me know if there's anything else I have to do to get this merged into master. I'm still not familiar with this process. I couldn't figure out how to retrieve the login path for unit testing. If you can show me how I can attempt to write a unit test for renaming a file with a relative path. |
github says "This branch has conflicts that must be resolved" still, so those would be good to have fixed first. Then, once the patch is fine and we agree about it, someone (most likely me) will merge it to master. |
OK I think I fixed the conflicts. It's weird because it passed all tests when I checked it in. I re-based to your current master and resolved the conflicts. Hopefully this will work. This is a very painful process. I'd like to find the best way to resolve the conflicts. The on line resolver is not very easy to use. |
My advice: don't use the web UI for conflicts. Rebase your local git branch, squash/fixup the commits that are better off as single units, fix the conflicts if any, and finally force push your work again when you've cleaned it up. |
That's what I did originally. But then it still showed conflicts so I wasn't sure if I needed to do it on line. Next time I'll try res-basing again. I think some of the problem was that my forked branch wasn't up to date with the main repository. I finally figured out (I think) how to get that sync'd up. I had to do it from within github. |
OK Looks clean now. The main reason I need this is that although we can place a file on the server with a relative path we cannot then rename it with that same relative path. The Curl_getworkingpath gets the full path for actions like downloading and uploading files but the Curl_get_pathname didn't so commands like rename didn't work with a relative path specified with the /~/ prefix. I would have liked to have make it work for no / up front too but this function is also used for commands like chmod where the parameters aren't actually paths but rather properties. Let me know if you'd like to discuss it any further and what the best means of communication with you is. |
This branch now contains 20(!) commits and github says "This branch cannot be rebased due to conflicts". I don't think that fits my definition of clean! :-) You should make your origin/master branch up-to-date and then rebase your branch on top of that (and then it should only have one or two commits above the master branch), then force-push your branch here so that we only see those extra commits of yours and no others' commits. |
I'm not sure I understand. |
Rebased (Cherry picked changes) into master @912324024b3be13ef9c3eedfc437a9fcb7961228
2e63267
to
d29e14c
Compare
I think I did it this time - can you check before all the tests finish if I only have one commit now? OK I refreshed and saw only one commit - I will keep my fingers crossed. |
Yes, this looks perfect! |
Thanks a lot for your hard work on this! |
When using the sftp quoted rename function it was necessary to know the full path on the server in order to work. This change allows using a relative path.
Note: This is another attempt at merging this in after I figured out how to run the tests. I couldn't figure out how to add a test for the relative path. In order to do it you need to know in the test script the full path. I tried using /home/$USER/Test as a path but it couldn't find it. If you could please add a test where the file is uploaded using a relative path and the rename uses a relative path (no / in front).