Repository: peak/s5cmd
Pull request #322 (2021-07-13 15:24:22)

storage/fs: change order of precedence for URL expansion
* Glob(*) expansion must have precedence over directory expansion.

If the source URL is both glob and directory, `s5cmd` only copies objects in the directory. Example setup:

```
testdir
|-- abc*
|   |-- file1.txt
|   `-- file2.txt
`-- abcd
    |-- file1.txt
    `-- file2.txt
```
```
❯ s5cmd cp 'testdir/abc*' s3://bigtmp/prefix/
cp testdir/abc*/file2.txt s3://bigtmp/prefix/abc*/file2.txt
cp testdir/abc*/file1.txt s3://bigtmp/prefix/abc*/file1.txt
```

URL expansion for fs.List() is working like following:
```
1) if given source URL is a directory, it only walks the directory.
2) if given source URL has glob, it expands glob.
3) if given source is not glob or directory, it sends stat call to single file.

```

Since `abc*` is a directory, currently `s5cmd` does not try to expand glob. I think we should change the order of precedence for dir/glob expansion.

I added tests for both cp/rm commands.



Patch:
diff --git a/storage/fs.go b/storage/fs.go
index f523acf..86d8327 100644
--- a/storage/fs.go
+++ b/storage/fs.go
@@ -39,6 +39,10 @@ func (f *Filesystem) Stat(ctx context.Context, url *url.URL) (*Object, error) {
 
 // List returns the objects and directories reside in given src.
 func (f *Filesystem) List(ctx context.Context, src *url.URL, followSymlinks bool) <-chan *Object {
+	if src.HasGlob() {
+		return f.expandGlob(ctx, src, followSymlinks)
+	}
+
 	obj, err := f.Stat(ctx, src)
 	isDir := err == nil && obj.Type.IsDir()
 
@@ -46,10 +50,6 @@ func (f *Filesystem) List(ctx context.Context, src *url.URL, followSymlinks bool
 		return f.walkDir(ctx, src, followSymlinks)
 	}
 
-	if src.HasGlob() {
-		return f.expandGlob(ctx, src, followSymlinks)
-	}
-
 	return f.listSingleObject(ctx, src)
 }
 


Test patch:
diff --git a/e2e/cp_test.go b/e2e/cp_test.go
index b59cf43..4aca7ed 100644
--- a/e2e/cp_test.go
+++ b/e2e/cp_test.go
@@ -1109,6 +1109,62 @@ func TestCopyMultipleFilesToS3WithPrefixWithoutSlash(t *testing.T) {
 	assert.Assert(t, fs.Equal(workdir.Path(), expected))
 }
 
+// cp prefix* s3://bucket/
+func TestCopyDirectoryWithGlobCharactersToS3Bucket(t *testing.T) {
+	t.Parallel()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("Files in Windows cannot contain glob(*) characters")
+	}
+
+	bucket := s3BucketFromTestName(t)
+
+	s3client, s5cmd, cleanup := setup(t)
+	defer cleanup()
+
+	createBucket(t, s3client, bucket)
+
+	folderLayout := []fs.PathOp{
+		fs.WithDir(
+			"abc*",
+			fs.WithFile("file1.txt", "this is the first test file"),
+			fs.WithFile("file2.txt", "this is the second test file"),
+		),
+		fs.WithDir(
+			"abcd",
+			fs.WithFile("file1.txt", "this is the first test file"),
+		),
+		fs.WithDir(
+			"abcde",
+			fs.WithFile("file1.txt", "this is the first test file"),
+			fs.WithFile("file2.txt", "this is the second test file"),
+		),
+	}
+
+	workdir := fs.NewDir(t, "somedir", folderLayout...)
+	defer workdir.Remove()
+
+	src := fmt.Sprintf("%v/abc*", workdir.Path())
+	dst := fmt.Sprintf("s3://%v", bucket)
+
+	cmd := s5cmd("cp", src, dst)
+	result := icmd.RunCmd(cmd)
+
+	result.Assert(t, icmd.Success)
+
+	assertLines(t, result.Stdout(), map[int]compareFunc{
+		0: equals(`cp %v/abc*/file1.txt %v/abc*/file1.txt`, workdir.Path(), dst),
+		1: equals(`cp %v/abc*/file2.txt %v/abc*/file2.txt`, workdir.Path(), dst),
+		2: equals(`cp %v/abcd/file1.txt %v/abcd/file1.txt`, workdir.Path(), dst),
+		3: equals(`cp %v/abcde/file1.txt %v/abcde/file1.txt`, workdir.Path(), dst),
+		4: equals(`cp %v/abcde/file2.txt %v/abcde/file2.txt`, workdir.Path(), dst),
+	}, sortInput(true))
+
+	// assert local filesystem
+	expected := fs.Expected(t, folderLayout...)
+	assert.Assert(t, fs.Equal(workdir.Path(), expected))
+}
+
 // cp dir/* s3://bucket/prefix/
 func TestCopyMultipleFilesToS3WithPrefixWithSlash(t *testing.T) {
 	t.Parallel()
diff --git a/e2e/rm_test.go b/e2e/rm_test.go
index acbd9d0..3075e6c 100644
--- a/e2e/rm_test.go
+++ b/e2e/rm_test.go
@@ -2,6 +2,7 @@ package e2e
 
 import (
 	"fmt"
+	"runtime"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -383,6 +384,62 @@ func TestRemoveLocalDirectory(t *testing.T) {
 	assert.Assert(t, fs.Equal(workdir.Path(), expected))
 }
 
+// rm prefix*
+func TestRemoveLocalDirectoryWithGlob(t *testing.T) {
+	t.Parallel()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("Files in Windows cannot contain glob(*) characters")
+	}
+
+	_, s5cmd, cleanup := setup(t)
+	defer cleanup()
+
+	folderLayout := []fs.PathOp{
+		fs.WithDir(
+			"abc*",
+			fs.WithFile("file1.txt", "this is the first test file"),
+			fs.WithFile("file2.txt", "this is the second test file"),
+		),
+		fs.WithDir(
+			"abcd",
+			fs.WithFile("file1.txt", "this is the first test file"),
+		),
+		fs.WithDir(
+			"abcde",
+			fs.WithFile("file1.txt", "this is the first test file"),
+			fs.WithFile("file2.txt", "this is the second test file"),
+		),
+	}
+
+	workdir := fs.NewDir(t, t.Name(), folderLayout...)
+	defer workdir.Remove()
+
+	cmd := s5cmd("rm", "abc*")
+	result := icmd.RunCmd(cmd, withWorkingDir(workdir))
+
+	result.Assert(t, icmd.Success)
+
+	assertLines(t, result.Stdout(), map[int]compareFunc{
+		0: equals("rm abc*/file1.txt"),
+		1: equals("rm abc*/file2.txt"),
+		2: equals("rm abcd/file1.txt"),
+		3: equals("rm abcde/file1.txt"),
+		4: equals("rm abcde/file2.txt"),
+	}, sortInput(true))
+
+	assertLines(t, result.Stderr(), map[int]compareFunc{})
+
+	// expected 3 empty dirs
+	expected := fs.Expected(
+		t,
+		fs.WithDir("abc*"),
+		fs.WithDir("abcd"),
+		fs.WithDir("abcde"),
+	)
+	assert.Assert(t, fs.Equal(workdir.Path(), expected))
+}
+
 // rm dir/ file file2
 func TestVariadicMultipleLocalFilesWithDirectory(t *testing.T) {
 	t.Parallel()