From 20acf6ba05c2e10cc01c5f9dd012fe7d7ad575cf Mon Sep 17 00:00:00 2001
From: John Ericson <git@JohnEricson.me>
Date: Wed, 7 Aug 2024 18:07:26 -0400
Subject: [PATCH] Apply suggestions from code review

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
---
 src/libutil/executable-path.cc |  1 +
 src/libutil/executable-path.hh |  2 +-
 src/libutil/file-system.cc     |  3 ++-
 tests/unit/libutil/strings.cc  | 14 ++------------
 4 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/src/libutil/executable-path.cc b/src/libutil/executable-path.cc
index a976cd7ebd53..1658e36676d0 100644
--- 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
index 16a8eb9688ce..bcb8d28e8e6f 100644
--- 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
index 20d41b49597b..8d9f22084d61 100644
--- 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
index cbb8f13bdf90..61174f02627f 100644
--- 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