widelands-dev team mailing list archive

[Nicolai Hähnle <prefect_@xxxxxxx>]

Nicolai Hähnle has proposed merging lp:~nha/widelands/filesystem into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev): crossplatform
Related bugs:
  #583774 endless loop in RealFSImpl::EnsureDirectoryExists(
  https://bugs.launchpad.net/bugs/583774


Clean up the recursiveness of EnsureDirectoryExists(), and ensure that the home directory exists.

I'm a bit surprised nobody else has run into the non-creation of ~/.widelands issue, so maybe it is done somewhere I have missed, but in a way that makes it fail given the changes to EnsureDirectoryExists()? I'd appreciate if somebody who has messed with that stuff before could look over it, especially whether it affects OSX or Windows.
-- 
https://code.launchpad.net/~nha/widelands/filesystem/+merge/25802
Your team Widelands Developers is requested to review the proposed merge of lp:~nha/widelands/filesystem into lp:widelands.

=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc	2010-03-16 20:18:53 +0000
+++ src/io/filesystem/disk_filesystem.cc	2010-05-21 20:53:27 +0000
@@ -76,7 +76,6 @@
 #ifdef WIN32
 	m_root = Directory;
 #else
-	m_root = "";
 	m_root = FS_CanonicalizeName(Directory);
 #endif
 }
@@ -86,10 +85,8 @@
  * Return true if this directory is writable.
  */
 bool RealFSImpl::IsWritable() const {
-	return true; // should be checked in constructor
-
-	//fweber: no, should be checked here, because the ondisk state might have
-	//changed since then
+	// Should be checked here (ondisk state can change)
+	return true;
 }
 
 /**
@@ -294,23 +291,27 @@
  * Create this directory if it doesn't exist, throws an error
  * if the dir can't be created or if a file with this name exists
  */
-void RealFSImpl::EnsureDirectoryExists(std::string const & dirname) {
-	FileSystemPath fspath(FS_CanonicalizeName(dirname));
-	if (fspath.m_exists and fspath.m_isDirectory)
-		return; //  ok, dir is already there
+void RealFSImpl::EnsureDirectoryExists(std::string const & dirname)
+{
 	try {
-		MakeDirectory(dirname);
-	} catch (DirectoryCannotCreate_error const & e) {
-		// need more work to do it right
-		//  iterate through all possible directories
-		size_t it = 0;
-		while (it != dirname.size() && it != std::string::npos) {
+		std::string::size_type it = 0;
+		while (it < dirname.size()) {
 			it = dirname.find('/', it);
-			EnsureDirectoryExists(dirname.substr(0, it));
-			++it; //make sure we don't keep finding the same directories
+
+			FileSystemPath fspath(FS_CanonicalizeName(dirname.substr(0, it)));
+			if (fspath.m_exists and !fspath.m_isDirectory)
+				throw wexception
+					("%s exists and is not a directory",
+					dirname.substr(0, it).c_str());
+			if (!fspath.m_exists)
+				MakeDirectory(dirname.substr(0, it));
+
+			if (it == std::string::npos)
+				break;
+			++it;
 		}
-	} catch (_wexception const & e) {
-		throw wexception ("RealFSImpl::EnsureDirectory"); //, e.what());
+	} catch (const std::exception & e) {
+		throw wexception("RealFSImpl::EnsureDirectoryExists(%s): %s", dirname.c_str(), e.what());
 	}
 }
 
@@ -338,8 +339,9 @@
 		 ==
 		 -1)
 		throw DirectoryCannotCreate_error
-			("could not create directory %s: %s",
-			 dirname.c_str(), strerror(errno));
+			("RealFSImpl::MakeDirectory",
+			 dirname,
+			 strerror(errno));
 }
 
 /**

=== modified file 'src/io/filesystem/filesystem.h'
--- src/io/filesystem/filesystem.h	2010-03-16 20:18:53 +0000
+++ src/io/filesystem/filesystem.h	2010-05-21 20:53:27 +0000
@@ -94,7 +94,7 @@
 	 */
 	virtual StreamWrite * OpenStreamWrite(std::string const & fname) = 0;
 
-	virtual FileSystem &   MakeSubFileSystem(std::string const & dirname) = 0;
+	virtual FileSystem & MakeSubFileSystem(std::string const & dirname) = 0;
 	virtual FileSystem & CreateSubFileSystem
 		(std::string const & dirname, Type) = 0;
 	virtual void Unlink(std::string const &) = 0;
@@ -114,7 +114,7 @@
 	bool pathIsAbsolute(std::string const & path) const;
 	static char const * FS_Filename(char const *);
 	static char const * FS_Filename(char const *, char const * & extension);
-    static std::string FS_FilenameWoExt(char const *);
+	static std::string FS_FilenameWoExt(char const *);
 	static std::string GetHomedir();
 
 	virtual unsigned long long DiskSpace() = 0;

=== modified file 'src/io/filesystem/layered_filesystem.cc'
--- src/io/filesystem/layered_filesystem.cc	2010-03-16 20:18:53 +0000
+++ src/io/filesystem/layered_filesystem.cc	2010-05-21 20:53:27 +0000
@@ -56,24 +56,35 @@
 }
 
 /**
- * Add a new filesystem to the top of the stack
+ * Add a new filesystem to the top of the stack.
+ * Take ownership of the given filesystem.
  * \todo throw on failure
  */
 void LayeredFileSystem::AddFileSystem(FileSystem & fs)
 {
 	//only add it to the stack if there isn't a conflicting version file in
 	//the directory
-	if (! FindConflictingVersionFile(fs)) {
+	if (!FindConflictingVersionFile(fs)) {
 		m_filesystems.push_back(&fs);
 	} else {
+		delete &fs;
 		log("File system NOT added\n");
 	}
 }
+
+/**
+ * Set the home filesystem (which is the preferred filesystem for writing files).
+ * Take ownership of the given filesystem.
+ */
 void LayeredFileSystem::SetHomeFileSystem(FileSystem & fs)
 {
-	if (! FindConflictingVersionFile(fs)) {
+	delete m_home;
+	m_home = 0;
+
+	if (!FindConflictingVersionFile(fs)) {
 		m_home = &fs;
 	} else {
+		delete &fs;
 		log("File system NOT added\n");
 	}
 }

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2010-04-28 16:12:03 +0000
+++ src/wlapplication.cc	2010-05-21 20:53:27 +0000
@@ -200,12 +200,12 @@
 		//assume some dir exists
 		try {
 			log ("Set home directory: %s\n", m_homedir.c_str());
-			g_fs->SetHomeFileSystem(FileSystem::Create(m_homedir.c_str()));
-		} catch (FileNotFound_error     const & e) {
-		} catch (FileAccessDenied_error const & e) {
-			log("Access denied on %s. Continuing.\n", e.m_filename.c_str());
-		} catch (FileType_error         const & e) {
-			//TODO: handle me
+
+			std::auto_ptr<FileSystem> home(new RealFSImpl(m_homedir));
+			home->EnsureDirectoryExists(".");
+			g_fs->SetHomeFileSystem(*home.release());
+		} catch (const std::exception & e) {
+			log("Failed to add home directory: %s\n", e.what());
 		}
 	} else {
 		//TODO: complain