commit 0964277a37e285003d9874b83bac6725f953b773
parent 1ff1fabb315ffb6f12e77df5e689434161792911
Author: Dan Stillman <dstillman@zotero.org>
Date: Wed, 22 Feb 2017 04:56:49 -0500
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.
Diffstat:
3 files changed, 61 insertions(+), 23 deletions(-)
diff --git 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
@@ -497,12 +497,19 @@ 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)
*
* @param {Boolean} [options.allowExistingTarget=false] - If true, merge files into an existing
@@ -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
@@ -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* () {
