improve directory deletion/editing handling

pass/de.lproj/Localizable.strings

@@ -72,6 +72,7 @@
 "KeyImportError."                       = "Schlüssel kann nicht importiert werden.";
 "FileNotFoundError."                    = "Die Datei '%@' kann nicht gelesen werden.";
 "PasswordDuplicatedError."              = "Passwort kann nicht hinzugefügt werden; es existiert bereits.";
+"CannotDeleteNonEmptyDirectoryError."   = "Ordner muss erst leer sein um gelöscht werden zu können.";
 "GitResetError."                        = "Der zuletzt synchronisierte Commit kann nicht identifiziert werden.";
 "GitCreateSignatureError."              = "Es konnte keine valide Signatur für den Author/Committer angelegt werden.";
 "GitPushNotSuccessfulError."            = "Die Übertragung der lokalen Änderungen war nicht erfolgreich. Stelle bitte sicher, dass auf dem Remote-Repository alle Änderungen commitet sind.";

pass/en.lproj/Localizable.strings

@@ -73,7 +73,7 @@
 "KeyImportError."                       = "Cannot import the key.";
 "FileNotFoundError."                    = "File '%@' cannot be read.";
 "PasswordDuplicatedError."              = "Cannot add the password; password is duplicated.";
-"CannotDeleteDirectoryError."           = "Cannot delete directories; delete passwords instead.";
+"CannotDeleteNonEmptyDirectoryError."   = "Delete passwords from the directory before deleting the directory itself.";
 "GitResetError."                        = "Cannot identify the latest synced commit.";
 "GitCreateSignatureError."              = "Cannot create a valid author/committer signature.";
 "GitPushNotSuccessfulError."            = "Pushing local changes was not successful. Make sure there are no uncommitted changes on the remote repository.";

passKit/Helpers/AppError.swift

@@ -15,7 +15,7 @@ public enum AppError: Error, Equatable {
     case keyImport
     case readingFile(fileName: String)
     case passwordDuplicated
-    case cannotDeleteDirectory
+    case cannotDeleteNonEmptyDirectory
     case gitReset
     case gitCommit
     case gitCreateSignature

passKit/Models/PasswordStore.swift

@@ -273,13 +273,15 @@ public class PasswordStore {
     }
 
     public func delete(passwordEntity: PasswordEntity) throws {
-        if passwordEntity.isDir {
+        if !passwordEntity.children.isEmpty {
-            throw AppError.cannotDeleteDirectory
+            throw AppError.cannotDeleteNonEmptyDirectory
         }
 
         let deletedFileURL = passwordEntity.fileURL(in: storeURL)
         let deletedFilePath = passwordEntity.path
+        if !passwordEntity.isDir {
             try gitRm(path: passwordEntity.path)
+        }
         try deletePasswordEntities(passwordEntity: passwordEntity)
         try deleteDirectoryTree(at: deletedFileURL)
         try gitCommit(message: "RemovePassword.".localize(deletedFilePath))
@@ -287,6 +289,11 @@ public class PasswordStore {
     }
 
     public func edit(passwordEntity: PasswordEntity, password: Password, keyID: String? = nil) throws -> PasswordEntity? {
+        guard !passwordEntity.isDir else {
+            // caller should ensure this, so this is not a user-facing error
+            throw AppError.other(message: "Cannot edit a directory")
+        }
+
         var newPasswordEntity: PasswordEntity? = passwordEntity
         let url = passwordEntity.fileURL(in: storeURL)
 

passKitTests/Fixtures/password-store-empty-dirs.git/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/main

passKitTests/Fixtures/password-store-empty-dirs.git/config

@@ -0,0 +1,6 @@
+[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = true
+	ignorecase = true
+	precomposeunicode = true

passKitTests/Fixtures/password-store-empty-dirs.git/description

@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.

passKitTests/Fixtures/password-store-empty-dirs.git/info/exclude

@@ -0,0 +1,6 @@
+# git ls-files --others --exclude-from=.git/info/exclude
+# Lines that start with '#' are comments.
+# For a project mostly in C, the following would be a good set of
+# exclude patterns (uncomment them if you want to use them):
+# *.[oa]
+# *~

passKitTests/Fixtures/password-store-empty-dirs.git/packed-refs

@@ -0,0 +1,2 @@
+# pack-refs with: peeled fully-peeled sorted 
+fbcbb5819e1c864ef33cfffa179a71387a5d90d0 refs/heads/main

passKitTests/Models/PasswordStoreTest.swift

@@ -176,7 +176,40 @@ final class PasswordStoreTest: XCTestCase {
         waitForExpectations(timeout: 1, handler: nil)
     }
 
-    func testDeleteDirectoryFails() throws {
+    func testDeletePasswordKeepsFileSystemFolderIfNotEmpty() throws {
+        try cloneRepository(.withGPGID)
+
+        // /work contains .gpg-id in addition to a password file
+        let entity = passwordStore.fetchPasswordEntity(with: "work/github.com.gpg")
+        try passwordStore.delete(passwordEntity: entity!)
+
+        XCTAssertFalse(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent("work/github.com.gpg").path))
+        XCTAssertNil(passwordStore.fetchPasswordEntity(with: "work/github.com.gpg"))
+        XCTAssertNil(passwordStore.fetchPasswordEntity(with: "work"))
+        XCTAssertTrue(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent("work/.gpg-id").path))
+    }
+
+    func testDeleteEmptyDirectory() throws {
+        try cloneRepository(.emptyDirs)
+        let numCommitsBefore = passwordStore.numberOfCommits!
+        let numLocalCommitsBefore = passwordStore.numberOfLocalCommits
+
+        expectation(forNotification: .passwordStoreUpdated, object: nil)
+
+        // Note: the directory isn't truely empty since Git doesn't track empty directories,
+        // but it should be treated as empty by the app since it contains only hidden files
+        let entityToDelete = passwordStore.fetchPasswordEntity(with: "empty-dir")
+        XCTAssertNotNil(entityToDelete)
+        try passwordStore.delete(passwordEntity: entityToDelete!)
+
+        XCTAssertNil(passwordStore.fetchPasswordEntity(with: "empty-dir"))
+        XCTAssertTrue(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent("empty-dir/.gitkeep").path))
+        XCTAssertEqual(passwordStore.numberOfCommits!, numCommitsBefore + 1)
+        XCTAssertEqual(passwordStore.numberOfLocalCommits, numLocalCommitsBefore + 1)
+        waitForExpectations(timeout: 1, handler: nil)
+    }
+
+    func testDeleteNonEmptyDirectoryFails() throws {
         try cloneRepository(.withGPGID)
         let numCommitsBefore = passwordStore.numberOfCommits!
         let numLocalCommitsBefore = passwordStore.numberOfLocalCommits
@@ -186,7 +219,7 @@ final class PasswordStoreTest: XCTestCase {
         let entity = passwordStore.fetchPasswordEntity(with: "personal")
         XCTAssertThrowsError(try passwordStore.delete(passwordEntity: entity!)) { error in
             XCTAssertTrue(error is AppError, "Unexpected error type: \(type(of: error))")
-            XCTAssertEqual(error as? AppError, .cannotDeleteDirectory)
+            XCTAssertEqual(error as? AppError, .cannotDeleteNonEmptyDirectory)
         }
 
         XCTAssertNotNil(passwordStore.fetchPasswordEntity(with: "personal/github.com.gpg"))
@@ -240,6 +273,23 @@ final class PasswordStoreTest: XCTestCase {
         waitForExpectations(timeout: 1, handler: nil)
     }
 
+    func testEditDirectoryFails() throws {
+        try cloneRepository(.withGPGID)
+        try importSinglePGPKey()
+        let numCommitsBefore = passwordStore.numberOfCommits!
+
+        let directoryEntity = passwordStore.fetchPasswordEntity(with: "personal")!
+        let editedPassword = Password(name: "new name", path: "new name", plainText: "")
+        editedPassword.changed = PasswordChange.path.rawValue
+        XCTAssertThrowsError(try passwordStore.edit(passwordEntity: directoryEntity, password: editedPassword)) { error in
+            XCTAssertTrue(error is AppError, "Unexpected error type: \(type(of: error))")
+            XCTAssertEqual(error as? AppError, .other(message: "Cannot edit a directory"))
+        }
+
+        XCTAssertNotNil(passwordStore.fetchPasswordEntity(with: "personal"))
+        XCTAssertEqual(passwordStore.numberOfCommits!, numCommitsBefore)
+    }
+
     func testReset() throws {
         try cloneRepository(.withGPGID)
         try importSinglePGPKey()
@@ -292,12 +342,15 @@ final class PasswordStoreTest: XCTestCase {
 
     private enum RemoteRepo {
         case empty
+        case emptyDirs
         case withGPGID
 
         var url: URL {
             switch self {
             case .empty:
                 Bundle(for: PasswordStoreTest.self).resourceURL!.appendingPathComponent("Fixtures/password-store-empty.git")
+            case .emptyDirs:
+                Bundle(for: PasswordStoreTest.self).resourceURL!.appendingPathComponent("Fixtures/password-store-empty-dirs.git")
             case .withGPGID:
                 Bundle(for: PasswordStoreTest.self).resourceURL!.appendingPathComponent("Fixtures/password-store-with-gpgid.git")
             }
@@ -307,6 +360,8 @@ final class PasswordStoreTest: XCTestCase {
             switch self {
             case .empty:
                 "main"
+            case .emptyDirs:
+                "main"
             case .withGPGID:
                 "master"
             }