linuxdcpp-team team mailing list archive

[noreply@xxxxxxxxxxxxx]

------------------------------------------------------------
revno: 3151
committer: poy <poy@xxxxxxxxxx>
branch nick: trunk
timestamp: Wed 2012-12-12 23:44:47 +0100
message:
  Share file name duplicates due to directory merges (not tested)
modified:
  changelog.txt
  dcpp/ShareManager.cpp
  dcpp/ShareManager.h


--
lp:dcplusplus
https://code.launchpad.net/~dcplusplus-team/dcplusplus/trunk

Your team Dcplusplus-team is subscribed to branch lp:dcplusplus.
To unsubscribe from this branch go to https://code.launchpad.net/~dcplusplus-team/dcplusplus/trunk/+edit-subscription

=== modified file 'changelog.txt'
--- changelog.txt	2012-11-08 19:11:36 +0000
+++ changelog.txt	2012-12-12 22:44:47 +0000
@@ -9,6 +9,7 @@
 * Update Boost to version 1.52
 * Restore "Requesting" messages in the transfer list
 * [L#1071363] Apply link & plugin formatting to status messages (crise, poy)
+* [L#311818] Share file name duplicates due to directory merges (poy)
 
 -- 0.802 2012-10-20 --
 * Perf improvements using lock-free queues, requires P6 CPUs (poy)

=== modified file 'dcpp/ShareManager.cpp'
--- dcpp/ShareManager.cpp	2012-06-21 18:52:47 +0000
+++ dcpp/ShareManager.cpp	2012-12-12 22:44:47 +0000
@@ -486,17 +486,17 @@
 		Lock l(cs);
 
 		shares.emplace(realPath, vName);
-		updateIndices(*merge(dp));
+		updateIndices(*merge(dp, realPath));
 
 		setDirty();
 	}
 }
 
-ShareManager::Directory::Ptr ShareManager::merge(const Directory::Ptr& directory) {
+ShareManager::Directory::Ptr ShareManager::merge(const Directory::Ptr& directory, const string& realPath) {
 	for(auto& i: directories) {
 		if(Util::stricmp(i->getName(), directory->getName()) == 0) {
-			dcdebug("Merging directory %s\n", directory->getName().c_str());
-			i->merge(directory);
+			dcdebug("Merging directory <%s> into %s\n", realPath.c_str(), directory->getName().c_str());
+			i->merge(directory, realPath);
 			return i;
 		}
 	}
@@ -507,38 +507,63 @@
 	return directory;
 }
 
-void ShareManager::Directory::merge(const Directory::Ptr& source) {
+void ShareManager::Directory::merge(const Directory::Ptr& source, const string& realPath) {
+	// merge directories
 	for(auto& i: source->directories) {
 		auto subSource = i.second;
 
 		auto ti = directories.find(subSource->getName());
 		if(ti == directories.end()) {
-			if(findFile(subSource->getName()) != files.end()) {
-				dcdebug("File named the same as directory");
-			} else {
-				directories.emplace(subSource->getName(), subSource);
-				subSource->parent = this;
+			// the directory doesn't exist; create it.
+			directories.emplace(subSource->getName(), subSource);
+			subSource->parent = this;
+
+			auto f = findFile(subSource->getName());
+			if(f != files.end()) {
+				// we have a file that has the same name as the dir being merged; rename it.
+				const_cast<File&>(*f).validateName(Util::getFilePath(f->getRealPath()));
 			}
+
 		} else {
+			// the directory was already existing; merge into it.
 			auto subTarget = ti->second;
-			subTarget->merge(subSource);
+			subTarget->merge(subSource, realPath + subSource->getName() + PATH_SEPARATOR);
 		}
 	}
 
-	// All subdirs either deleted or moved to target...
-	source->directories.clear();
-
+	// merge files
 	for(auto& i: source->files) {
-		if(findFile(i.getName()) == files.end()) {
-			if(directories.find(i.getName()) != directories.end()) {
-				dcdebug("Directory named the same as file");
-			} else {
-				auto added = files.insert(i);
-				if(added.second) {
-					const_cast<File&>(*added.first).setParent(this);
-				}
-			}
+		auto& file = const_cast<File&>(i);
+
+		file.setParent(this);
+		file.validateName(realPath);
+
+		files.insert(move(file));
+	}
+}
+
+bool ShareManager::Directory::nameInUse(const string& name) const {
+	return findFile(name) != files.end() || directories.find(name) != directories.end();
+}
+
+void ShareManager::Directory::File::validateName(const string& sourcePath) {
+	if(parent->nameInUse(name)) {
+		uint32_t num = 0;
+		string base = name, ext, vname;
+		auto dot = base.rfind('.');
+		if(dot != string::npos) {
+			ext = base.substr(dot);
+			base.erase(dot);
 		}
+		do {
+			++num;
+			vname = base + " (" + Util::toString(num) + ")" + ext;
+		} while(parent->nameInUse(vname));
+		dcdebug("Renaming duplicate <%s> to <%s>\n", name.c_str(), vname.c_str());
+		realPath = sourcePath + name;
+		name = move(vname);
+	} else {
+		realPath.reset();
 	}
 }
 
@@ -573,7 +598,7 @@
 		if(Util::stricmp(i->second, vName) == 0 && checkHidden(i->first)) {
 			Directory::Ptr dp = buildTree(i->first, 0);
 			dp->setName(i->second);
-			merge(dp);
+			merge(dp, i->first);
 		}
 	}
 
@@ -721,7 +746,7 @@
 		if(!SETTING(LIST_DUPES)) {
 			try {
 				LogManager::getInstance()->message(str(F_("Duplicate file will not be shared: %1% (Size: %2% B) Dupe matched against: %3%")
-				% Util::addBrackets(dir.getRealPath(f.getName())) % Util::toString(f.getSize()) % Util::addBrackets(j->second->getParent()->getRealPath(j->second->getName()))));
+					% Util::addBrackets(f.getRealPath()) % Util::toString(f.getSize()) % Util::addBrackets(j->second->getRealPath())));
 				dir.files.erase(i);
 			} catch (const ShareException&) {
 			}
@@ -784,12 +809,12 @@
 
 		lastFullUpdate = GET_TICK();
 
-		DirList newDirs;
+		vector<pair<Directory::Ptr, string>> newDirs;
 		for(auto& i: dirs) {
 			if (checkHidden(i.second)) {
 				Directory::Ptr dp = buildTree(i.second, Directory::Ptr());
 				dp->setName(i.first);
-				newDirs.push_back(dp);
+				newDirs.emplace_back(dp, i.second);
 			}
 		}
 
@@ -798,7 +823,7 @@
 			directories.clear();
 
 			for(auto& i: newDirs) {
-				merge(i);
+				merge(i.first, i.second);
 			}
 
 			rebuildIndices();

=== modified file 'dcpp/ShareManager.h'
--- dcpp/ShareManager.h	2012-05-27 14:02:55 +0000
+++ dcpp/ShareManager.h	2012-12-12 22:44:47 +0000
@@ -24,6 +24,8 @@
 #include <set>
 #include <unordered_map>
 
+#include <boost/optional.hpp>
+
 #include "TimerManager.h"
 #include "SearchManager.h"
 #include "SettingsManager.h"
@@ -143,28 +145,28 @@
 			File() : size(0), parent(0) { }
 			File(const string& aName, int64_t aSize, const Directory::Ptr& aParent, const TTHValue& aRoot) :
 			name(aName), tth(aRoot), size(aSize), parent(aParent.get()) { }
-			File(const File& rhs) :
-			name(rhs.getName()), tth(rhs.getTTH()), size(rhs.getSize()), parent(rhs.getParent()) { }
-
-			~File() { }
-
-			File& operator=(const File& rhs) {
-				name = rhs.name; size = rhs.size; parent = rhs.parent; tth = rhs.tth;
-				return *this;
-			}
 
 			bool operator==(const File& rhs) const {
 				return getParent() == rhs.getParent() && (Util::stricmp(getName(), rhs.getName()) == 0);
 			}
 
+			/** Ensure this file's name doesn't clash with the names of the parent directory's sub-
+			directories or files; rename to "file (N).ext" otherwise (and set realPath to the
+			actual path on disk).
+			@param sourcePath Real path (on the disk) of the directory this file came from. */
+			void validateName(const string& sourcePath);
+
 			string getADCPath() const { return parent->getADCPath() + name; }
 			string getFullName() const { return parent->getFullName() + name; }
-			string getRealPath() const { return parent->getRealPath(name); }
+			string getRealPath() const { return realPath ? realPath.get() : parent->getRealPath(name); }
 
 			GETSET(string, name, Name);
 			GETSET(TTHValue, tth, TTH);
 			GETSET(int64_t, size, Size);
 			GETSET(Directory*, parent, Parent);
+
+		private:
+			boost::optional<string> realPath;
 		};
 
 		int64_t size;
@@ -182,6 +184,10 @@
 		string getFullName() const noexcept;
 		string getRealPath(const std::string& path) const;
 
+		/** Check whether the given name would clash with this directory's sub-directories or
+		files. */
+		bool nameInUse(const string& name) const;
+
 		int64_t getSize() const noexcept;
 
 		void search(SearchResultList& aResults, StringSearch::List& aStrings, int aSearchType, int64_t aSize, int aFileType, Client* aClient, StringList::size_type maxResults) const noexcept;
@@ -193,7 +199,7 @@
 
 		File::Set::const_iterator findFile(const string& aFile) const { return find_if(files.begin(), files.end(), Directory::File::StringComp(aFile)); }
 
-		void merge(const Ptr& source);
+		void merge(const Ptr& source, const string& realPath);
 
 		GETSET(string, name, Name);
 		GETSET(Directory*, parent, Parent);
@@ -262,8 +268,9 @@
 	typedef std::list<Directory::Ptr> DirList;
 	DirList directories;
 
-	/** Map real name to virtual name - multiple real names may be mapped to a single virtual one */
-	StringMap shares;
+	/** Map real name to virtual name - multiple real names may be mapped to a single virtual one.
+	The map is sorted to make sure conflicts are always resolved in the same order when merging. */
+	map<string, string> shares;
 
 	typedef unordered_map<TTHValue, Directory::File::Set::const_iterator> HashFileMap;
 	typedef HashFileMap::iterator HashFileIter;
@@ -282,7 +289,7 @@
 	void updateIndices(Directory& aDirectory);
 	void updateIndices(Directory& dir, const Directory::File::Set::iterator& i);
 
-	Directory::Ptr merge(const Directory::Ptr& directory);
+	Directory::Ptr merge(const Directory::Ptr& directory, const string& realPath);
 
 	void generateXmlList();
 	bool loadCache() noexcept;