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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@JohnDeHelian

JohnDeHelian commented Sep 20, 2017

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).

@bagder bagder added the SCP/SFTP label Sep 20, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 20, 2017

Member

So this PR replaces #1800 ?

Member

bagder commented Sep 20, 2017

So this PR replaces #1800 ?

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Sep 20, 2017

JohnDeHelian commented Sep 20, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 20, 2017

Member

No problems at all!

Member

bagder commented Sep 20, 2017

No problems at all!

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Sep 21, 2017

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.

JohnDeHelian commented Sep 21, 2017

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.

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Oct 4, 2017

When do changes normally get merged? Is there anything else I have to do to get this merged into the project?

JohnDeHelian commented Oct 4, 2017

When do changes normally get merged? Is there anything else I have to do to get this merged into the project?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 5, 2017

Member

I'd like to see at least one test case that uses the new functionality

The new function get_realPathname (which I think is a weird name, since the other function is also returning a real name, this however merges a relative name with another path) has a lot of duplicated code with the get_pathname function.

Wouldn't it be possible to let get_pathname get an optional third argument that is the homedir, that gets used if the 'path' argument is not absolute or perhaps even if non-NULL? Then we could manage with a single function for this!

Member

bagder commented Oct 5, 2017

I'd like to see at least one test case that uses the new functionality

The new function get_realPathname (which I think is a weird name, since the other function is also returning a real name, this however merges a relative name with another path) has a lot of duplicated code with the get_pathname function.

Wouldn't it be possible to let get_pathname get an optional third argument that is the homedir, that gets used if the 'path' argument is not absolute or perhaps even if non-NULL? Then we could manage with a single function for this!

JohnDeHelian pushed a commit to JohnDeHelian/curl that referenced this pull request Dec 4, 2017

John DeHelian
Allow quoted commands to use relative paths #1900
Changed Curl_get_pathname to accept /~/ to specify relative path.
@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 4, 2017

Daniel
I noticed you re-factored the ssh.c file to remove the get_path name. So here's what I did. I fast forwarded this branch to your current master. Then added the changes back, this time using your suggestion to pass in the home directory to the Curl_get_pathname function. If the path begins with /~/ it will replace it with the homedir. I would prefer to also accept no prefix. But since the same function is used for commands like chmod it would have been difficult.

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.

JohnDeHelian commented Dec 4, 2017

Daniel
I noticed you re-factored the ssh.c file to remove the get_path name. So here's what I did. I fast forwarded this branch to your current master. Then added the changes back, this time using your suggestion to pass in the home directory to the Curl_get_pathname function. If the path begins with /~/ it will replace it with the homedir. I would prefer to also accept no prefix. But since the same function is used for commands like chmod it would have been difficult.

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.

JohnDeHelian pushed a commit to JohnDeHelian/curl that referenced this pull request Dec 4, 2017

John DeHelian
Allow quoted commands to use relative paths #1900
Changed Curl_get_pathname to accept /~/ to specify relative path.
ssh-libssh.c also uses same function now.

JohnDeHelian pushed a commit to JohnDeHelian/curl that referenced this pull request Dec 4, 2017

John DeHelian
Allow quoted commands to use relative paths #1900
Changed Curl_get_pathname to accept /~/ to specify relative path.
ssh-libssh.c also uses same function now.
@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 4, 2017

Test case:
curl --user user_name:Password sftp://url_address/~/RESPONSE/test -v -T gm -Q '-rename /~/RESPONSE/test /~/RESPONSE/test1'
Folder RESPONSE is in user login path.

It solves this user's problem:
https://curl.haxx.se/mail/archive-2011-01/0021.html

JohnDeHelian commented Dec 4, 2017

Test case:
curl --user user_name:Password sftp://url_address/~/RESPONSE/test -v -T gm -Q '-rename /~/RESPONSE/test /~/RESPONSE/test1'
Folder RESPONSE is in user login path.

It solves this user's problem:
https://curl.haxx.se/mail/archive-2011-01/0021.html

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 5, 2017

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.

JohnDeHelian commented Dec 5, 2017

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.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 6, 2017

Member

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.

Member

bagder commented Dec 6, 2017

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.

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 7, 2017

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.

JohnDeHelian commented Dec 7, 2017

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.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 7, 2017

Member

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.

Member

bagder commented Dec 7, 2017

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.

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 7, 2017

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.

JohnDeHelian commented Dec 7, 2017

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.

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 8, 2017

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.

JohnDeHelian commented Dec 8, 2017

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.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 8, 2017

Member

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.

Member

bagder commented Dec 8, 2017

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.

@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 8, 2017

I'm not sure I understand.
So I have a branch sftpQuoteAllowRelativePath that when I push from my local branch causes another build under this pull request 1900. So should I update my forked repository to reflect the lastest master (now 9123) then rebase sftpQuoteAllowRelativePath to the new master and push it? Or should I reset it to master and then apply my changes. Or should I cherry pick ? I'm really unfamiliar with all this so if you could give me some detailed instruction it may help. I will try again.

JohnDeHelian commented Dec 8, 2017

I'm not sure I understand.
So I have a branch sftpQuoteAllowRelativePath that when I push from my local branch causes another build under this pull request 1900. So should I update my forked repository to reflect the lastest master (now 9123) then rebase sftpQuoteAllowRelativePath to the new master and push it? Or should I reset it to master and then apply my changes. Or should I cherry pick ? I'm really unfamiliar with all this so if you could give me some detailed instruction it may help. I will try again.

John DeHelian
Allow quoted commands to use relative paths #1900
Rebased (Cherry picked changes) into master @912324024b3be13ef9c3eedfc437a9fcb7961228
@JohnDeHelian

This comment has been minimized.

Show comment
Hide comment
@JohnDeHelian

JohnDeHelian Dec 8, 2017

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.

JohnDeHelian commented Dec 8, 2017

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.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 8, 2017

Member

Yes, this looks perfect!

Member

bagder commented Dec 8, 2017

Yes, this looks perfect!

@bagder bagder closed this in a4a56ec Dec 9, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 9, 2017

Member

Thanks a lot for your hard work on this!

Member

bagder commented Dec 9, 2017

Thanks a lot for your hard work on this!

@lock lock bot locked as resolved and limited conversation to collaborators May 9, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.