Keep the last value when collapsing duplicate config lines

SaveConfig collapsed duplicate lines to the first matching entry's value,
but Options::SetOption overwrites the entry in place while parsing, so the
last line for a name is the value nzbget actually runs on. Healing a
damaged config therefore discarded the live value and fell back to the
option default on the next start - a disabled news server came back
enabled.

Resolve duplicates the same way the loader does: keep the first
occurrence's name and position, but the last occurrence's value. Applied
to both the replace and append paths. Extend the regression tests to
assert the surviving value, not just the line count.

Author: xbmc4lyfe

daemon/extension/ScriptConfig.cpp

@@ -93,6 +93,22 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries)
 	std::vector<CString> config;
 	std::set<CString, CaseInsensitiveLess> writtenOptions;
 
+	// Options::SetOption overwrites the existing entry while parsing, so when a
+	// name occurs on several lines the last one is the value nzbget runs on.
+	// Collapsing duplicates must preserve that value, otherwise saving a damaged
+	// config silently changes settings.
+	auto findLastValue = [optEntries](const char* name) -> const char*
+	{
+		for (auto it = optEntries->rbegin(); it != optEntries->rend(); ++it)
+		{
+			if (!strcasecmp(it->GetName(), name))
+			{
+				return it->GetValue();
+			}
+		}
+		return nullptr;
+	};
+
 	// read config file into memory array
 	int fileLen = (int)FileSystem::FileSize(g_Options->GetConfigFilename()) + 1;
 	CString content;
@@ -119,10 +135,12 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries)
 			{
 				Options::OptEntry* optEntry = optEntries->FindOption(optname);
 				// write each option only once, dropping duplicate lines accumulated
-				// in the config file by earlier versions (issue #588)
+				// in the config file by earlier versions (issue #588); keep the
+				// first occurrence's name and position but the last occurrence's
+				// value, so the collapsed line is the one nzbget was running on
 				if (optEntry && writtenOptions.find(optEntry->GetName()) == writtenOptions.end())
 				{
-					infile.Print("%s=%s\n", optEntry->GetName(), optEntry->GetValue());
+					infile.Print("%s=%s\n", optEntry->GetName(), findLastValue(optEntry->GetName()));
 					writtenOptions.insert(optEntry->GetName());
 				}
 			}
@@ -139,7 +157,7 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries)
 		std::set<CString, CaseInsensitiveLess>::iterator fit = writtenOptions.find(optEntry.GetName());
 		if (fit == writtenOptions.end())
 		{
-			infile.Print("%s=%s\n", optEntry.GetName(), optEntry.GetValue());
+			infile.Print("%s=%s\n", optEntry.GetName(), findLastValue(optEntry.GetName()));
 			writtenOptions.insert(optEntry.GetName());
 		}
 	}

tests/extension/ScriptConfig.cpp

@@ -65,7 +65,9 @@ BOOST_AUTO_TEST_CASE(SaveConfigDoesNotAppendDuplicateOptionNames)
 		contents << firstFile.rdbuf();
 		firstContents = contents.str();
 	}
-	BOOST_CHECK_EQUAL(firstContents, "# existing config\nExtension.Option=first\n");
+	// one line only, carrying the last entry's value - Options::SetOption
+	// resolves duplicates the same way, so the file matches what nzbget loads
+	BOOST_CHECK_EQUAL(firstContents, "# existing config\nExtension.Option=second\n");
 
 	BOOST_REQUIRE(scriptConfig.SaveConfig(&optEntries));
 	std::string secondContents;
@@ -116,4 +118,46 @@ BOOST_AUTO_TEST_CASE(SaveConfigRemovesDuplicateLinesFromDamagedConfig)
 		"Server2.Host=news.example.com\n");
 }
 
+// When a config file carries duplicate lines for one option, Options::SetOption
+// overwrites the same entry while parsing, so the LAST line is the value nzbget
+// actually runs on. Collapsing the duplicates must preserve that value,
+// otherwise saving a damaged config silently changes settings (issue #588).
+BOOST_AUTO_TEST_CASE(SaveConfigKeepsLastValueWhenConfigHasDuplicateLines)
+{
+	TempConfigFile configFile;
+	{
+		std::ofstream output(configFile.path);
+		output << "# existing config\n"
+			<< "Server2.Active=yes\n"
+			<< "Server2.Name=xsusenet\n"
+			<< "server2.active=no\n";
+	}
+
+	OptionsGuard optionsGuard;
+	Options options("nzbget", configFile.path.string().c_str(), true, nullptr, nullptr);
+	g_Options = &options;
+
+	// as loadconfig reports it: raw file lines, duplicates included
+	Options::OptEntries optEntries;
+	optEntries.emplace_back("Server2.Active", "yes");
+	optEntries.emplace_back("Server2.Name", "xsusenet");
+	optEntries.emplace_back("server2.active", "no");
+	ScriptConfig scriptConfig;
+
+	BOOST_REQUIRE(scriptConfig.SaveConfig(&optEntries));
+	std::string contents;
+	{
+		std::ifstream file(configFile.path);
+		std::stringstream buffer;
+		buffer << file.rdbuf();
+		contents = buffer.str();
+	}
+	// one line per option, keeping the first occurrence's name and position
+	// but the last occurrence's value - exactly what Options::SetOption does
+	BOOST_CHECK_EQUAL(contents,
+		"# existing config\n"
+		"Server2.Active=no\n"
+		"Server2.Name=xsusenet\n");
+}
+
 BOOST_AUTO_TEST_SUITE_END()