From 0964277a37e285003d9874b83bac6725f953b773 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 22 Feb 2017 04:56:49 -0500 Subject: [PATCH] Use OS.File.move() for data-dir migration on Windows, and make automatic Previously on Windows, where we don't have /bin/mv, we were recursing into the data directory and copying files individually, which is very slow, so automatic migration was disabled. Instead, try moving directories with OS.File.move() with the `noCopy` flag. Moving directories is technically unsupported by OS.File, but probably only because of the possibility of a cross-volume copy (which is only implemented for some platforms), and using `noCopy` hopefully prevents that. If someone does have their data directory or storage directory on a different volume, the migration might be quite slow, but leaving a data directory behind in the Firefox profile directory (where it can be easily misplaced with a seemingly unrelated Firefox reset) is worse. --- chrome/content/zotero/xpcom/dataDirectory.js | 9 +---- chrome/content/zotero/xpcom/file.js | 37 +++++++++++++++++-- test/tests/dataDirectoryTest.js | 38 +++++++++++++------- 3 files changed, 61 insertions(+), 23 deletions(-) diff --git a/chrome/content/zotero/xpcom/dataDirectory.js b/chrome/content/zotero/xpcom/dataDirectory.js index 07f7176d0e..82a38c6540 100644 --- a/chrome/content/zotero/xpcom/dataDirectory.js +++ b/chrome/content/zotero/xpcom/dataDirectory.js @@ -484,14 +484,7 @@ Zotero.DataDirectory = { } let automatic = false; if (!exists) { - // Migrate automatically on macOS and Linux -- this should match the check in - // Zotero.File.moveDirectory() - if (!Zotero.isWin && (yield OS.File.exists("/bin/mv"))) { - automatic = true; - } - else { - return false; - } + automatic = true; // 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))) { diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js index 2e54259ff3..237b1181b8 100644 --- a/chrome/content/zotero/xpcom/file.js +++ b/chrome/content/zotero/xpcom/file.js @@ -497,11 +497,18 @@ Zotero.File = new function(){ * * Currently this means using /bin/mv, which only works on macOS and Linux */ - this.canMoveDirectoryAtomic = Zotero.lazy(function () { + this.canMoveDirectoryWithCommand = Zotero.lazy(function () { var cmd = "/bin/mv"; return !Zotero.isWin && this.pathToFile(cmd).exists(); }); + /** + * For tests + */ + this.canMoveDirectoryWithFunction = Zotero.lazy(function () { + return true; + }); + /** * Move directory (using mv on macOS/Linux, recursively on Windows) * @@ -513,7 +520,8 @@ Zotero.File = new function(){ this.moveDirectory = Zotero.Promise.coroutine(function* (oldDir, newDir, options = {}) { var maxDepth = options.maxDepth || 10; var cmd = "/bin/mv"; - var useCmd = this.canMoveDirectoryAtomic(); + var useCmd = this.canMoveDirectoryWithCommand(); + var useFunction = this.canMoveDirectoryWithFunction(); if (!options.allowExistingTarget && (yield OS.File.exists(newDir))) { throw new Error(newDir + " exists"); @@ -591,6 +599,7 @@ Zotero.File = new function(){ let moved = false; if (useCmd && !(yield OS.File.exists(dest))) { + Zotero.debug(`Moving ${entry.path} with ${cmd}`); let args = [entry.path, dest]; try { yield Zotero.Utilities.Internal.exec(cmd, args); @@ -598,7 +607,29 @@ Zotero.File = new function(){ } catch (e) { checkError(e); - addError(e); + Zotero.debug(e, 1); + } + } + + + // If can't use command, try moving with OS.File.move(). Technically this is + // unsupported for directories, but it works on all platforms as long as noCopy + // is set (and on some platforms regardless) + if (!moved && useFunction) { + Zotero.debug(`Moving ${entry.path} with OS.File`); + try { + yield OS.File.move( + entry.path, + dest, + { + noCopy: true + } + ); + moved = true; + } + catch (e) { + checkError(e); + Zotero.debug(e, 1); } } diff --git a/test/tests/dataDirectoryTest.js b/test/tests/dataDirectoryTest.js index 6d0cd77600..e0604292d9 100644 --- a/test/tests/dataDirectoryTest.js +++ b/test/tests/dataDirectoryTest.js @@ -59,19 +59,33 @@ describe("Zotero.DataDirectory", function () { stubs.pipeExists.restore(); }); + // Force non-mv mode var disableCommandMode = function () { - // Force non-mv mode - var origFunc = OS.File.exists; - if (!stubs.canMoveDirectoryAtomic) { - stubs.canMoveDirectoryAtomic = sinon.stub(Zotero.File, "canMoveDirectoryAtomic") + if (!stubs.canMoveDirectoryWithCommand) { + stubs.canMoveDirectoryWithCommand = sinon.stub(Zotero.File, "canMoveDirectoryWithCommand") + .returns(false); + } + }; + + // Force non-OS.File.move() mode + var disableFunctionMode = function () { + if (!stubs.canMoveDirectoryWithFunction) { + stubs.canMoveDirectoryWithFunction = sinon.stub(Zotero.File, "canMoveDirectoryWithFunction") .returns(false); } }; var resetCommandMode = function () { - if (stubs.canMoveDirectoryAtomic) { - stubs.canMoveDirectoryAtomic.restore(); - stubs.canMoveDirectoryAtomic = undefined; + if (stubs.canMoveDirectoryWithCommand) { + stubs.canMoveDirectoryWithCommand.restore(); + stubs.canMoveDirectoryWithCommand = undefined; + } + }; + + var resetFunctionMode = function () { + if (stubs.canMoveDirectoryWithFunction) { + stubs.canMoveDirectoryWithFunction.restore(); + stubs.canMoveDirectoryWithFunction = undefined; } }; @@ -144,10 +158,12 @@ describe("Zotero.DataDirectory", function () { beforeEach(function () { disableCommandMode(); + disableFunctionMode(); }); after(function () { resetCommandMode(); + resetFunctionMode(); }); var tests = []; @@ -157,11 +173,7 @@ describe("Zotero.DataDirectory", function () { 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(); - } + resetFunctionMode(); yield populateDataDirectory(oldDir); yield OS.File.remove(oldMigrationMarker); @@ -347,10 +359,12 @@ describe("Zotero.DataDirectory", function () { before(function () { disableCommandMode(); + disableFunctionMode(); }); after(function () { resetCommandMode(); + resetFunctionMode(); }); it("should handle partial failure", function* () {