From 0db0c6e0d55c177a4d3acccf23960e049ff10da6 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Thu, 4 Oct 2018 13:17:21 -0400 Subject: [PATCH] Reorganize some commit logic in DirManager::SetProject --- src/DirManager.cpp | 233 +++++++++++++++++++++++---------------------- src/DirManager.h | 4 +- 2 files changed, 123 insertions(+), 114 deletions(-) diff --git a/src/DirManager.cpp b/src/DirManager.cpp index 41900141a..c1b518dcc 100644 --- a/src/DirManager.cpp +++ b/src/DirManager.cpp @@ -474,16 +474,65 @@ void DirManager::CleanDir( RecursivelyRemove(dirPathArray, count, countFiles, flags | kCleanDirs, msg); } -bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const bool bCreate) -{ - bool moving = true; - wxString oldPath = this->projPath; - wxString oldName = this->projName; - wxString oldFull = projFull; - wxString oldLoc = projFull; - if (oldLoc == wxT("")) - oldLoc = mytemp; +namespace { + struct PathRestorer { + PathRestorer( + bool &commitFlag, wxString &path, wxString &name, wxString &full ) + : committed( commitFlag ) + , projPath( path ) + , projName( name ) + , projFull( full ) + + , oldPath( path ) + , oldName( name ) + , oldFull( full ) + {} + + ~PathRestorer() + { + if (!committed) + projFull = oldFull, projName = oldName, projPath = oldPath; + } + + bool &committed; + wxString &projPath, &projName, &projFull; + const wxString oldPath, oldName, oldFull; + }; + + struct DirCleaner { + DirCleaner( bool &commitFlag, const wxString &path ) + : committed( commitFlag ) + , fullPath( path ) + {} + ~DirCleaner() + { + if (!committed) + DirManager::CleanDir( + fullPath, + wxEmptyString, + wxEmptyString, + _("Cleaning up after failed save"), + kCleanTopDirToo); + } + + bool &committed; + wxString fullPath; + }; +} + +bool DirManager::SetProject( + wxString& newProjPath, const wxString& newProjName, const bool bCreate) +{ + bool committed = false; + + // Save old state of paths in case of failure + PathRestorer pathRestorer{ committed, projPath, projName, projFull }; + + // Remember old path to be cleaned up in case of successful move + const auto oldFull = projFull; + + // Choose new paths if (newProjPath == wxT("")) newProjPath = ::wxGetCwd(); @@ -494,22 +543,11 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const else this->projFull = newProjPath + wxFILE_SEP_PATH + newProjName; - wxString cleanupLoc1=oldLoc; - wxString cleanupLoc2=projFull; - - bool created = false; - + // Verify new paths, maybe creating a directory if (bCreate) { - if (!wxDirExists(projFull)) { - if (!wxMkdir(projFull)) { - this->projFull = oldFull; - this->projPath = oldPath; - this->projName = oldName; - return false; - } - else - created = true; - } + if (!wxDirExists(projFull) && + !wxMkdir(projFull)) + return false; #ifdef __UNIX__ chmod(OSFILENAME(projFull), 0775); @@ -519,14 +557,14 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const chmod(OSFILENAME(projFull), 0775); #endif - } else { - if (!wxDirExists(projFull)) { - this->projFull = oldFull; - this->projPath = oldPath; - this->projName = oldName; - return false; - } } + else if (!wxDirExists(projFull)) + return false; + + // Be prepared to un-create directory on failure + Maybe dirCleaner; + if (bCreate) + dirCleaner.create( committed, projFull ); /* Move all files into this NEW directory. Files which are "locked" get copied instead of moved. (This happens when @@ -534,111 +572,79 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const saved version of the old project must not be moved, otherwise the old project would not be safe.) */ - int trueTotal = 0; + bool moving = true; + size_t trueTotal = 0; + wxArrayString newPaths; { - /*i18n-hint: This title appears on a dialog that indicates the progress in doing something.*/ + /* i18n-hint: This title appears on a dialog that indicates the progress + in doing something.*/ ProgressDialog progress(_("Progress"), _("Saving project data files")); int total = mBlockFileHash.size(); - bool success = true; - int count = 0; - wxArrayString newPaths; - for (const auto &pair : mBlockFileHash) - { + for (const auto &pair : mBlockFileHash) { + if( progress.Update(newPaths.size(), total) != ProgressResult::Success ) + return false; + wxString newPath; - BlockFilePtr b = pair.second.lock(); - if (b) { - // FIXME: TRAP_ERR - // JKC: The 'success' variable and recovery strategy looks - // broken/bogus to me. Would need to be using &= to catch - // failure in one of the copies/moves. Besides which, - // our temporary files are going to be deleted when we exit - // anyway, if saving from temporary to named project. - - if( progress.Update(count, total) != ProgressResult::Success ) - success = false; - else { - moving = moving && !b->IsLocked(); - auto result = CopyToNewProjectDirectory( &*b ); - success = result.first; - newPath = result.second; - } - - if (success) { - count++; - } - else - break; + if (auto b = pair.second.lock()) { + moving = moving && !b->IsLocked(); + auto result = CopyToNewProjectDirectory( &*b ); + if (!result.first) + return false; + newPath = result.second; + ++trueTotal; } - newPaths.push_back( newPath ); } + } - // in case there are any nulls - trueTotal = count; + // We have built all of the new file tree. + committed = true; - if (success) { - auto size = newPaths.size(); - wxASSERT( size == mBlockFileHash.size() ); + auto size = newPaths.size(); + wxASSERT( size == mBlockFileHash.size() ); - // Commit changes to filenames in the BlockFile objects, and removal - // of files at old paths, ONLY NOW! This must be nothrow. + // Commit changes to filenames in the BlockFile objects, and removal + // of files at old paths, ONLY NOW! This must be nothrow. - // This copy-then-delete procedure is needed to make it safe to - // attempt save to another storage device, but fail. + // This copy-then-delete procedure is needed to make it safe to + // attempt save to another storage device, but fail. - // It has the consequence that saving a project from one part of - // the device to another will not succeed unless there is sufficient - // space to hold originals and copies at the same time. Perhaps the - // extra cautions are not needed in that case, and the old procedure - // of renaming first, and reversing the renamings in case of failure, - // could still work safely. + // It has the consequence that saving a project from one part of + // the device to another will not succeed unless there is sufficient + // space to hold originals and copies at the same time. Perhaps the + // extra cautions are not needed in that case, and the old procedure + // of renaming first, and reversing the renamings in case of failure, + // could still work safely. - // But I don't know whether wxWidgets gives us a reliable means to - // distinguish that case. + // But I don't know whether wxWidgets gives us a reliable means to + // distinguish that case. - // I will err on the side of safety and simplicity and follow the - // same procedure in all cases. + // I will err on the side of safety and simplicity and follow the + // same procedure in all cases. - size_t ii = 0; - for (const auto &pair : mBlockFileHash) - { - BlockFilePtr b = pair.second.lock(); + size_t ii = 0; + for (const auto &pair : mBlockFileHash) + { + BlockFilePtr b = pair.second.lock(); - if (b) { - if (moving || !b->IsLocked()) { - auto result = b->GetFileName(); - oldPath = result.name.GetFullPath(); - if (!oldPath.empty()) - wxRemoveFile( oldPath ); - } - - if (ii < size) - b->SetFileName( - wxFileNameWrapper{ wxFileName{ newPaths[ii] } } ); - } - - ++ii; + if (b) { + if (moving || !b->IsLocked()) { + auto result = b->GetFileName(); + auto oldPath = result.name.GetFullPath(); + if (!oldPath.empty()) + wxRemoveFile( oldPath ); } - } - else { - this->projFull = oldFull; - this->projPath = oldPath; - this->projName = oldName; - if (created) - CleanDir( - cleanupLoc2, - wxEmptyString, - wxEmptyString, - _("Cleaning up after failed save"), - kCleanTopDirToo); - - return false; + if (ii < size) + b->SetFileName( + wxFileNameWrapper{ wxFileName{ newPaths[ii] } } ); } + + ++ii; } // Some subtlety; SetProject is used both to move a temp project @@ -661,6 +667,7 @@ bool DirManager::SetProject(wxString& newProjPath, wxString& newProjName, const // that we didn't put there, but that Finder may insert into the folders, // and mercilessly remove them, in addition to removing the directories. + auto cleanupLoc1 = oldFull.empty() ? mytemp : oldFull; CleanDir( cleanupLoc1, wxEmptyString, // EmptyString => ALL directories. diff --git a/src/DirManager.h b/src/DirManager.h index 67fbed234..98a915356 100644 --- a/src/DirManager.h +++ b/src/DirManager.h @@ -62,7 +62,9 @@ class PROFILE_DLL_API DirManager final : public XMLTagHandler { // Returns true on success. // If SetProject is told NOT to create the directory // but it doesn't already exist, SetProject fails and returns false. - bool SetProject(wxString& newProjPath, wxString& newProjName, const bool bCreate); + bool SetProject( + wxString& newProjPath, // assigns it if empty + const wxString& newProjName, const bool bCreate); wxString GetProjectDataDir(); wxString GetProjectName();