From e0b87690877a97387e1de7b327a3c4fe255a2f40 Mon Sep 17 00:00:00 2001
From: Chip Senkbeil <chipsenkbeil@meta.com>
Date: Thu, 6 Jul 2023 18:29:36 -0500
Subject: [PATCH] Fix return code of --help and --version on cli

---
 CHANGELOG.md   |  5 +++++
 src/cli.rs     |  6 +++---
 src/lib.rs     | 26 +++++++++++++++++++++++++-
 src/options.rs | 16 +++++++++++++---
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7a76e3e..3c32a37 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   requirement. This technically applied to v0.20.0-alpha.11, but wasn't caught
   until the dependency updated
 
+### Fixed
+
+- `distant --help` will now return exit code of 0
+- `distant --version` will now return exit code of 0
+
 ## [0.20.0-alpha.11]
 
 ### Added
diff --git a/src/cli.rs b/src/cli.rs
index d9c0868..5456c77 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -1,6 +1,6 @@
 use std::ffi::OsString;
 
-use crate::options::DistantSubcommand;
+use crate::options::{DistantSubcommand, OptionsError};
 use crate::{CliResult, Options};
 
 mod commands;
@@ -18,12 +18,12 @@ pub struct Cli {
 
 impl Cli {
     /// Creates a new CLI instance by parsing command-line arguments
-    pub fn initialize() -> anyhow::Result<Self> {
+    pub fn initialize() -> Result<Self, OptionsError> {
         Self::initialize_from(std::env::args_os())
     }
 
     /// Creates a new CLI instance by parsing providing arguments
-    pub fn initialize_from<I, T>(args: I) -> anyhow::Result<Self>
+    pub fn initialize_from<I, T>(args: I) -> Result<Self, OptionsError>
     where
         I: IntoIterator<Item = T>,
         T: Into<OsString> + Clone,
diff --git a/src/lib.rs b/src/lib.rs
index ad5846d..95af545 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -6,6 +6,7 @@ pub struct ReadmeDoctests;
 
 use std::process::{ExitCode, Termination};
 
+use clap::error::ErrorKind;
 use derive_more::{Display, Error, From};
 
 mod cli;
@@ -16,7 +17,7 @@ mod options;
 pub mod win_service;
 
 pub use cli::Cli;
-pub use options::Options;
+pub use options::{Options, OptionsError};
 
 /// Wrapper around a [`CliResult`] that provides [`Termination`] support
 pub struct MainResult(CliResult);
@@ -31,6 +32,29 @@ impl From<CliResult> for MainResult {
     }
 }
 
+impl From<OptionsError> for MainResult {
+    fn from(x: OptionsError) -> Self {
+        Self(match x {
+            OptionsError::Config(x) => Err(CliError::Error(x)),
+            OptionsError::Options(x) => match x.kind() {
+                // --help and --version should not actually exit with an error and instead display
+                // their related information while succeeding
+                ErrorKind::DisplayHelp | ErrorKind::DisplayVersion => {
+                    // NOTE: We're causing a side effect here in constructing the main result,
+                    //       but seems cleaner than returning an error with an exit code of 0
+                    //       and a message to try to print. Plus, we leverage automatic color
+                    //       handling in this approach.
+                    let _ = x.print();
+                    Ok(())
+                }
+
+                // Everything else is an actual error and should fail
+                _ => Err(CliError::Error(anyhow::anyhow!(x))),
+            },
+        })
+    }
+}
+
 impl From<anyhow::Error> for MainResult {
     fn from(x: anyhow::Error) -> Self {
         Self(Err(CliError::Error(x)))
diff --git a/src/options.rs b/src/options.rs
index 700bfc7..5b9163f 100644
--- a/src/options.rs
+++ b/src/options.rs
@@ -4,7 +4,7 @@ use std::path::{Path, PathBuf};
 use clap::builder::TypedValueParser as _;
 use clap::{Args, Parser, Subcommand, ValueEnum, ValueHint};
 use clap_complete::Shell as ClapCompleteShell;
-use derive_more::IsVariant;
+use derive_more::{Display, Error, From, IsVariant};
 use distant_core::net::common::{ConnectionId, Destination, Map, PortRange};
 use distant_core::net::server::Shutdown;
 use distant_core::protocol::ChangeKind;
@@ -36,14 +36,24 @@ pub struct Options {
     pub command: DistantSubcommand,
 }
 
+/// Represents an error associated with parsing options.
+#[derive(Debug, Display, From, Error)]
+pub enum OptionsError {
+    // When configuration file fails to load
+    Config(#[error(not(source))] anyhow::Error),
+
+    // When parsing options fails (or is something like --version or --help)
+    Options(#[error(not(source))] clap::Error),
+}
+
 impl Options {
     /// Creates a new CLI instance by parsing command-line arguments
-    pub fn load() -> anyhow::Result<Self> {
+    pub fn load() -> Result<Self, OptionsError> {
         Self::load_from(std::env::args_os())
     }
 
     /// Creates a new CLI instance by parsing providing arguments
-    pub fn load_from<I, T>(args: I) -> anyhow::Result<Self>
+    pub fn load_from<I, T>(args: I) -> Result<Self, OptionsError>
     where
         I: IntoIterator<Item = T>,
         T: Into<OsString> + Clone,