refactor(pager): make pager somewhat testable

include/dwarfs/os_access.h

@@ -24,6 +24,7 @@
 #include <chrono>
 #include <filesystem>
 #include <memory>
+#include <optional>
 #include <span>
 #include <string>
 #include <system_error>
@@ -65,5 +66,7 @@ class os_access {
                       std::error_code& ec) const = 0;
   virtual std::chrono::nanoseconds
   thread_get_cpu_time(std::thread::id tid, std::error_code& ec) const = 0;
+  virtual std::filesystem::path
+  find_executable(std::filesystem::path const& name) const = 0;
 };
 } // namespace dwarfs

include/dwarfs/os_access_generic.h

@@ -53,5 +53,7 @@ class os_access_generic : public os_access {
                            std::error_code& ec) const override;
   std::chrono::nanoseconds
   thread_get_cpu_time(std::thread::id tid, std::error_code& ec) const override;
+  std::filesystem::path
+  find_executable(std::filesystem::path const& name) const override;
 };
 } // namespace dwarfs

include/dwarfs/pager.h

@@ -21,10 +21,21 @@
 
 #pragma once
 
+#include <filesystem>
+#include <optional>
 #include <string>
+#include <vector>
 
 namespace dwarfs {
 
-bool show_in_pager(std::string text);
+class os_access;
+
+struct pager_program {
+  std::filesystem::path name;
+  std::vector<std::string> args;
+};
+
+std::optional<pager_program> find_pager_program(os_access const& os);
+void show_in_pager(pager_program const& pager, std::string text);
 
 } // namespace dwarfs

src/dwarfs/os_access_generic.cpp

@@ -25,6 +25,8 @@
 #include <folly/portability/PThread.h>
 #include <folly/portability/Unistd.h>
 
+#include <boost/process/search_path.hpp>
+
 #include "dwarfs/mmap.h"
 #include "dwarfs/os_access_generic.h"
 #include "dwarfs/util.h"
@@ -193,4 +195,9 @@ os_access_generic::thread_get_cpu_time(std::thread::id tid,
 #endif
 }
 
+std::filesystem::path
+os_access_generic::find_executable(std::filesystem::path const& name) const {
+  return boost::process::search_path(name.wstring()).wstring();
+}
+
 } // namespace dwarfs

src/dwarfs/pager.cpp

@@ -26,6 +26,9 @@
 #include <boost/asio/io_service.hpp>
 #include <boost/process.hpp>
 
+#include <folly/portability/Unistd.h>
+
+#include "dwarfs/os_access.h"
 #include "dwarfs/pager.h"
 
 namespace dwarfs {
@@ -34,63 +37,53 @@ namespace {
 
 namespace bp = boost::process;
 
-struct pager_def {
+std::vector<pager_program> const pagers{
-  std::string name;
-  std::vector<std::string> args;
-};
-
-std::vector<pager_def> const pagers{
     {"less", {"-R"}},
 };
 
-auto find_executable(std::string name) { return bp::search_path(name); }
+} // namespace
+
+std::optional<pager_program> find_pager_program(os_access const& os) {
+  if (auto pager_env = os.getenv("PAGER")) {
+    std::string_view sv{pager_env.value()};
+
+    if (sv == "cat") {
+      return std::nullopt;
+    }
 
-std::pair<boost::filesystem::path, std::vector<std::string>> find_pager() {
-  if (auto pager_env = std::getenv("PAGER")) {
-    std::string_view sv(pager_env);
     if (sv.starts_with('"') && sv.ends_with('"')) {
       sv.remove_prefix(1);
       sv.remove_suffix(1);
     }
-    if (sv == "cat") {
+
-      return {};
+    std::filesystem::path p{std::string(sv)};
+
+    if (os.access(p, X_OK) == 0) {
+      return pager_program{p, {}};
     }
-    boost::filesystem::path p{std::string(sv)};
+
-    if (boost::filesystem::exists(p)) {
+    if (auto exe = os.find_executable(p); !exe.empty()) {
-      return {p.string(), {}};
+      return pager_program{exe, {}};
-    }
-    if (auto exe = find_executable(pager_env); !exe.empty()) {
-      return {exe, {}};
     }
   }
 
   for (auto const& p : pagers) {
-    if (auto exe = find_executable(std::string(p.name)); !exe.empty()) {
+    if (auto exe = os.find_executable(p.name); !exe.empty()) {
-      return {exe, p.args};
+      return pager_program{exe, p.args};
     }
   }
 
-  return {};
+  return std::nullopt;
 }
 
-} // namespace
+void show_in_pager(pager_program const& pager, std::string text) {
-
-bool show_in_pager(std::string text) {
-  auto [pager_exe, pager_args] = find_pager();
-
-  if (pager_exe.empty()) {
-    return false;
-  }
-
   boost::asio::io_service ios;
-  bp::child proc(pager_exe, bp::args(pager_args),
+  bp::child proc(pager.name.wstring(), bp::args(pager.args),
                  bp::std_in =
                      boost::asio::const_buffer(text.data(), text.size()),
                  bp::std_out > stdout, ios);
   ios.run();
   proc.wait();
-
-  return true;
 }
 
 } // namespace dwarfs

src/dwarfs/tool.cpp

@@ -91,11 +91,18 @@ void add_common_options(po::options_description& opts,
 #ifdef DWARFS_BUILTIN_MANPAGE
 void show_manpage(manpage::document doc, iolayer const& iol) {
   bool is_tty = iol.term->is_tty(iol.out);
+
   auto content =
       render_manpage(doc, iol.term->width(), is_tty && iol.term->is_fancy());
-  if (!is_tty || !show_in_pager(content)) {
+
-    iol.out << content;
+  if (is_tty) {
+    if (auto pager = find_pager_program(*iol.os)) {
+      show_in_pager(*pager, content);
+      return;
+    }
   }
+
+  iol.out << content;
 }
 #endif
 

test/manpage_test.cpp

@@ -25,8 +25,11 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include "dwarfs/pager.h"
 #include "dwarfs/render_manpage.h"
 
+#include "test_helpers.h"
+
 using namespace dwarfs;
 
 namespace {
@@ -62,3 +65,92 @@ INSTANTIATE_TEST_SUITE_P(
     ::testing::Combine(::testing::Values("mkdwarfs", "dwarfs", "dwarfsck",
                                          "dwarfsextract"),
                        ::testing::Bool()));
+
+TEST(pager_test, find_pager_program) {
+  auto resolver = [](std::filesystem::path const& name) {
+    std::map<std::string, std::filesystem::path> const programs = {
+        {"less", "/whatever/bin/less"},
+        {"more", "/somewhere/bin/more"},
+        {"cat", "/bin/cat"},
+    };
+    for (auto const& [n, p] : programs) {
+      if (name == n || name == p) {
+        return p;
+      }
+    }
+    return std::filesystem::path{};
+  };
+
+  {
+    test::os_access_mock os;
+    os.set_executable_resolver(
+        [](std::filesystem::path const&) { return std::filesystem::path{}; });
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_FALSE(pager);
+    }
+
+    os.set_executable_resolver(resolver);
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_TRUE(pager);
+      EXPECT_EQ("/whatever/bin/less", pager->name);
+      EXPECT_EQ(std::vector<std::string>({"-R"}), pager->args);
+    }
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_TRUE(pager);
+      EXPECT_EQ("/whatever/bin/less", pager->name);
+      EXPECT_EQ(std::vector<std::string>({"-R"}), pager->args);
+    }
+
+    os.set_access_fail("more");
+    os.set_access_fail("less");
+
+    os.setenv("PAGER", "more");
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_TRUE(pager);
+      EXPECT_EQ("/somewhere/bin/more", pager->name);
+      EXPECT_TRUE(pager->args.empty());
+    }
+
+    os.setenv("PAGER", "less");
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_TRUE(pager);
+      EXPECT_EQ("/whatever/bin/less", pager->name);
+      EXPECT_TRUE(pager->args.empty());
+    }
+
+    os.setenv("PAGER", "cat");
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_FALSE(pager);
+    }
+
+    os.setenv("PAGER", "/bla/foo");
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_TRUE(pager);
+      EXPECT_EQ("/bla/foo", pager->name);
+      EXPECT_TRUE(pager->args.empty());
+    }
+
+    os.setenv("PAGER", R"("/bla/foo")");
+
+    {
+      auto pager = find_pager_program(os);
+      ASSERT_TRUE(pager);
+      EXPECT_EQ("/bla/foo", pager->name);
+      EXPECT_TRUE(pager->args.empty());
+    }
+  }
+}

test/test_helpers.cpp

@@ -516,20 +516,25 @@ os_access_mock::thread_get_cpu_time(std::thread::id tid,
   return real_os_->thread_get_cpu_time(tid, ec);
 }
 
-std::optional<fs::path> find_binary(std::string_view name) {
+std::filesystem::path
-  auto path_str = std::getenv("PATH");
+os_access_mock::find_executable(std::filesystem::path const& name) const {
-  if (!path_str) {
+  if (executable_resolver_) {
-    return std::nullopt;
+    return executable_resolver_(name);
   }
 
-  std::vector<std::string> path;
+  return real_os_->find_executable(name);
-  folly::split(':', path_str, path);
+}
 
-  for (auto dir : path) {
+void os_access_mock::set_executable_resolver(
-    auto cand = fs::path(dir) / name;
+    executable_resolver_type resolver) {
-    if (fs::exists(cand) and ::access(cand.string().c_str(), X_OK) == 0) {
+  executable_resolver_ = std::move(resolver);
-      return cand;
+}
-    }
+
+std::optional<fs::path> find_binary(std::string_view name) {
+  os_access_generic os;
+
+  if (auto path = os.find_executable(name); !path.empty()) {
+    return path;
   }
 
   return std::nullopt;

test/test_helpers.h

@@ -73,6 +73,9 @@ class os_access_mock : public os_access {
       std::variant<std::monostate, std::string, std::function<std::string()>,
                    std::unique_ptr<mock_directory>>;
 
+  using executable_resolver_type =
+      std::function<std::filesystem::path(std::filesystem::path const&)>;
+
   os_access_mock();
   ~os_access_mock();
 
@@ -126,6 +129,11 @@ class os_access_mock : public os_access {
   std::chrono::nanoseconds
   thread_get_cpu_time(std::thread::id tid, std::error_code& ec) const override;
 
+  std::filesystem::path
+  find_executable(std::filesystem::path const& name) const override;
+
+  void set_executable_resolver(executable_resolver_type resolver);
+
   std::set<std::filesystem::path> get_failed_paths() const;
 
   std::vector<
@@ -150,6 +158,7 @@ class os_access_mock : public os_access {
   std::map<std::filesystem::path, error_info> map_file_errors_;
   std::map<std::string, std::string> env_;
   std::shared_ptr<os_access> real_os_;
+  executable_resolver_type executable_resolver_;
 };
 
 class script_mock : public script {