fix: keep the previous repository copy when a download fails part way

code/+openminds/+internal/+utility/+git/downloadRepository.m

@@ -7,6 +7,11 @@ function downloadRepository(repositoryName, options)
 %   Optional parameters:
 %       BranchName - Name of branch
 %       Owner - Name of repository owner
+%       TargetDirectory - Folder the repository folder is placed in
+%
+%   The commit is recorded only once the repository is in place, so a
+%   download that failed is retried on the next call rather than taken
+%   for the current commit.
 
     arguments
         repositoryName = "openMINDS"
@@ -17,8 +22,9 @@ function downloadRepository(repositoryName, options)
 
     % Todo: Should be a preference.
     targetDirectory = options.TargetDirectory;
-    
+
     import openminds.internal.extern.fex.filedownload.downloadFile
+    import openminds.internal.utility.git.extractRepositoryArchive
     import openminds.internal.utility.git.getCurrentCommitID
     import openminds.internal.utility.git.writeRecordedCommitID
     import openminds.internal.utility.git.isRecordedCommitCurrent
@@ -37,40 +43,16 @@ function downloadRepository(repositoryName, options)
     % - Create path for saving and download types
     zipFileName = webURI.Path(end);
     tempZipFilepath = tempname + "-" + zipFileName;
-    C1 = onCleanup(@(pathStr) delete(tempZipFilepath) );
-   
+    zipCleanup = onCleanup(@() delete(tempZipFilepath));
+
     fprintf('Downloading repository "%s" from "%s"... ', ...
         repositoryName, options.Owner)
     downloadFile(tempZipFilepath, webURI.EncodedURI, 'ShowFilename', true);
     fprintf('Done.\n')
 
-    directoryForUnzip = tempname;
-    if ~isfolder(directoryForUnzip)
-        mkdir(directoryForUnzip)
-    end
-
-    C2 = onCleanup(@(pathStr, mode) rmdir(directoryForUnzip, "s") );
-
-    fprintf('Unzipping repository file (%s)... ', repositoryName)
-    unzip(tempZipFilepath, directoryForUnzip)
-    fprintf('Done.\n')
-
-    sourceDirectory = directoryForUnzip;
-
-    if ~isfolder(targetDirectory)
-        mkdir(targetDirectory)
-    end
-
-    % Get repository folder name
-    L = dir(sourceDirectory); L(startsWith({L.name}, '.')) = [];
-    assert(isscalar(L), "Expected temporary folder to contain one downloaded item")
-    folderName = strtrim( L.name );
-    if isfolder( fullfile(targetDirectory, folderName) )
-        rmdir(fullfile(targetDirectory, folderName), "s")
-    end
-
-    fprintf('Copying repository "%s" to local directory:\n%s... ', repositoryName, targetDirectory)
-    copyfile(sourceDirectory, targetDirectory)
+    fprintf('Extracting repository "%s" into local directory:\n%s... ', ...
+        repositoryName, targetDirectory)
+    extractRepositoryArchive(tempZipFilepath, targetDirectory);
     fprintf('Done.\n')
 
     % Save current commit ID and repository details

code/+openminds/+internal/+utility/+git/extractRepositoryArchive.m

@@ -0,0 +1,73 @@
+function repositoryFolder = extractRepositoryArchive(zipFilePath, targetDirectory)
+% extractRepositoryArchive - Put the repository in an archive in place of the previous copy
+%
+%   Syntax:
+%       repositoryFolder = extractRepositoryArchive(zipFilePath, targetDirectory)
+%
+%   Input Arguments:
+%       zipFilePath - A GitHub archive: a zip file holding one folder,
+%           named after the repository and branch
+%       targetDirectory - Folder the repository folder is placed in
+%
+%   Output Arguments:
+%       repositoryFolder - Folder the repository was placed in
+%
+%   The archive is extracted into a staging folder beside the repository
+%   folder and moved into place last, so an extraction that fails part
+%   way, from a full disk or an interrupted job, leaves the previous
+%   copy where it was instead of a folder that exists but is incomplete.
+%   Nothing of the staging folder remains afterwards, whether or not the
+%   extraction succeeded.
+%
+%   See also downloadRepository
+
+    arguments
+        zipFilePath (1,1) string {mustBeFile}
+        targetDirectory (1,1) string
+    end
+
+    if ~isfolder(targetDirectory)
+        mkdir(targetDirectory)
+    end
+
+    % Beside the repository folder, not under tempdir, so that moving the
+    % result into place is a rename on one volume rather than a second
+    % copy of every file.
+    [~, stagingName] = fileparts(tempname);
+    stagingDirectory = fullfile(targetDirectory, stagingName);
+    stagingCleanup = onCleanup(@() removeFolder(stagingDirectory));
+
+    unzip(zipFilePath, stagingDirectory)
+
+    L = dir(stagingDirectory);
+    L(startsWith({L.name}, '.')) = [];
+    if ~isscalar(L) || ~L.isdir
+        error('OPENMINDS:Git:UnexpectedArchiveLayout', ...
+            ['The archive "%s" was expected to hold one folder, as a ', ...
+            'GitHub archive does, but holds %d items.'], zipFilePath, numel(L))
+    end
+    folderName = string(L.name);
+    stagedFolder = fullfile(stagingDirectory, folderName);
+    repositoryFolder = fullfile(targetDirectory, folderName);
+
+    % The previous copy goes only now that the new one is complete, and it
+    % must be gone before the move: movefile moves a folder into one that
+    % exists rather than over it.
+    if isfolder(repositoryFolder)
+        rmdir(repositoryFolder, "s")
+    end
+
+    [isMoved, message] = movefile(stagedFolder, repositoryFolder);
+    if ~isMoved
+        error('OPENMINDS:Git:MoveFailed', ...
+            'The repository folder "%s" could not be moved into "%s": %s', ...
+            folderName, targetDirectory, message)
+    end
+end
+
+function removeFolder(folderPath)
+% removeFolder - Remove a folder and its contents if it exists
+    if isfolder(folderPath)
+        rmdir(folderPath, "s")
+    end
+end

tools/tests/+ommtest/+internal/GitDownloadTest.m

@@ -18,5 +18,89 @@ function testDownloadRepo(testCase)
 
             testCase.verifyTrue(openminds.internal.utility.git.isRecordedCommitCurrent())
         end
+
+        function testArchiveReplacesThePreviousCopy(testCase)
+        % The previous copy is replaced, not merged into, and nothing of
+        % the staging folder remains.
+            import openminds.internal.utility.git.extractRepositoryArchive
+
+            targetDirectory = fullfile(pwd, "Repositories");
+            writeRepositoryFolder(fullfile(targetDirectory, "repo-main"), "old.txt")
+            zipFilePath = writeRepositoryArchive(pwd, "repo-main", "new.txt");
+
+            repositoryFolder = extractRepositoryArchive(zipFilePath, targetDirectory);
+
+            testCase.verifyEqual(repositoryFolder, fullfile(targetDirectory, "repo-main"))
+            testCase.verifyTrue(isfile(fullfile(repositoryFolder, "new.txt")))
+            testCase.verifyFalse(isfile(fullfile(repositoryFolder, "old.txt")), ...
+                'The previous copy must be replaced, not merged into.')
+            testCase.verifyEqual(folderNames(targetDirectory), "repo-main", ...
+                'Nothing of the staging folder may remain.')
+        end
+
+        function testFailedExtractionLeavesThePreviousCopy(testCase)
+        % An archive that cannot be read is reported, the previous copy is
+        % untouched, and nothing of the staging folder remains.
+            import openminds.internal.utility.git.extractRepositoryArchive
+
+            targetDirectory = fullfile(pwd, "Repositories");
+            writeRepositoryFolder(fullfile(targetDirectory, "repo-main"), "old.txt")
+            zipFilePath = fullfile(pwd, "damaged.zip");
+            writeTextFile(zipFilePath, "not an archive")
+
+            testCase.verifyError( ...
+                @() extractRepositoryArchive(zipFilePath, targetDirectory), ?MException)
+
+            testCase.verifyTrue(isfile(fullfile(targetDirectory, "repo-main", "old.txt")), ...
+                'The previous copy must survive a failed extraction.')
+            testCase.verifyEqual(folderNames(targetDirectory), "repo-main", ...
+                'Nothing of the staging folder may remain.')
+        end
+
+        function testAnArchiveNotHoldingOneFolderIsRejected(testCase)
+        % A GitHub archive holds one folder. Anything else is reported by
+        % identifier before the previous copy is touched.
+            import openminds.internal.utility.git.extractRepositoryArchive
+
+            targetDirectory = fullfile(pwd, "Repositories");
+            writeRepositoryFolder(fullfile(targetDirectory, "repo-main"), "old.txt")
+            sourceFolder = fullfile(pwd, "archive-source");
+            writeRepositoryFolder(fullfile(sourceFolder, "repo-main"), "new.txt")
+            writeTextFile(fullfile(sourceFolder, "stray.txt"), "x")
+            zipFilePath = fullfile(pwd, "two-items.zip");
+            zip(zipFilePath, ["repo-main", "stray.txt"], sourceFolder)
+
+            testCase.verifyError( ...
+                @() extractRepositoryArchive(zipFilePath, targetDirectory), ...
+                'OPENMINDS:Git:UnexpectedArchiveLayout')
+
+            testCase.verifyTrue(isfile(fullfile(targetDirectory, "repo-main", "old.txt")))
+            testCase.verifyEqual(folderNames(targetDirectory), "repo-main")
+        end
     end
 end
+
+function writeRepositoryFolder(folderPath, fileName)
+    mkdir(folderPath)
+    writeTextFile(fullfile(folderPath, fileName), "x")
+end
+
+function writeTextFile(filePath, text)
+    fileId = fopen(filePath, "w");
+    fwrite(fileId, text);
+    fclose(fileId);
+end
+
+function zipFilePath = writeRepositoryArchive(rootFolder, folderName, fileName)
+% An archive laid out like a GitHub archive: one folder at its top.
+    sourceFolder = fullfile(rootFolder, "archive-source");
+    writeRepositoryFolder(fullfile(sourceFolder, folderName), fileName)
+    zipFilePath = fullfile(rootFolder, folderName + ".zip");
+    zip(zipFilePath, folderName, sourceFolder)
+end
+
+function names = folderNames(folderPath)
+    L = dir(folderPath);
+    L(startsWith({L.name}, ".")) = [];
+    names = string({L.name});
+end