Skip to content
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

Conversation

JohnDeHelian
Copy link

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
Copy link
Member

bagder commented Sep 20, 2017

So this PR replaces #1800 ?

@JohnDeHelian
Copy link
Author

JohnDeHelian commented Sep 20, 2017 via email

@bagder
Copy link
Member

bagder commented Sep 20, 2017

No problems at all!

@JohnDeHelian
Copy link
Author

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
Copy link
Author

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

@bagder
Copy link
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
Changed Curl_get_pathname to accept /~/ to specify relative path.
@JohnDeHelian
Copy link
Author

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
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
Changed Curl_get_pathname to accept /~/ to specify relative path.
ssh-libssh.c also uses same function now.
@JohnDeHelian
Copy link
Author

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
Copy link
Author

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
Copy link
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
Copy link
Author

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
Copy link
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
Copy link
Author

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
Copy link
Author

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
Copy link
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
Copy link
Author

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.

Rebased (Cherry picked changes) into master @912324024b3be13ef9c3eedfc437a9fcb7961228
@JohnDeHelian
Copy link
Author

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
Copy link
Member

bagder commented Dec 8, 2017

Yes, this looks perfect!

@bagder bagder closed this in a4a56ec Dec 9, 2017
@bagder
Copy link
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.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants