From c281acbc4a2b7fad9475ec5084f9ff14dff2c5c8 Mon Sep 17 00:00:00 2001 From: Terry Rankine Date: Thu, 17 Sep 2026 21:04:03 +0200 Subject: [PATCH] fix: sync --delete respects --exclude and --include Objects that exist only in the destination were deleted even when they matched an --exclude pattern or did not match an --include pattern. The generated rm command did receive the patterns, but rm runs in raw mode and matched them against the full object key, so a pattern like "folder/*" never matched under a destination prefix. sync now filters the delete candidates itself, relative to the destination prefix, and omits --exclude/--include from the generated rm command. generateCommand treats a nil default flag value as "omit this flag". Fixes #815 Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 3 + command/context.go | 5 ++ command/context_test.go | 24 +++++ command/sync.go | 41 ++++++++- e2e/sync_test.go | 194 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 266 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index b6854e0d2..46546616e 100644 --- a/README.md +++ b/README.md @@ -443,6 +443,9 @@ cp styles.css s3://bucket/static/styles.css cp readme.md s3://bucket/static/readme.md ``` +`--exclude` and `--include` also apply to `--delete`: destination objects that +match an exclude pattern, or that match none of the include patterns, are kept. + It's also possible to use wildcards to sync only a subset of files. To sync only `.html` files in S3 bucket above to same local file system; diff --git a/command/context.go b/command/context.go index 1eb78fb3d..554b8873f 100644 --- a/command/context.go +++ b/command/context.go @@ -62,6 +62,8 @@ func contextValue(c *cli.Context, flagname string) []string { } // generateCommand generates command string from given context, app command, default flags and urls. +// A default flag with a nil value is omitted from the generated command, even +// if it is set in the given context. func generateCommand(c *cli.Context, cmd string, defaultFlags map[string]interface{}, urls ...*url.URL) (string, error) { command := AppCommand(cmd) flagset := flag.NewFlagSet(command.Name, flag.ContinueOnError) @@ -73,6 +75,9 @@ func generateCommand(c *cli.Context, cmd string, defaultFlags map[string]interfa flags := []string{} for flagname, flagvalue := range defaultFlags { + if flagvalue == nil { + continue + } flags = append(flags, fmt.Sprintf("--%s='%v'", flagname, flagvalue)) } diff --git a/command/context_test.go b/command/context_test.go index e7783356f..fe2ca4745 100644 --- a/command/context_test.go +++ b/command/context_test.go @@ -135,6 +135,30 @@ func TestGenerateCommand(t *testing.T) { }, expectedCommand: `cp --exclude='*.log' --exclude='*.txt' "/source/dir" "s3://bucket/prefix/"`, }, + { + name: "nil-default-flag-should-be-omitted-even-if-set-in-context", + cmd: "rm", + flags: []cli.Flag{ + &cli.StringSliceFlag{ + Name: "exclude", + Value: cli.NewStringSlice("*.txt"), + }, + &cli.StringSliceFlag{ + Name: "include", + Value: cli.NewStringSlice("*.log"), + }, + }, + defaultFlags: map[string]interface{}{ + "raw": true, + "exclude": nil, + "include": nil, + }, + urls: []*url.URL{ + mustNewURL(t, "s3://bucket/key1"), + mustNewURL(t, "s3://bucket/key2"), + }, + expectedCommand: `rm --raw='true' "s3://bucket/key1" "s3://bucket/key2"`, + }, { name: "command-with-multiple-args", cmd: "rm", diff --git a/command/sync.go b/command/sync.go index 6dede5a66..943083acc 100644 --- a/command/sync.go +++ b/command/sync.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "sync" @@ -131,6 +132,12 @@ type Sync struct { delete bool sizeOnly bool exitOnError bool + exclude []string + include []string + + // patterns + excludePatterns []*regexp.Regexp + includePatterns []*regexp.Regexp // s3 options storageOpts storage.Options @@ -155,6 +162,8 @@ func NewSync(c *cli.Context) Sync { delete: c.Bool("delete"), sizeOnly: c.Bool("size-only"), exitOnError: c.Bool("exit-on-error"), + exclude: c.StringSlice("exclude"), + include: c.StringSlice("include"), // flags followSymlinks: !c.Bool("no-follow-symlinks"), @@ -180,6 +189,18 @@ func (s Sync) Run(c *cli.Context) error { return err } + s.excludePatterns, err = createRegexFromWildcard(s.exclude) + if err != nil { + printError(s.fullCommand, s.op, err) + return err + } + + s.includePatterns, err = createRegexFromWildcard(s.include) + if err != nil { + printError(s.fullCommand, s.op, err) + return err + } + ctx, cancel := context.WithCancel(c.Context) sourceObjects, destObjects, err := s.getSourceAndDestinationObjects(ctx, cancel, srcurl, dsturl) @@ -505,6 +526,15 @@ func (s Sync) planRun( dstURLs := make([]*url.URL, 0, extsortChunkSize) for d := range onlyDest { + // objects filtered out by --exclude/--include are not part + // of the sync, so they must not be deleted from the + // destination either. + if len(s.excludePatterns) > 0 && isURLMatched(s.excludePatterns, d.Path, dsturl.Prefix) { + continue + } + if len(s.includePatterns) > 0 && !isURLMatched(s.includePatterns, d.Path, dsturl.Prefix) { + continue + } dstURLs = append(dstURLs, d) } @@ -512,7 +542,16 @@ func (s Sync) planRun( return } - command, err := generateCommand(c, "rm", defaultFlags, dstURLs...) + // --exclude and --include are already applied above, relative to + // the destination prefix. Omit them from the generated rm command, + // which would match them against the full object key instead. + rmFlags := map[string]interface{}{ + "raw": true, + "exclude": nil, + "include": nil, + } + + command, err := generateCommand(c, "rm", rmFlags, dstURLs...) if err != nil { printDebug(s.op, err, dstURLs...) return diff --git a/e2e/sync_test.go b/e2e/sync_test.go index d592e6d52..0dfd65666 100644 --- a/e2e/sync_test.go +++ b/e2e/sync_test.go @@ -2439,6 +2439,200 @@ func TestSyncLocalDirectoryToS3WithExcludeFilter(t *testing.T) { } } +// sync --delete --exclude "sub/*" folder/ s3://bucket/prefix/ +func TestSyncLocalToS3BucketWithDeleteAndExcludeFilter(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + folderLayout := []fs.PathOp{ + fs.WithFile("readme.md", "S: this is a readme file"), + } + + workdir := fs.NewDir(t, "somedir", folderLayout...) + defer workdir.Remove() + + s3Content := map[string]string{ + "prefix/sub/keep.txt": "D: this is a text file", + "prefix/old.log": "D: this is a log file", + } + + for filename, content := range s3Content { + putFile(t, s3client, bucket, filename, content) + } + + // pattern is relative to the destination prefix. + const excludePattern = "sub/*" + + src := fmt.Sprintf("%v/", workdir.Path()) + src = filepath.ToSlash(src) + dst := fmt.Sprintf("s3://%v/prefix/", bucket) + + cmd := s5cmd("sync", "--delete", "--exclude", excludePattern, src, dst) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(`cp %vreadme.md %vreadme.md`, src, dst), + 1: equals(`rm %vold.log`, dst), + }, sortInput(true)) + + // assert local filesystem + expected := fs.Expected(t, folderLayout...) + assert.Assert(t, fs.Equal(workdir.Path(), expected)) + + expectedS3Content := map[string]string{ + "prefix/readme.md": "S: this is a readme file", + // excluded object exists only in destination and must not be deleted. + "prefix/sub/keep.txt": "D: this is a text file", + } + + nonExpectedS3Content := map[string]string{ + "prefix/old.log": "D: this is a log file", + } + + // assert objects should be in S3 + for key, content := range expectedS3Content { + assert.Assert(t, ensureS3Object(s3client, bucket, key, content)) + } + + // assert objects should not be in S3. + for key, content := range nonExpectedS3Content { + err := ensureS3Object(s3client, bucket, key, content) + assertError(t, err, errS3NoSuchKey) + } +} + +// sync --delete --exclude "sub/*" s3://bucket/* folder/ +func TestSyncS3BucketToLocalWithDeleteAndExcludeFilter(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + s3Content := map[string]string{ + "readme.md": "S: this is a readme file", + } + + for filename, content := range s3Content { + putFile(t, s3client, bucket, filename, content) + } + + folderLayout := []fs.PathOp{ + fs.WithFile("old.log", "D: this is a log file"), + fs.WithDir("sub", + fs.WithFile("keep.txt", "D: this is a text file"), + ), + } + + workdir := fs.NewDir(t, "somedir", folderLayout...) + defer workdir.Remove() + + // pattern is relative to the destination directory. + const excludePattern = "sub/*" + + src := fmt.Sprintf("s3://%v/", bucket) + dst := fmt.Sprintf("%v/", workdir.Path()) + dst = filepath.ToSlash(dst) + + cmd := s5cmd("sync", "--delete", "--exclude", excludePattern, src+"*", dst) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(`cp %vreadme.md %vreadme.md`, src, dst), + 1: equals(`rm %vold.log`, dst), + }, sortInput(true)) + + expectedFolderLayout := []fs.PathOp{ + fs.WithFile("readme.md", "S: this is a readme file"), + // excluded file exists only in destination and must not be deleted. + fs.WithDir("sub", + fs.WithFile("keep.txt", "D: this is a text file"), + ), + } + + expected := fs.Expected(t, expectedFolderLayout...) + assert.Assert(t, fs.Equal(workdir.Path(), expected)) + + for key, content := range s3Content { + assert.Assert(t, ensureS3Object(s3client, bucket, key, content)) + } +} + +// sync --delete --include "*.md" --include "sub/*" folder/ s3://bucket/prefix/ +func TestSyncLocalToS3BucketWithDeleteAndIncludeFilter(t *testing.T) { + t.Parallel() + + s3client, s5cmd := setup(t) + + bucket := s3BucketFromTestName(t) + createBucket(t, s3client, bucket) + + folderLayout := []fs.PathOp{ + fs.WithFile("readme.md", "S: this is a readme file"), + } + + workdir := fs.NewDir(t, "somedir", folderLayout...) + defer workdir.Remove() + + s3Content := map[string]string{ + "prefix/keep.txt": "D: this is a text file", + "prefix/sub/old.log": "D: this is a log file", + } + + for filename, content := range s3Content { + putFile(t, s3client, bucket, filename, content) + } + + src := fmt.Sprintf("%v/", workdir.Path()) + src = filepath.ToSlash(src) + dst := fmt.Sprintf("s3://%v/prefix/", bucket) + + // patterns are relative to the destination prefix. + cmd := s5cmd("sync", "--delete", "--include", "*.md", "--include", "sub/*", src, dst) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(`cp %vreadme.md %vreadme.md`, src, dst), + 1: equals(`rm %vsub/old.log`, dst), + }, sortInput(true)) + + // assert local filesystem + expected := fs.Expected(t, folderLayout...) + assert.Assert(t, fs.Equal(workdir.Path(), expected)) + + expectedS3Content := map[string]string{ + "prefix/readme.md": "S: this is a readme file", + // object not matching --include exists only in destination and must not be deleted. + "prefix/keep.txt": "D: this is a text file", + } + + nonExpectedS3Content := map[string]string{ + "prefix/sub/old.log": "D: this is a log file", + } + + // assert objects should be in S3 + for key, content := range expectedS3Content { + assert.Assert(t, ensureS3Object(s3client, bucket, key, content)) + } + + // assert objects should not be in S3. + for key, content := range nonExpectedS3Content { + err := ensureS3Object(s3client, bucket, key, content) + assertError(t, err, errS3NoSuchKey) + } +} + // sync --delete somedir s3://bucket/ (removes 10k objects) func TestIssue435(t *testing.T) { t.Parallel()