Harden recursive directory import edge cases

src/commands/AddCommand.cpp

@@ -1,12 +1,12 @@
 #include "commands/AddCommand.hpp"
 
+#include "Exceptions.hpp"
 #include "utils/FileUtils.hpp"
 #include "utils/LogUtils.hpp"
 #include "utils/PathUtils.hpp"
 
 #include <algorithm>
 #include <format>
-#include <stdexcept>
 #include <string>
 #include <vector>
 
@@ -27,11 +27,21 @@ std::string DescribeSkippedEntry(const fs::path& path, const std::error_code& er
 
 std::string DescribeUnsupportedEntry(const fs::path& path) { return "Skipping unsupported entry: " + path.string(); }
 
-void IncrementIterator(fs::recursive_directory_iterator& iterator, const fs::recursive_directory_iterator& end) {
+std::string DescribeSymlinkEntry(const fs::directory_entry& entry) {
+    std::error_code errorCode;
+    if (const auto targetStatus = entry.status(errorCode); !errorCode && fs::is_directory(targetStatus)) {
+        return "Skipping symlinked directory: " + entry.path().string();
+    }
+
+    return "Skipping symlink: " + entry.path().string();
+}
+
+void IncrementIterator(fs::recursive_directory_iterator& iterator, const fs::recursive_directory_iterator& end,
+                       const fs::path& entryPath) {
     std::error_code errorCode;
     iterator.increment(errorCode);
     if (errorCode) {
-        utils::LogWarn("Skipping directory entry: " + errorCode.message());
+        utils::LogWarn(DescribeSkippedEntry(entryPath, errorCode));
         iterator = end;
     }
 }
@@ -53,33 +63,33 @@ std::vector<fs::path> CollectOrdinaryFiles(const fs::path& directoryPath) {
         if (errorCode) {
             iterator.disable_recursion_pending();
             utils::LogWarn(DescribeSkippedEntry(entryPath, errorCode));
-            IncrementIterator(iterator, end);
+            IncrementIterator(iterator, end, entryPath);
             continue;
         }
 
         if (fs::is_symlink(status)) {
             iterator.disable_recursion_pending();
-            utils::LogWarn("Skipping symlink: " + entryPath.string());
-            IncrementIterator(iterator, end);
+            utils::LogWarn(DescribeSymlinkEntry(*iterator));
+            IncrementIterator(iterator, end, entryPath);
             continue;
         }
 
         if (fs::is_directory(status)) {
-            IncrementIterator(iterator, end);
+            IncrementIterator(iterator, end, entryPath);
             continue;
         }
 
         if (fs::is_regular_file(status)) {
             files.push_back(utils::NormalizePath(entryPath));
-            IncrementIterator(iterator, end);
+            IncrementIterator(iterator, end, entryPath);
             continue;
         }
 
         utils::LogWarn(DescribeUnsupportedEntry(entryPath));
-        IncrementIterator(iterator, end);
+        IncrementIterator(iterator, end, entryPath);
     }
 
-    std::sort(files.begin(), files.end(), [](const fs::path& left, const fs::path& right) {
+    std::ranges::sort(files, [](const fs::path& left, const fs::path& right) {
         return left.generic_string() < right.generic_string();
     });
 
@@ -93,15 +103,14 @@ AddCommand::AddCommand(core::Registry& registry) : Registry_(registry) {}
 bool AddCommand::AddFileEntry(const fs::path& normalizedPath) const {
     const auto storedRelativePath = utils::MakeStorageRelativePath(normalizedPath);
     if (storedRelativePath.empty()) {
-        throw std::logic_error{"Unable to derive a storage path for: " + normalizedPath.string()};
+        throw CommandError{"Unable to derive a storage path for: " + normalizedPath.string()};
     }
 
-    const auto added = Registry_.AddEntry({
-        .OriginalPath = normalizedPath.string(),
-        .StoredRelativePath = storedRelativePath.generic_string(),
-    });
-
-    if (!added) {
+    if (const auto added = Registry_.AddEntry({
+            .OriginalPath = normalizedPath.string(),
+            .StoredRelativePath = storedRelativePath.generic_string(),
+        });
+        !added) {
         utils::LogInfo("File is already tracked: " + normalizedPath.string());
         return false;
     }
@@ -126,9 +135,12 @@ void AddCommand::ExecuteDirectory(const fs::path& normalizedPath) const {
     }
 
     auto addedCount = 0U;
+    auto duplicateCount = 0U;
     for (const auto& filePath : files) {
         if (AddFileEntry(filePath)) {
             ++addedCount;
+        } else {
+            ++duplicateCount;
         }
     }
 
@@ -138,7 +150,11 @@ void AddCommand::ExecuteDirectory(const fs::path& normalizedPath) const {
     }
 
     Registry_.Save();
-    utils::LogInfo(std::format("Imported {} file(s) from directory: {}", addedCount, normalizedPath.string()));
+    auto summary = std::format("Imported {} file(s) from directory: {}", addedCount, normalizedPath.string());
+    if (duplicateCount > 0U) {
+        summary += std::format(" (skipped {} already tracked file(s))", duplicateCount);
+    }
+    utils::LogInfo(summary);
 }
 
 void AddCommand::Execute(const fs::path& path) const {

tests/AddCommandCliTests.cpp

@@ -78,6 +78,8 @@ TEST_F(AddCommandCliTest, AddDirectoryUsesActiveStorageRootAndWritesImportedFile
     const auto result = RunCommand("add " + cfgsync::tests::QuoteForCommand(directoryPath));
 
     EXPECT_EQ(result.ExitCode, 0);
+    EXPECT_NE(result.Output.find("Imported 2 file(s) from directory"), std::string::npos);
+    EXPECT_NE(result.Output.find(directoryPath.string()), std::string::npos);
     EXPECT_TRUE(result.Error.empty());
 
     const std::vector<fs::path> expectedPaths{
@@ -109,10 +111,37 @@ TEST_F(AddCommandCliTest, RepeatedDirectoryAddSucceedsAndLeavesRegistryUnchanged
     const auto result = RunCommand("add " + cfgsync::tests::QuoteForCommand(directoryPath));
 
     EXPECT_EQ(result.ExitCode, 0);
+    EXPECT_NE(result.Output.find("No new files to track under directory"), std::string::npos);
+    EXPECT_NE(result.Output.find(directoryPath.string()), std::string::npos);
     EXPECT_TRUE(result.Error.empty());
     EXPECT_EQ(ReadJsonFile(StorageRoot() / "registry.json"), documentAfterFirstAdd);
 }
 
+TEST_F(AddCommandCliTest, DirectoryAddReportsSkippedAlreadyTrackedFilesWhenImportingNewFiles) {
+    const auto directoryPath = GetTestRoot() / "configs";
+    const auto existingPath = directoryPath / ".gitconfig";
+    const auto newPath = directoryPath / "nvim" / "init.lua";
+    WriteTextFile(existingPath, "[user]\n");
+    WriteTextFile(newPath, "vim.opt.number = true\n");
+
+    ASSERT_TRUE(cfgsync::tests::CfgsyncCommandSucceeded(
+        "init --storage " + cfgsync::tests::QuoteForCommand(StorageRoot()), GetTestRoot()));
+    ASSERT_TRUE(
+        cfgsync::tests::CfgsyncCommandSucceeded("add " + cfgsync::tests::QuoteForCommand(existingPath), GetTestRoot()));
+
+    const auto result = RunCommand("add " + cfgsync::tests::QuoteForCommand(directoryPath));
+
+    EXPECT_EQ(result.ExitCode, 0);
+    EXPECT_NE(result.Output.find("Imported 1 file(s) from directory"), std::string::npos);
+    EXPECT_NE(result.Output.find("skipped 1 already tracked file(s)"), std::string::npos);
+    EXPECT_TRUE(result.Error.empty());
+
+    const auto document = ReadJsonFile(StorageRoot() / "registry.json");
+    ASSERT_EQ(document["tracked_files"].size(), 2U);
+    EXPECT_EQ(document["tracked_files"][0]["original_path"], cfgsync::utils::NormalizePath(existingPath).string());
+    EXPECT_EQ(document["tracked_files"][1]["original_path"], cfgsync::utils::NormalizePath(newPath).string());
+}
+
 }  // namespace
 
 int main(int argc, char** argv) {

tests/AddCommandTests.cpp

@@ -11,6 +11,10 @@
 #include <string>
 #include <vector>
 
+#ifndef _WIN32
+#include <sys/stat.h>
+#endif
+
 namespace {
 namespace fs = std::filesystem;
 
@@ -134,6 +138,29 @@ TEST_F(AddCommandTest, DirectoryImportSkipsSymlinksAndKeepsImportingOrdinaryFile
     EXPECT_EQ(Registry().GetTrackedEntries()[0].OriginalPath, cfgsync::utils::NormalizePath(sourcePath).string());
 }
 
+TEST_F(AddCommandTest, DirectoryImportSkipsSymlinkedDirectoriesWithoutImportingTheirContents) {
+    const auto directoryPath = SourcePath().parent_path();
+    const auto sourcePath = directoryPath / ".gitconfig";
+    const auto linkedDirectoryTarget = StorageRoot() / "external-configs";
+    const auto linkedFilePath = linkedDirectoryTarget / "secret.conf";
+    const auto symlinkPath = directoryPath / "linked-configs";
+    WriteTextFile(sourcePath, "[user]\n");
+    WriteTextFile(linkedFilePath, "secret\n");
+
+    std::error_code errorCode;
+    fs::create_directory_symlink(linkedDirectoryTarget, symlinkPath, errorCode);
+    if (errorCode) {
+        GTEST_SKIP() << "Directory symlink creation is not available in this test environment.";
+    }
+
+    cfgsync::commands::AddCommand command{Registry()};
+
+    command.Execute(directoryPath);
+
+    ASSERT_EQ(Registry().GetTrackedEntries().size(), 1U);
+    EXPECT_EQ(Registry().GetTrackedEntries()[0].OriginalPath, cfgsync::utils::NormalizePath(sourcePath).string());
+}
+
 TEST_F(AddCommandTest, EmptyDirectoryImportSucceedsWithoutChangingRegistry) {
     const auto directoryPath = SourcePath().parent_path();
     cfgsync::utils::EnsureDirectoryExists(directoryPath);
@@ -183,6 +210,27 @@ TEST_F(AddCommandTest, DirectoryImportContinuesAfterUnreadableSubdirectoryWhenPe
 }
 #endif
 
+#ifndef _WIN32
+TEST_F(AddCommandTest, DirectoryImportSkipsSpecialFilesAndKeepsImportingOrdinaryFiles) {
+    const auto directoryPath = SourcePath().parent_path();
+    const auto sourcePath = directoryPath / ".gitconfig";
+    const auto fifoPath = directoryPath / "events.fifo";
+    WriteTextFile(sourcePath, "[user]\n");
+
+    const auto result = mkfifo(fifoPath.c_str(), S_IRUSR | S_IWUSR);
+    if (result != 0) {
+        GTEST_SKIP() << "Unable to create a FIFO in this test environment.";
+    }
+
+    cfgsync::commands::AddCommand command{Registry()};
+
+    command.Execute(directoryPath);
+
+    ASSERT_EQ(Registry().GetTrackedEntries().size(), 1U);
+    EXPECT_EQ(Registry().GetTrackedEntries()[0].OriginalPath, cfgsync::utils::NormalizePath(sourcePath).string());
+}
+#endif
+
 TEST_F(AddCommandTest, MissingFileFailsClearly) {
     cfgsync::utils::EnsureDirectoryExists(SourcePath().parent_path());
     cfgsync::commands::AddCommand command{Registry()};