Apply suggestions from code review

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
obsidiansystems · Aug 7, 2024 · 20acf6b · 20acf6b
1 parent 1216778
commit 20acf6b
Show file tree

Hide file tree

Showing 4 changed files with 6 additions and 14 deletions.
diff --git a/src/libutil/executable-path.cc b/src/libutil/executable-path.cc
@@ -42,6 +42,7 @@ ExecutablePath ExecutablePath::parse(const OsString & path)
                     // ("::"), as an initial <colon> preceding the rest
                     // of the list, or as a trailing <colon> following
                     // the rest of the list."
+                    // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
                     ? OS_STR(".")
                     : std::move(str),
             };

diff --git a/src/libutil/executable-path.hh b/src/libutil/executable-path.hh
@@ -56,7 +56,7 @@ struct ExecutablePath
      */
     std::optional<std::filesystem::path> find(
         const OsString & exe,
-        std::function<bool(const std::filesystem::path &)> isExecutable = isExecutableAmbient) const;
+        std::function<bool(const std::filesystem::path &)> isExecutableFile = isExecutableFileAmbient) const;
 
     bool operator==(const ExecutablePath &) const = default;
 };

diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc
@@ -685,7 +685,8 @@ void moveFile(const Path & oldName, const Path & newName)
 
 //////////////////////////////////////////////////////////////////////
 
-bool isExecutableAmbient(const fs::path & exe) {
+bool isExecutableFileAmbient(const fs::path & exe) {
+    // FIXME: access probably returns the wrong thing on directories
     return access(exe.string().c_str(),
 #ifdef WIN32
         0 // TODO do better

diff --git a/tests/unit/libutil/strings.cc b/tests/unit/libutil/strings.cc
@@ -366,18 +366,8 @@ std::string concatStringsSep2(const std::string_view sep, const C & ss)
 // concatStringsSep sep . splitString sep = id   if sep is 1 char
 RC_GTEST_PROP(splitString, recoveredByConcatStringsSep, (const std::string & s))
 {
-    if (!s.empty()) {
-        // See FIXME in concatStringsSep tests
-        // RC_ASSERT(
-        //     concatStringsSep(std::string(1, s[0]), splitString<Strings>(s, std::string(1, s[0]))) == s
-        // );
-
-        // Work around the bug. Make sure it doesn't start with the separate, because that's broken.
-        RC_ASSERT(concatStringsSep2(std::string(1, s[0]), splitString<Strings>(s, std::string(1, s[0]))) == s);
-    }
-
-    RC_ASSERT(concatStringsSep2("/", splitString<Strings>(s, "/")) == s);
-    RC_ASSERT(concatStringsSep2("a", splitString<Strings>(s, "a")) == s);
+    RC_ASSERT(concatStringsSep("/", splitString<Strings>(s, "/")) == s);
+    RC_ASSERT(concatStringsSep("a", splitString<Strings>(s, "a")) == s);
 }
 
 } // namespace nix