From 1b67ed071e80f1091fa77ec910f098495fbf6a40 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 30 Nov 2016 01:50:49 -0500 Subject: [PATCH] Skip auto data dir migration if target dir exists and is non-empty --- chrome/content/zotero/xpcom/dataDirectory.js | 9 +++++ chrome/content/zotero/xpcom/file.js | 13 +++++++- test/tests/dataDirectoryTest.js | 35 ++++++++++++++------ 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/chrome/content/zotero/xpcom/dataDirectory.js b/chrome/content/zotero/xpcom/dataDirectory.js index 6545910c8b..8860663c99 100644 --- a/chrome/content/zotero/xpcom/dataDirectory.js +++ b/chrome/content/zotero/xpcom/dataDirectory.js @@ -428,6 +428,9 @@ Zotero.DataDirectory = { }, + /** + * Determine if current data directory is in a legacy location + */ canMigrate: function () { // If (not default location) && (not useDataDir or within legacy location) var currentDir = this.dir; @@ -489,6 +492,12 @@ Zotero.DataDirectory = { else { return false; } + + // Skip automatic migration if there's a non-empty directory at the new location + if ((yield OS.File.exists(newDir)) && !(yield Zotero.File.directoryIsEmpty(newDir))) { + Zotero.debug(`${newDir} exists and is non-empty -- skipping migration`); + return false; + } } // Check for an existing pipe from other running versions of Zotero pointing at the same data diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js index c4e5b08758..27245a148a 100644 --- a/chrome/content/zotero/xpcom/file.js +++ b/chrome/content/zotero/xpcom/file.js @@ -486,6 +486,17 @@ Zotero.File = new function(){ }); } + + /** + * If directories can be moved at once, instead of recursively creating directories and moving files + * + * Currently this means using /bin/mv, which only works on macOS and Linux + */ + this.canMoveDirectoryAtomic = Zotero.lazy(function () { + var cmd = "/bin/mv"; + return !Zotero.isWin && this.pathToFile(cmd).exists(); + }); + /** * Move directory (using mv on macOS/Linux, recursively on Windows) * @@ -497,7 +508,7 @@ Zotero.File = new function(){ this.moveDirectory = Zotero.Promise.coroutine(function* (oldDir, newDir, options = {}) { var maxDepth = options.maxDepth || 10; var cmd = "/bin/mv"; - var useCmd = !Zotero.isWin && (yield OS.File.exists(cmd)); + var useCmd = this.canMoveDirectoryAtomic(); if (!options.allowExistingTarget && (yield OS.File.exists(newDir))) { throw new Error(newDir + " exists"); diff --git a/test/tests/dataDirectoryTest.js b/test/tests/dataDirectoryTest.js index 638ad790c0..882a9eca8c 100644 --- a/test/tests/dataDirectoryTest.js +++ b/test/tests/dataDirectoryTest.js @@ -62,18 +62,17 @@ describe("Zotero.DataDirectory", function () { var disableCommandMode = function () { // Force non-mv mode var origFunc = OS.File.exists; - stubs.fileExists = sinon.stub(OS.File, "exists", function (path) { - if (path == '/bin/mv') { - return Zotero.Promise.resolve(false); - } - else { - return origFunc(path); - } - }); + if (!stubs.canMoveDirectoryAtomic) { + stubs.canMoveDirectoryAtomic = sinon.stub(Zotero.File, "canMoveDirectoryAtomic") + .returns(false); + } }; var resetCommandMode = function () { - stubs.fileExists.restore(); + if (stubs.canMoveDirectoryAtomic) { + stubs.canMoveDirectoryAtomic.restore(); + stubs.canMoveDirectoryAtomic = undefined; + } }; var populateDataDirectory = Zotero.Promise.coroutine(function* (dir, srcDir, automatic = false) { @@ -143,7 +142,7 @@ describe("Zotero.DataDirectory", function () { describe("#checkForMigration()", function () { let fileMoveStub; - before(function () { + beforeEach(function () { disableCommandMode(); }); @@ -156,6 +155,22 @@ describe("Zotero.DataDirectory", function () { tests.push([desc, fn]); } + it("should skip automatic migration if target directory exists and is non-empty", function* () { + resetCommandMode(); + + // No automatic migration without atomic directory move + if (!Zotero.File.canMoveDirectoryAtomic()) { + this.skip(); + } + + yield populateDataDirectory(oldDir); + yield OS.File.remove(oldMigrationMarker); + yield OS.File.makeDir(newDir, { unixMode: 0o755 }); + yield Zotero.File.putContentsAsync(OS.Path.join(newDir, 'a'), ''); + + yield assert.eventually.isFalse(Zotero.DataDirectory.checkForMigration(oldDir, newDir)); + }); + add("should show error on partial failure", function (automatic) { return function* () { yield populateDataDirectory(oldDir, null, automatic);