[main] rootcp: error out when trying to copy inexisting objects

Author: silverweed

main/src/RootObjTree.cxx

@@ -35,9 +35,12 @@ ROOT::CmdLine::GetMatchingPathsInFile(std::string_view fileName, std::string_vie
    }
 
    const auto patternSplits = pattern.empty() ? std::vector<std::string>{} : ROOT::Split(pattern, "/");
+   std::vector<bool> patternWasMatchedAtLeastOnce(patternSplits.size());
 
-   // Match all objects at all nesting levels down to the deepest nesting level of `pattern` (or all nesting levels
-   // if we have the "recursive listing" flag). The nodes are visited breadth-first.
+   /// Match all objects at all nesting levels down to the deepest nesting level of `pattern` (or all nesting levels
+   /// if we have the "recursive listing" flag). The nodes are visited breadth-first.
+
+   // Initialize the nodeTree with the root node and mark it as the first node to visit.
    {
       ROOT::CmdLine::RootObjNode rootNode = {};
       rootNode.fName = std::string(fileName);
@@ -57,7 +60,7 @@ ROOT::CmdLine::GetMatchingPathsInFile(std::string_view fileName, std::string_vie
       ROOT::CmdLine::RootObjNode *cur = &nodeTree.fNodes[curIdx];
       assert(cur->fDir);
 
-      // Sort the keys by name
+      // Gather all keys under this directory and sort them by name.
       std::vector<TKey *> keys;
       keys.reserve(cur->fDir->GetListOfKeys()->GetEntries());
       for (TKey *key : ROOT::Detail::TRangeStaticCast<TKey>(cur->fDir->GetListOfKeys()))
@@ -68,10 +71,19 @@ ROOT::CmdLine::GetMatchingPathsInFile(std::string_view fileName, std::string_vie
 
       namesFound.clear();
 
+      // Iterate the keys and find matches
       for (TKey *key : keys) {
-         // Don't recurse lower than requested by `pattern` unless we explicitly have the `recursive listing` flag.
-         if (cur->fNesting < patternSplits.size() && !MatchesGlob(key->GetName(), patternSplits[cur->fNesting]))
-            continue;
+         // NOTE: cur->fNesting can only be >= patternSplits.size() if we have `isRecursive == true` (see the code near
+         // the end of the outer do/while loop).
+         // In that case we don't care about matching patterns anymore because we are already beyond the nesting level
+         // where pattern filtering applies.
+         // In all other cases, we check if the key name matches the pattern and skip it if it doesn't.
+         if (cur->fNesting < patternSplits.size()) {
+            if (MatchesGlob(key->GetName(), patternSplits[cur->fNesting]))
+               patternWasMatchedAtLeastOnce[cur->fNesting] = true;
+            else
+               continue;
+         }
 
          if (namesFound.count(key->GetName()) > 0) {
             std::cerr << "WARNING: Several versions of '" << key->GetName() << "' are present in '" << fileName
@@ -94,7 +106,8 @@ ROOT::CmdLine::GetMatchingPathsInFile(std::string_view fileName, std::string_vie
             newChild.fDir = cur->fDir->GetDirectory(key->GetName());
       }
 
-      // Only recurse into subdirectories that are up to the deepest level we ask for through `pattern`.
+      // Only recurse into subdirectories that are up to the deepest level we ask for through `pattern` (except in
+      // case of recursive listing).
       if (cur->fNesting < patternSplits.size() || isRecursive) {
          for (auto childIdx = cur->fFirstChild; childIdx < cur->fFirstChild + cur->fNChildren; ++childIdx) {
             auto &child = nodeTree.fNodes[childIdx];
@@ -112,6 +125,19 @@ ROOT::CmdLine::GetMatchingPathsInFile(std::string_view fileName, std::string_vie
       }
    } while (!nodesToVisit.empty());
 
+   if (!(flags & kIgnoreFailedMatches)) {
+      for (auto i = 0u; i < patternSplits.size(); ++i) {
+         // We don't append errors for '*' because its semantics imply "0 or more matches", so 0 matches is a valid
+         // case. For any other pattern we expect at least 1 match.
+         if (!patternWasMatchedAtLeastOnce[i] && !patternSplits[i].empty() && patternSplits[i] != "*") {
+            std::string err = "'" + std::string(fileName) + ":" +
+                              ROOT::Join("/", std::span<const std::string>{patternSplits.data(), i + 1}) +
+                              "' matches no objects.";
+            source.fErrors.push_back(err);
+         }
+      }
+   }
+
    return source;
 }
 

main/src/RootObjTree.hxx

@@ -103,9 +103,12 @@ struct RootSource {
 enum EGetMatchingPathsFlags {
    /// Recurse into subdirectories when matching objects
    kRecursive = 1 << 0,
+   kIgnoreFailedMatches = 1 << 1,
 };
 
 /// Given a file and a "path pattern", returns a RootSource containing the tree of matched objects.
+/// If `pattern` is non-empty, not a single '*' and it doesn't match any object in `fileName`,
+/// an error will be appended to RootSource::fErrors unless `kIgnoreFailedMatches` is part of `flags`.
 ///
 /// \param fileName The name of the ROOT file to look into
 /// \param pattern A glob-like pattern (basically a `ls` pattern). May be empty to match anything.

main/src/rootcp.cxx

@@ -323,16 +323,34 @@ int main(int argc, char **argv)
    // a valid location.
    // First validate the destination syntax.
    const auto destFnameAndPattern = args.fSources.back();
+   args.fSources.pop_back();
    auto splitRes = SplitIntoFileNameAndPattern(destFnameAndPattern);
    if (!splitRes) {
       Err() << splitRes.GetError()->GetReport() << "\n";
       return 1;
    }
    auto [destFname, destPath] = splitRes.Unwrap();
 
+   // Validate and split all input sources into filename + pattern
+   std::vector<std::pair<std::string_view, std::string_view>> sourcesFileAndPattern;
+   sourcesFileAndPattern.reserve(args.fSources.size());
+   bool srcIsSameAsDstFile = false;
+   for (const auto &src : args.fSources) {
+      auto res = SplitIntoFileNameAndPattern(src);
+      if (!res) {
+         Err() << res.GetError()->GetReport() << "\n";
+         return 1;
+      }
+      auto fNameAndPattern = res.Unwrap();
+      if (fNameAndPattern.first == destFname) {
+         srcIsSameAsDstFile = true;
+      }
+      sourcesFileAndPattern.push_back(fNameAndPattern);
+   }
+
    // Check if the operation is allowed.
-   args.fSources.pop_back();
-   if (args.fRecreate && std::find(args.fSources.begin(), args.fSources.end(), destFname) != args.fSources.end()) {
+
+   if (args.fRecreate && srcIsSameAsDstFile) {
       Err() << "cannot recreate destination file if this is also a source file\n";
       return 1;
    }
@@ -382,8 +400,8 @@ int main(int argc, char **argv)
 
    const std::uint32_t flags = args.fRecursive * EGetMatchingPathsFlags::kRecursive;
    bool errors = false;
-   for (const auto &srcFname : args.fSources) {
-      auto src = ROOT::CmdLine::ParseRootSource(srcFname, flags);
+   for (const auto &[srcFname, srcPattern] : sourcesFileAndPattern) {
+      auto src = ROOT::CmdLine::GetMatchingPathsInFile(srcFname, srcPattern, flags);
       if (!src.fErrors.empty()) {
          for (const auto &err : src.fErrors)
             Err() << err << "\n";
@@ -425,8 +443,8 @@ int main(int argc, char **argv)
       }
    }
 
-   if (errors) {
-      // Make sure we don't end up with a half-copied file in case of errors.
+   if (errors && !srcIsSameAsDstFile) {
+      // If the destination file was fresh, make sure we don't end up with a half-copied file in case of errors.
       gSystem->Unlink(std::string(destFname).c_str());
    }
 

roottest/main/CMakeLists.txt

@@ -399,6 +399,12 @@ ROOTTEST_ADD_TEST(RootcpReplaceRecursiveCheckOutput
 ROOTTEST_ADD_TEST(RootcpReplaceRecursiveClean
                   COMMAND ${PY_TOOLS_PREFIX}/rootrm${pyext} -r copy_replace_recursive.root
                   FIXTURES_REQUIRED main-RootcpReplaceRecursiveCheckOutput-fixture)
+
+
+ROOTTEST_ADD_TEST(SimpleRootcpInexistent
+                  COMMAND ${TOOLS_PREFIX}/rootcp${exeext} test.root:does_not_exist test.root:
+                  PASSREGEX "Error in <rootcp>: 'test.root:does_not_exist' matches no objects.")
+
 #########################################################################
 
 ############################# ROOMV TESTS ############################