Repository: tonarino/innernet (language: rust)

Issue:
Replace ChangeString with an enum
The [ChangeString](https://github.com/tonarino/innernet/blob/ae96e05e903a9946f9da4579baac11fef7ad9ecf/shared/src/types.rs#L523-L527) type is mainly just used for presentation to the user in `verbose` mode to explain which properties of a peer changed.

It currently uses Strings, but since there are a finite number of properties that could change, and we know what they all are, might as well use an enum.

I'd like them to be enums as well because I want to detect certain changes and suppress/modify the output.

If a peer is not reachable but the server has reported endpoint candidates, `innernet fetch` will claim the peer is "modified".

I would prefer to not print "modified" but instead something along the lines of "attempting to connect to peer X". And this should only be printed if NAT traversal is enabled.

Alternatively, if the actual list of endpoint candidates has changed then it would be good to explicitly mention that. Right now the condition is `!endpoint_changed && !new.candidates.is_empty()`.


Discussion / hints:

Fix patch:
diff --git a/client/src/util.rs b/client/src/util.rs
index 72112b1..cfc2ec8 100644
--- a/client/src/util.rs
+++ b/client/src/util.rs
@@ -3,7 +3,9 @@ use colored::*;
 use indoc::eprintdoc;
 use log::{Level, LevelFilter};
 use serde::{de::DeserializeOwned, Serialize};
-use shared::{interface_config::ServerInfo, Interface, PeerDiff, INNERNET_PUBKEY_HEADER};
+use shared::{
+    interface_config::ServerInfo, Interface, PeerChange, PeerDiff, INNERNET_PUBKEY_HEADER,
+};
 use std::{ffi::OsStr, io, path::Path, time::Duration};
 use ureq::{Agent, AgentBuilder};
 
@@ -137,13 +139,30 @@ pub fn permissions_helptext(config_dir: &Path, data_dir: &Path, e: &io::Error) {
     }
 }
 
+#[derive(Debug, Copy, Clone, PartialEq)]
+enum ChangeAction {
+    Added,
+    Modified,
+    Removed,
+}
+
+impl ChangeAction {
+    fn colored_output(&self) -> ColoredString {
+        match self {
+            Self::Added => "added".green(),
+            Self::Modified => "modified".yellow(),
+            Self::Removed => "removed".red(),
+        }
+    }
+}
+
 pub fn print_peer_diff(store: &DataStore, diff: &PeerDiff) {
     let public_key = diff.public_key().to_base64();
 
-    let text = match (diff.old, diff.new) {
-        (None, Some(_)) => "added".green(),
-        (Some(_), Some(_)) => "modified".yellow(),
-        (Some(_), None) => "removed".red(),
+    let change_action = match (diff.old, diff.new) {
+        (None, Some(_)) => ChangeAction::Added,
+        (Some(_), Some(_)) => ChangeAction::Modified,
+        (Some(_), None) => ChangeAction::Removed,
         _ => unreachable!("PeerDiff can't be None -> None"),
     };
 
@@ -158,15 +177,30 @@ pub fn print_peer_diff(store: &DataStore, diff: &PeerDiff) {
     };
     let peer_name = peer_hostname.as_deref().unwrap_or("[unknown]");
 
+    if change_action == ChangeAction::Modified
+        && diff
+            .changes()
+            .iter()
+            .all(|c| *c == PeerChange::NatTraverseReattempt)
+    {
+        // If this peer was "modified" but the only change is a NAT Traversal Reattempt,
+        // don't bother printing this peer.
+        return;
+    }
+
     log::info!(
         "  peer {} ({}...) was {}.",
         peer_name.yellow(),
         &public_key[..10].dimmed(),
-        text
+        change_action.colored_output(),
     );
 
     for change in diff.changes() {
-        log::debug!("    {}", change);
+        if let PeerChange::Endpoint { .. } = change {
+            log::info!("    {}", change);
+        } else {
+            log::debug!("    {}", change);
+        }
     }
 }
 
diff --git a/shared/src/types.rs b/shared/src/types.rs
index 7a33bc4..b1e398d 100644
--- a/shared/src/types.rs
+++ b/shared/src/types.rs
@@ -519,35 +519,55 @@ impl Display for Peer {
     }
 }
 
-#[derive(Clone, Debug, PartialEq, Eq)]
-pub struct ChangeString {
-    name: &'static str,
-    old: Option<String>,
-    new: Option<String>,
-}
-
-impl Display for ChangeString {
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum PeerChange {
+    AllowedIPs {
+        old: Vec<AllowedIp>,
+        new: Vec<AllowedIp>,
+    },
+    PersistentKeepalive {
+        old: Option<u16>,
+        new: Option<u16>,
+    },
+    Endpoint {
+        old: Option<SocketAddr>,
+        new: Option<SocketAddr>,
+    },
+    NatTraverseReattempt,
+}
+
+impl Display for PeerChange {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        write!(
-            f,
-            "{}: {} => {}",
-            self.name,
-            self.old.as_deref().unwrap_or("[none]"),
-            self.new.as_deref().unwrap_or("[none]")
-        )
+        match self {
+            Self::AllowedIPs { old, new } => write!(f, "Allowed IPs: {:?} => {:?}", old, new),
+            Self::PersistentKeepalive { old, new } => write!(
+                f,
+                "Persistent Keepalive: {} => {}",
+                old.display_string(),
+                new.display_string()
+            ),
+            Self::Endpoint { old, new } => write!(
+                f,
+                "Endpoint: {} => {}",
+                old.display_string(),
+                new.display_string()
+            ),
+            Self::NatTraverseReattempt => write!(f, "NAT Traversal Reattempt"),
+        }
     }
 }
 
-impl ChangeString {
-    pub fn new<T, U>(name: &'static str, old: Option<T>, new: Option<U>) -> Self
-    where
-        T: fmt::Debug,
-        U: fmt::Debug,
-    {
-        Self {
-            name,
-            old: old.map(|t| format!("{t:?}")),
-            new: new.map(|t| format!("{t:?}")),
+trait OptionExt {
+    fn display_string(&self) -> String;
+}
+
+impl<T: std::fmt::Debug> OptionExt for Option<T> {
+    fn display_string(&self) -> String {
+        match self {
+            Some(x) => {
+                format!("{:?}", x)
+            },
+            None => "[none]".to_string(),
         }
     }
 }
@@ -559,7 +579,7 @@ pub struct PeerDiff<'a> {
     pub old: Option<&'a PeerConfig>,
     pub new: Option<&'a Peer>,
     builder: PeerConfigBuilder,
-    changes: Vec<ChangeString>,
+    changes: Vec<PeerChange>,
 }
 
 impl<'a> PeerDiff<'a> {
@@ -588,14 +608,14 @@ impl<'a> PeerDiff<'a> {
         self.builder.public_key()
     }
 
-    pub fn changes(&self) -> &[ChangeString] {
+    pub fn changes(&self) -> &[PeerChange] {
         &self.changes
     }
 
     fn peer_config_builder(
         old_info: Option<&PeerInfo>,
         new: Option<&Peer>,
-    ) -> Option<(PeerConfigBuilder, Vec<ChangeString>)> {
+    ) -> Option<(PeerConfigBuilder, Vec<PeerChange>)> {
         let old = old_info.map(|p| &p.config);
         let public_key = match (old, new) {
             (Some(old), _) => old.public_key.clone(),
@@ -622,11 +642,10 @@ impl<'a> PeerDiff<'a> {
             builder = builder
                 .replace_allowed_ips()
                 .add_allowed_ips(new_allowed_ips);
-            changes.push(ChangeString::new(
-                "AllowedIPs",
-                old.map(|o| &o.allowed_ips[..]),
-                Some(&new_allowed_ips[0]),
-            ));
+            changes.push(PeerChange::AllowedIPs {
+                old: old.map(|o| o.allowed_ips.clone()).unwrap_or_else(Vec::new),
+                new: new_allowed_ips.to_vec(),
+            });
         }
 
         if old.is_none()
@@ -636,11 +655,10 @@ impl<'a> PeerDiff<'a> {
                 Some(interval) => builder.set_persistent_keepalive_interval(interval),
                 None => builder.unset_persistent_keepalive(),
             };
-            changes.push(ChangeString::new(
-                "PersistentKeepalive",
-                old.and_then(|p| p.persistent_keepalive_interval),
-                new.persistent_keepalive_interval,
-            ));
+            changes.push(PeerChange::PersistentKeepalive {
+                old: old.and_then(|p| p.persistent_keepalive_interval),
+                new: new.persistent_keepalive_interval,
+            });
         }
 
         // We won't update the endpoint if there's already a stable connection.
@@ -653,20 +671,15 @@ impl<'a> PeerDiff<'a> {
             if let Some(addr) = resolved {
                 if old.is_none() || matches!(old, Some(old) if old.endpoint != resolved) {
                     builder = builder.set_endpoint(addr);
-                    changes.push(ChangeString::new(
-                        "Endpoint",
-                        old.and_then(|p| p.endpoint),
-                        Some(addr),
-                    ));
+                    changes.push(PeerChange::Endpoint {
+                        old: old.and_then(|p| p.endpoint),
+                        new: Some(addr),
+                    });
                     endpoint_changed = true;
                 }
             }
             if !endpoint_changed && !new.candidates.is_empty() {
-                changes.push(ChangeString::new(
-                    "Connection status",
-                    "Disconnected".into(),
-                    "NAT traverse reattempt".into(),
-                ));
+                changes.push(PeerChange::NatTraverseReattempt)
             }
         }
 


Test patch: