From 04335ef418eb58bced2ee518d66321b9bdfe6e00 Mon Sep 17 00:00:00 2001
From: Aurimas Vinckevicius <aurimas.dev@gmail.com>
Date: Sat, 23 Nov 2013 18:28:46 -0600
Subject: [PATCH] Properly revert attachment renames if they fail. Closes #430

---
 chrome/content/zotero/bindings/attachmentbox.xml | 16 +++++++++-------
 chrome/content/zotero/xpcom/data/item.js         | 14 ++++++++------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/chrome/content/zotero/bindings/attachmentbox.xml b/chrome/content/zotero/bindings/attachmentbox.xml
index 2c75f6affc..7e53b2517a 100644
--- a/chrome/content/zotero/bindings/attachmentbox.xml
+++ b/chrome/content/zotero/bindings/attachmentbox.xml
@@ -401,15 +401,17 @@
 									'',
 									newFilename + ' exists. Overwrite existing file?'
 								);
-								if (confirmed) {
-									item.renameAttachmentFile(newFilename, true);
-									break;
+								if (!confirmed) {
+									// If they said not to overwrite existing file,
+									// start again
+									continue;
 								}
-								// If they said not to overwrite existing file,
-								// start again
-								continue;
+								
+								// Force overwrite, but make sure we check that this doesn't fail
+								renamed = item.renameAttachmentFile(newFilename, true);
 							}
-							else if (renamed == -2) {
+							
+							if (renamed == -2) {
 								nsIPS.alert(
 									window,
 									Zotero.getString('general.error'),
diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 2a7df206d2..bb7090e0da 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -3009,6 +3009,7 @@ Zotero.Item.prototype.renameAttachmentFile = function(newName, overwrite) {
 		return false;
 	}
 	
+	var origModDate = file.lastModifiedTime;
 	try {
 		newName = Zotero.File.getValidFileName(newName);
 		
@@ -3027,18 +3028,17 @@ Zotero.Item.prototype.renameAttachmentFile = function(newName, overwrite) {
 		// files, since dest.exists() will just show true on a case-insensitive
 		// filesystem anyway.
 		if (file.leafName.toLowerCase() != dest.leafName.toLowerCase()) {
-			if (overwrite) {
-				dest.remove(false);
-			}
-			else if (dest.exists()) {
+			if (!overwrite && dest.exists()) {
 				return -1;
 			}
 		}
 		
-		file.moveTo(null, newName);
 		// Update mod time and clear hash so the file syncs
 		// TODO: use an integer counter instead of mod time for change detection
-		dest.lastModifiedTime = new Date();
+		// Update mod time first, because it may fail for read-only files on Windows
+		file.lastModifiedTime = new Date();
+		file.moveTo(null, newName);
+		
 		this.relinkAttachmentFile(dest);
 		
 		Zotero.DB.beginTransaction();
@@ -3051,6 +3051,8 @@ Zotero.Item.prototype.renameAttachmentFile = function(newName, overwrite) {
 		return true;
 	}
 	catch (e) {
+		// Restore original modification date in case we managed to change it
+		try { file.lastModifiedTime = origModDate } catch (e) {}
 		Zotero.debug(e);
 		Components.utils.reportError(e);
 		return -2;