DeskTop: Fix issues with replacing files/folders during copy

During a file copy, if creating the destination file failed because it already exists, DeskTop would mutate the destination file to match the source. This was bogus in two cases: * When replacing a regular file with a folder, the storage type would be bogus and the directory file wouldn't be populated correctly. * When replacing a folder with anything, the storage type would remain unchanged and the contents would be corrupted. Replace this by explicitly deleting the destination file if needed, and creating from scratch. Since recursive deletion is non-trivial, for now an error is shown if a folder is being replaced. That could be revisited in the future, but it's better than it was!
a2stuff · Mar 19, 2024 · 16ce268 · 16ce268
1 parent 96c93c6
commit 16ce268
Show file tree

Hide file tree

Showing 13 changed files with 50 additions and 41 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -20,6 +20,7 @@ Project Page: https://github.com/a2stuff/a2d
 * Prevent scrollbar thumb jumping when not moved. ([#778](https://github.com/a2stuff/a2d/issues/778))
 * Package now includes ProDOS 2.4.3 and BASIC.system 1.7.
 * Improve appearance of buttons when clicked. c/o @buserror
+* Fix corruption issues when replacing files with folders and folders with anything during copy.
 
 ### Launcher
 * If starting from a folder, brand it as a system folder.

diff --git a/desktop/auxmem.s b/desktop/auxmem.s
@@ -285,6 +285,8 @@ str_large_copy_prompt:
         PASCAL_STRING res_string_errmsg_too_large_to_copy
 str_large_move_prompt:
         PASCAL_STRING res_string_errmsg_too_large_to_move
+str_no_overwrite_dir:
+        PASCAL_STRING res_string_errmsg_overwrite_dir
 
         ;; "Delete" dialog strings
 str_delete_confirm_prefix:

diff --git a/desktop/main.s b/desktop/main.s
@@ -11271,14 +11271,14 @@ file_entry_buf          .tag    FileEntry
 
         DEFINE_CLOSE_PARAMS close_src_params
         DEFINE_CLOSE_PARAMS close_dst_params
-        DEFINE_DESTROY_PARAMS destroy_params, src_path_buf
+        DEFINE_DESTROY_PARAMS destroy_src_params, src_path_buf
+        DEFINE_DESTROY_PARAMS destroy_dst_params, dst_path_buf
         DEFINE_OPEN_PARAMS open_src_params, src_path_buf, $0D00
         DEFINE_OPEN_PARAMS open_dst_params, dst_path_buf, $1100
         DEFINE_READ_PARAMS read_src_params, file_data_buffer, kBufSize
         DEFINE_WRITE_PARAMS write_dst_params, file_data_buffer, kBufSize
         DEFINE_CREATE_PARAMS create_params3, dst_path_buf, ACCESS_DEFAULT
 
-        DEFINE_SET_EOF_PARAMS set_eof_params, 0
         DEFINE_SET_MARK_PARAMS mark_src_params, 0
         DEFINE_SET_MARK_PARAMS mark_dst_params, 0
         DEFINE_ON_LINE_PARAMS on_line_params2,, $800
@@ -11767,7 +11767,7 @@ ok_dir: jsr     RemoveSrcPathSegment
         bpl     done
 
         ;; Was a move - delete file
-@retry: MLI_CALL DESTROY, destroy_params
+@retry: MLI_CALL DESTROY, destroy_src_params
         bcc     done
         cmp     #ERR_ACCESS_ERROR
         bne     :+
@@ -11886,13 +11886,14 @@ existing_size:
 ;;; ============================================================
 ;;; Common implementation used by both `CopyProcessSelectedFile`
 ;;; and `CopyProcessDirectoryEntry`
+;;; Output: C=0 on success, C=1 on failure
 
 .proc TryCreateDst
         bit     move_flag       ; same volume relink move?
     IF_VC
         ;; No, verify that there is room.
         jsr     CheckSpaceAndShowPrompt
-        bcs     failure
+        RTS_IF_CS
     END_IF
 
         ;; Copy file_type, aux_type, storage_type
@@ -11918,11 +11919,23 @@ existing_size:
         lda     #ST_LINKED_DIRECTORY
         sta     create_params3::storage_type
 :
-retry:  MLI_CALL CREATE, create_params3
-        bcc     success
 
-        cmp     #ERR_DUPLICATE_FILENAME
-        bne     err
+        ;; --------------------------------------------------
+retry:  jsr     GetDstFileInfo
+        bcs     create
+        cmp     #ERR_FILE_NOT_FOUND
+        beq     create
+
+        ;; File exists
+        lda     dst_file_info_params::storage_type
+        cmp     #ST_LINKED_DIRECTORY
+    IF_EQ
+        ;; TODO: In the future, prompt and recursively delete
+        param_call ShowAlertParams, AlertButtonOptions::OK, aux::str_no_overwrite_dir
+        jsr     SetCursorWatch
+        jmp     CloseFilesCancelDialog
+    END_IF
+        ;; Prompt to replace
         bit     all_flag
         bmi     yes
 
@@ -11936,12 +11949,16 @@ retry:  MLI_CALL CREATE, create_params3
         cmp     #kAlertResultAll
         bne     cancel
         copy    #$80, all_flag
-yes:    jsr     ApplyFileInfoAndSize
-        jmp     success
-
-cancel: jmp     CloseFilesCancelDialog
+yes:
+        MLI_CALL DESTROY, destroy_dst_params
+        bcs     retry
 
-err:    jsr     ShowErrorAlertDst
+        ;; --------------------------------------------------
+        ;; Create the file
+create:
+        MLI_CALL CREATE, create_params3
+        bcc     success
+        jsr     ShowErrorAlertDst
         jmp     retry
 
 success:
@@ -11957,6 +11974,8 @@ success:
 failure:
         sec
         rts
+
+cancel: jmp     CloseFilesCancelDialog
 .endproc ; TryCreateDst
 
 ;;; ============================================================
@@ -12128,7 +12147,7 @@ Start:  lda     DEVNUM
         ;; --------------------------------------------------
         ;; Delete the file at the source location.
 
-:       MLI_CALL DESTROY, destroy_params
+:       MLI_CALL DESTROY, destroy_src_params
         bcc     :+
         jsr     ShowErrorAlert
         jmp     :-
@@ -12436,7 +12455,7 @@ do_destroy:
 ;;; and `DeleteProcessDirectoryEntry`
 
 .proc DeleteFileCommon
-retry:  MLI_CALL DESTROY, destroy_params
+retry:  MLI_CALL DESTROY, destroy_src_params
         bcc     done
 
         ;; Failed - determine why, maybe try to unlock.
@@ -12974,32 +12993,6 @@ match:  lda     flag
 
 ;;; ============================================================
 
-.proc ApplyFileInfoAndSize
-:       jsr     CopyFileInfo
-        copy    #ACCESS_DEFAULT, dst_file_info_params::access
-        jsr     SetDstFileInfo
-        lda     src_file_info_params::file_type
-        cmp     #FT_DIRECTORY
-        beq     done
-
-        ;; If a regular file, open/set eof/close
-        MLI_CALL OPEN, open_dst_params
-        bcc     :+
-        jsr     ShowErrorAlertDst
-        jmp     :-              ; retry
-
-:       lda     open_dst_params::ref_num
-        sta     set_eof_params::ref_num
-        sta     close_dst_params::ref_num
-@retry: MLI_CALL SET_EOF, set_eof_params
-        bcc     close
-        jsr     ShowErrorAlertDst
-        jmp     @retry
-
-close:  MLI_CALL CLOSE, close_dst_params
-done:   rts
-.endproc ; ApplyFileInfoAndSize
-
 .proc CopyFileInfo
         COPY_BYTES 11, src_file_info_params::access, dst_file_info_params::access
         rts

diff --git a/desktop/res/desktop.res.da b/desktop/res/desktop.res.da
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "Den fil findes allerede. Vil du erstatte det?"
 .define res_string_errmsg_too_large_to_copy "Denne fil er for stor til at kopiere."
 .define res_string_errmsg_too_large_to_move "Denne fil er for stor til at flytte."
+.define res_string_errmsg_overwrite_dir "En mappe kan ikke erstattes af en anden vare. Slet det s} pr|v igen."
 .define res_string_prompt_delete_confirm_prefix "Er du sikker p}, at du vil slette "
 .define res_string_prompt_delete_confirm_suffix " permanent?"
 .define res_string_label_delete_count "Sletter: "

diff --git a/desktop/res/desktop.res.de b/desktop/res/desktop.res.de
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "Datei bereits vorhanden. ]berschreiben?"
 .define res_string_errmsg_too_large_to_copy "Datei zu gro~ zum Kopieren."
 .define res_string_errmsg_too_large_to_move "Datei zu gro~ zum Bewegen."
+.define res_string_errmsg_overwrite_dir "Ein Ordner kann nicht durch einen anderen Artikel ersetzt werden. L|schen Sie es und versuchen Sie es erneut."
 .define res_string_prompt_delete_confirm_prefix "M|chten Sie wirklich "
 .define res_string_prompt_delete_confirm_suffix " dauerhaft l|schen?"
 .define res_string_label_delete_count "L|schen: "

diff --git a/desktop/res/desktop.res.en b/desktop/res/desktop.res.en
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "That file already exists. Do you want to replace it?"
 .define res_string_errmsg_too_large_to_copy "This file is too large to copy."
 .define res_string_errmsg_too_large_to_move "This file is too large to move."
+.define res_string_errmsg_overwrite_dir "A folder cannot be replaced by another item. Delete it then try again."
 .define res_string_prompt_delete_confirm_prefix "Are you sure you want to permanently delete "
 .define res_string_prompt_delete_confirm_suffix "?"
 .define res_string_label_delete_count "Deleting: "

diff --git a/desktop/res/desktop.res.es b/desktop/res/desktop.res.es
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "El archivo ya existe. ]Quiere reemplazarlo?"
 .define res_string_errmsg_too_large_to_copy "Archivo demasiado grande para ser escrito."
 .define res_string_errmsg_too_large_to_move "Archivo demasiado grande para mover."
+.define res_string_errmsg_overwrite_dir "Una carpeta no puede ser reemplazada por otro elemento. Elim\x11elo y luego vuelva a intentarlo."
 .define res_string_prompt_delete_confirm_prefix "]Est\x10s seguro de que quieres eliminar permanentemente "
 .define res_string_prompt_delete_confirm_suffix "?"
 .define res_string_label_delete_count "Borrado: "

diff --git a/desktop/res/desktop.res.fr b/desktop/res/desktop.res.fr
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "Le fichier existe d{j@. Voulez-vous le remplacer ?"
 .define res_string_errmsg_too_large_to_copy "Fichier trop grand."
 .define res_string_errmsg_too_large_to_move "Fichier trop large."
+.define res_string_errmsg_overwrite_dir "Un dossier ne peut pas etre remplac{ par un autre {l{ment. Supprimez-le puis r{essayez."
 .define res_string_prompt_delete_confirm_prefix "Voulez-vous vraiment supprimer d{finitivement "
 .define res_string_prompt_delete_confirm_suffix " ?"
 .define res_string_label_delete_count "Supprimer : "

diff --git a/desktop/res/desktop.res.it b/desktop/res/desktop.res.it
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "Il file esiste gi{. Vuoi sovrascriverlo?"
 .define res_string_errmsg_too_large_to_copy "File troppo grande."
 .define res_string_errmsg_too_large_to_move "File troppo grande."
+.define res_string_errmsg_overwrite_dir "Una cartella non pu| essere sostituita da un altro elemento. Elimina e poi riprova."
 .define res_string_prompt_delete_confirm_prefix "Sei sicuro di voler eliminare definitivamente "
 .define res_string_prompt_delete_confirm_suffix "?"
 .define res_string_label_delete_count "Cancellazione: "

diff --git a/desktop/res/desktop.res.nl b/desktop/res/desktop.res.nl
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "Bestand bestaat al. Overschrijven?"
 .define res_string_errmsg_too_large_to_copy "Bestand te groot om te kopi\x10ren."
 .define res_string_errmsg_too_large_to_move "Bestand te groot om te verplaatsen."
+.define res_string_errmsg_overwrite_dir "Een map kan niet worden vervangen door een ander item. Verwijder het en probeer het opnieuw."
 .define res_string_prompt_delete_confirm_prefix "Weet je zeker dat je "
 .define res_string_prompt_delete_confirm_suffix " permanent wilt verwijderen?"
 .define res_string_label_delete_count "Wissen: "

diff --git a/desktop/res/desktop.res.pt b/desktop/res/desktop.res.pt
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "O arquivo j@ existe. Quer substituir?"
 .define res_string_errmsg_too_large_to_copy "Arquivo muito grande para gravar."
 .define res_string_errmsg_too_large_to_move "Arquivo muito grande para mover."
+.define res_string_errmsg_overwrite_dir "Uma pasta n[o pode ser substitu{da por outro item. Exclua -o e tente novamente."
 .define res_string_prompt_delete_confirm_prefix "Tem certeza de que deseja excluir permanentemente "
 .define res_string_prompt_delete_confirm_suffix "?"
 .define res_string_label_delete_count "Excluindo: "

diff --git a/desktop/res/desktop.res.sv b/desktop/res/desktop.res.sv
@@ -47,6 +47,7 @@
 .define res_string_prompt_overwrite "Den filen finns redan. Vill du byta ut den?"
 .define res_string_errmsg_too_large_to_copy "Den h{r filen {r f|r stor f|r att kopieras."
 .define res_string_errmsg_too_large_to_move "Den h{r filen {r f|r stor f|r att flyttas."
+.define res_string_errmsg_overwrite_dir "En mapp kan inte ers{ttas av ett annat objekt. Radera det sedan f|rs|k igen."
 .define res_string_prompt_delete_confirm_prefix "[r du s{ker p} att du vill ta bort "
 .define res_string_prompt_delete_confirm_suffix " permanent?"
 .define res_string_label_delete_count "Raderar: "

diff --git a/res/notes/testplan.md b/res/notes/testplan.md
@@ -235,6 +235,10 @@
 * Load DeskTop. Create a folder, and a file within the folder with the same name as the folder (e.g. /RAM/F and /RAM/F/F). Try to move the file over the folder using drag and drop. Verify that an error is shown.
 * Load DeskTop. Create a folder, and a file within the folder with the same name as the folder, and another file (e.g. /RAM/F and /RAM/F/F and /RAM/F/B). Select both files and try to move them into the parent folder using drag and drop. Verify that an error is shown before any files are moved.
 
+* Load DeskTop. Create a folder on a volume. Create a non-folder file with the same name as the folder on a second volume. Drag the folder to the second volume. When prompted to overwrite, click Yes. Verify that the volume contains a folder of the appropriate name.
+* Load DeskTop. Create a folder on a volume, containing a non-folder file. Create a non-folder file with the same name as the folderon a second volume. Drag the folder to the second volume. When prompted to overwrite, click Yes. Verify that the volume contains a folder of the appropriate name, containing a non-folder file.
+* Load DeskTop. Create a non-folder file on a volume. Create a folder with the same name as the file on a second volume. Drag the file onto the second volume. Verify that an alert is shown about overwriting a directory.
+
 * Load DeskTop. Open a volume. Adjust the window size so that horizontal and vertical scrolling is required. Scroll to the bottom-right. Quit DeskTop, reload. Verify that the window size and scroll position was restored correctly.
 * Load DeskTop. Open a volume. Quit DeskTop, reload. Verify that the volume window was restored, and that the volume icon is dimmed. Close the volume window. Verify that the volume icon is no longer dimmed.
 * Load DeskTop. Open a window containing icons. View > by Name. Quit DeskTop, reload. Verify that the window is restored, and that it shows the icons in a list sorted by name, and that View > by Name is checked. Repeat for other View menu options.