From 244452224331d74439eb7cd3a51bd9f21f7e8498 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Mon, 4 Sep 2017 21:39:21 +0200 Subject: [PATCH 1/3] Add test for colliding names --- internal/archiver/archiver_test.go | 54 ++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 2961d403c..aaaf02a22 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -4,6 +4,9 @@ import ( "bytes" "context" "io" + "io/ioutil" + "os" + "path/filepath" "testing" "time" @@ -312,3 +315,54 @@ func TestArchiveEmptySnapshot(t *testing.T) { t.Errorf("expected null snapshot for empty snapshot, got %v", sn) } } + +func chdir(t testing.TB, target string) (cleanup func()) { + curdir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + t.Logf("chdir to %v", target) + err = os.Chdir(target) + if err != nil { + t.Fatal(err) + } + + return func() { + t.Logf("chdir back to %v", curdir) + err := os.Chdir(curdir) + if err != nil { + t.Fatal(err) + } + } +} + +func TestArchiveNameCollision(t *testing.T) { + repo, cleanup := repository.TestRepository(t) + defer cleanup() + + dir, cleanup := TempDir(t) + defer cleanup() + + root := filepath.Join(dir, "root") + OK(t, os.MkdirAll(root, 0755)) + + OK(t, ioutil.WriteFile(filepath.Join(dir, "testfile"), []byte("testfile1"), 0644)) + OK(t, ioutil.WriteFile(filepath.Join(dir, "root", "testfile"), []byte("testfile2"), 0644)) + + defer chdir(t, root)() + + arch := archiver.New(repo) + + sn, id, err := arch.Snapshot(context.TODO(), nil, []string{"testfile", filepath.Join("..", "testfile")}, nil, "localhost", nil) + OK(t, err) + + t.Logf("snapshot archived as %v", id) + + tree, err := repo.LoadTree(context.TODO(), *sn.Tree) + OK(t, err) + + if len(tree.Nodes) != 2 { + t.Fatalf("tree has %d nodes, wanted 2: %v", len(tree.Nodes), tree.Nodes) + } +} From 83eb075e3a955f2b2d0faea694779583549ac051 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Mon, 4 Sep 2017 21:13:20 +0200 Subject: [PATCH 2/3] Resolve name collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment when two items to be saved have the same directory name, restic only saves the first one to the repo. Let's say we have a structure like this: dir1 └── subdir └── file dir2 └── subdir └── file When restic is run on `dir1/subdir` and `dir2/subdir`, it will only save the first `subdir`: $ restic backup dir1/subdir dir2/subdir [...] $ restic ls -l latest drwxr-xr-x 1000 100 0 2017-08-27 20:56:39 /subdir -rw-r--r-- 1000 100 17 2017-08-27 20:56:39 /subdir/file That's obviously a bad thing, caused by an early decision to strip the full path to the files/dirs to save and only leave the last directory. This commit partly resolves this by handling colliding names and resolving the conflicts. Restic will now append a counter to the file (`-123`) until the conflict is resolved. So in the example above, we'll end up with the following structure: $ restic ls -l latest drwxr-xr-x 1000 100 0 2017-08-27 20:56:39 /subdir -rw-r--r-- 1000 100 17 2017-08-27 20:56:39 /subdir/file drwxr-xr-x 1000 100 0 2017-08-27 20:56:46 /subdir-1 -rw-r--r-- 1000 100 17 2017-08-27 20:56:46 /subdir-1/file This partly addresses #549 and closes #1179. At first I thought that the obvious correction would be to archive the full path. But it turns out that collisions may still occur: Suppose you have a file named `foo` in the current directory, and the parent directory also contains a file `foo`. Archiving these with restic also causes a collision, since restic strips the `../` from the first file: $ restic backup ../foo foo This also happens with `tar`, which does not handle the collision and will happily archive two files called `foo`. So, the best way forward is to handle name collisions and archive the whole path. The latter will be tackled in a separate PR. --- internal/archiver/archiver.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index dd7bea06f..3117ec509 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -383,7 +383,22 @@ func (arch *Archiver) dirWorker(ctx context.Context, wg *sync.WaitGroup, p *rest panic("invalid null subtree restic.ID") } } - tree.Insert(node) + + // insert node into tree, resolve name collisions + name := node.Name + i := 0 + for { + i++ + err := tree.Insert(node) + if err == nil { + break + } + + newName := fmt.Sprintf("%v-%d", name, i) + fmt.Fprintf(os.Stderr, "%v: name collision for %q, renaming to %q\n", filepath.Dir(node.Path), node.Name, newName) + node.Name = newName + } + } node := &restic.Node{} From c935d0558cd867e2fcf38e1d22174b2cdf19bc17 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Tue, 5 Sep 2017 21:48:13 +0200 Subject: [PATCH 3/3] Add entry to CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd5be92a6..3865f4b98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ Important Changes in 0.X.Y included) in a restore, they are not loaded from the repo any more. https://github.com/restic/restic/pull/1044 + * Name collisions are now resolved by appending a counter. + https://github.com/restic/restic/issues/1179 + https://github.com/restic/restic/pull/1209 + Small changes -------------