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

storage/url: fixes the bug where some part of destination path is removed by cp #456

Merged
merged 15 commits into from
Jul 22, 2022
Merged

storage/url: fixes the bug where some part of destination path is removed by cp #456

merged 15 commits into from
Jul 22, 2022

Conversation

kucukaslan
Copy link
Contributor

@kucukaslan kucukaslan commented Jul 1, 2022

…oved by cp

The cause of problem is a little bit weird.

(url).SetRelative method was given the source (abs.) path as it was provided in command line arguments including the wildcard characters(* and ?). Hence it determined relative path incorrectly, where "expected" relative path became "../expected" which in turn caused the some part of destination to be lost (since path/filepath.Join converts e.g. /a/b/../c to /a/c)

We also need to identify whether the * and ? characters present in base strings are part of key or they are used for wildcard operation. So we need to know if the "raw" flag is given. To. solve this I changed (URL).SetRelative method to take argument of type URL rather than string.

Fixes #360

ps. This PR is aimed for the v2.1.0.

…oved by `cp`

The cause of problem is a little bit weird.

(url).SetRelative method was given a source (abs:) path as it was provided in command line arguments including the  wildcard characters(* and ?). Hence it determined relative path incorrectly, where "expected" relative path became "../expected" which in turn caused the some part of destination to be lost (since path/filepath.Join converts e.g. /a/b/../c to /a/c)

We also need to identify whether the * and ? characters present in base strings are part of key or they are used for wildcard operation. So we need to know if the "raw" flag is given. To. solve this I changed (URL).SetRelative method to take argument of type URL rather than string.

Fixes #360
@kucukaslan kucukaslan added the bug label Jul 1, 2022
@kucukaslan kucukaslan added this to the v2.1.0 milestone Jul 1, 2022
@kucukaslan kucukaslan requested a review from a team as a code owner July 1, 2022 13:50
@kucukaslan kucukaslan requested review from aykutfarsak and seruman and removed request for a team July 1, 2022 13:50
storage/url/url.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
storage/url/url.go Show resolved Hide resolved
storage/url/url.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
kucukaslan and others added 5 commits July 5, 2022 14:11
Co-authored-by: Selman Kayrancioglu <seruman@users.noreply.github.com>
use raw string in path strings of windows test.
add bucket names for the s3 key examples.
storage/url/url_test.go Outdated Show resolved Hide resolved
storage/url/url_test.go Outdated Show resolved Hide resolved
storage/url/url_test.go Outdated Show resolved Hide resolved
storage/url/url.go Show resolved Hide resolved
@kucukaslan kucukaslan requested a review from aykutfarsak July 6, 2022 16:21
CHANGELOG.md Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/cp_test.go Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
e2e/sync_test.go Outdated Show resolved Hide resolved
Co-authored-by: Aykut Farsak <aykutfarsak@gmail.com>
storage/url/url_test.go Outdated Show resolved Hide resolved
@kucukaslan kucukaslan requested a review from igungor July 18, 2022 14:33
@igungor igungor changed the title storage/url: fixes the bug where some part of destination path is rem… storage/url: fixes the bug where some part of destination path is removed by cp Jul 22, 2022
@igungor igungor merged commit 6727b11 into peak:master Jul 22, 2022
@kucukaslan kucukaslan deleted the fix-360-preserve-destination-path branch July 22, 2022 21:52
igungor pushed a commit that referenced this pull request Aug 22, 2022
Assumes that user has two copies of s5cmd one is default commandline app, second one is in the current directory.

A bucket name MUST be provided with -b flag.

User can optionally:
- add a key prefix with -k flag.
- specify hyperfine warmup counts with -w flag
- specify hyperfine runs counts with -r flag.

Example calls
`./benchmark.sh -b mcanktmpbuck`

`./benchmark.sh -w 1 -r 4 -b mcanktmpbuck -k example_key_prefix `

* allow user to specify what version/PR/commit of s5cmd will be used in the tests.

user can specify either version(tag), commit hash, or the PR number of the s5cmd that are to be used in the tests.
Two versions of the s5cmd will be used namely old and new. They can be specified by -o and -n flags, respectively.
Though version specified by o does not have to be older than that of specified by -n flag.

Default values of versions are v1.4.0 for old (-o), and v2.0.0 for new (-n).

Example execution that compares performance of #456 with the v1.4.0:
` ./benchmark.sh -b mys5cmbuck -n 456 -o v1.4.0 -k 1829`

Also note that user must provide a proper bucket which she has write/read/delete access.

Co-Authored-By: boraberke <67373739+boraberke@users.noreply.github.com>
Co-authored-by: MUHAMMED CAN KÜÇÜKASLAN <muhammedcankucukaslan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

part of a path removed by cp
4 participants