From 4abea6b601c53adfa5719a7aac11069dc054cdf8 Mon Sep 17 00:00:00 2001
From: Leon Strauss <me@lionc.de>
Date: Mon, 29 Jul 2024 22:22:55 +0200
Subject: [PATCH] fix(cmd_duration): Make render_time format more consistent
 (#5825)

* fix(cmd_duration): Make render_time format consistent

* Cleanup render_time comments

* Fix AWS test for new render_time format
---
 src/modules/aws.rs |  4 +--
 src/utils.rs       | 74 +++++++++++++++++++++++++++-------------------
 2 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/src/modules/aws.rs b/src/modules/aws.rs
index ae5c35d7..c5a3a6f9 100644
--- a/src/modules/aws.rs
+++ b/src/modules/aws.rs
@@ -702,7 +702,7 @@ credential_process = /opt/bin/awscreds-retriever
                 .collect();
 
             let possible_values = [
-                "30m2s", "30m1s", "30m", "29m59s", "29m58s", "29m57s", "29m56s", "29m55s",
+                "30m2s", "30m1s", "30m0s", "29m59s", "29m58s", "29m57s", "29m56s", "29m55s",
             ];
             let possible_values = possible_values.map(|duration| {
                 let segment_colored = format!("☁️  astronauts (ap-northeast-2) [{duration}] ");
@@ -756,7 +756,7 @@ aws_secret_access_key=dummy
                 // In principle, "30m" should be correct. However, bad luck in scheduling
                 // on shared runners may delay it.
                 let possible_values = [
-                    "30m2s", "30m1s", "30m", "29m59s", "29m58s", "29m57s", "29m56s", "29m55s",
+                    "30m2s", "30m1s", "30m0s", "29m59s", "29m58s", "29m57s", "29m56s", "29m55s",
                 ];
                 let possible_values = possible_values.map(|duration| {
                     let segment_colored = format!("☁️  astronauts (ap-northeast-2) [{duration}] ");
diff --git a/src/utils.rs b/src/utils.rs
index 7bd1ee97..be42539e 100644
--- a/src/utils.rs
+++ b/src/utils.rs
@@ -622,9 +622,11 @@ pub fn exec_timeout(cmd: &mut Command, time_limit: Duration) -> Option<CommandOu
 
 // Render the time into a nice human-readable string
 pub fn render_time(raw_millis: u128, show_millis: bool) -> String {
-    // Make sure it renders something if the time equals zero instead of an empty string
-    if raw_millis == 0 {
-        return "0ms".into();
+    // Fast returns for zero cases to render something
+    match (raw_millis, show_millis) {
+        (0, true) => return "0ms".into(),
+        (0..=999, false) => return "0s".into(),
+        _ => (),
     }
 
     // Calculate a simple breakdown into days/hours/minutes/seconds/milliseconds
@@ -633,25 +635,29 @@ pub fn render_time(raw_millis: u128, show_millis: bool) -> String {
     let (minutes, raw_hours) = (raw_minutes % 60, raw_minutes / 60);
     let (hours, days) = (raw_hours % 24, raw_hours / 24);
 
-    let components = [days, hours, minutes, seconds];
-    let suffixes = ["d", "h", "m", "s"];
+    // Calculate how long the string will be to allocate once in most cases
+    let result_capacity = match raw_millis {
+        1..=59 => 3,
+        60..=3599 => 6,
+        3600..=86399 => 9,
+        _ => 12,
+    } + if show_millis { 5 } else { 0 };
 
-    let mut rendered_components: Vec<String> = components
-        .iter()
-        .zip(&suffixes)
-        .map(render_time_component)
-        .collect();
-    if show_millis || raw_millis < 1000 {
-        rendered_components.push(render_time_component((&millis, &"ms")));
-    }
-    rendered_components.join("")
-}
+    let components = [(days, "d"), (hours, "h"), (minutes, "m"), (seconds, "s")];
 
-/// Render a single component of the time string, giving an empty string if component is zero
-fn render_time_component((component, suffix): (&u128, &&str)) -> String {
-    match component {
-        0 => String::new(),
-        n => format!("{n}{suffix}"),
+    // Concat components ito result starting from the first non-zero one
+    let result = components.iter().fold(
+        String::with_capacity(result_capacity),
+        |acc, (component, suffix)| match component {
+            0 if acc.is_empty() => acc,
+            n => acc + &n.to_string() + suffix,
+        },
+    );
+
+    if show_millis {
+        result + &millis.to_string() + "ms"
+    } else {
+        result
     }
 }
 
@@ -713,28 +719,36 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_0ms() {
+    fn render_time_test_0ms() {
         assert_eq!(render_time(0_u128, true), "0ms")
     }
     #[test]
-    fn test_500ms() {
+    fn render_time_test_0s() {
+        assert_eq!(render_time(0_u128, false), "0s")
+    }
+    #[test]
+    fn render_time_test_500ms() {
         assert_eq!(render_time(500_u128, true), "500ms")
     }
     #[test]
-    fn test_10s() {
-        assert_eq!(render_time(10_000_u128, true), "10s")
+    fn render_time_test_500ms_no_millis() {
+        assert_eq!(render_time(500_u128, false), "0s")
     }
     #[test]
-    fn test_90s() {
-        assert_eq!(render_time(90_000_u128, true), "1m30s")
+    fn render_time_test_10s() {
+        assert_eq!(render_time(10_000_u128, true), "10s0ms")
     }
     #[test]
-    fn test_10110s() {
-        assert_eq!(render_time(10_110_000_u128, true), "2h48m30s")
+    fn render_time_test_90s() {
+        assert_eq!(render_time(90_000_u128, true), "1m30s0ms")
     }
     #[test]
-    fn test_1d() {
-        assert_eq!(render_time(86_400_000_u128, true), "1d")
+    fn render_time_test_10110s() {
+        assert_eq!(render_time(10_110_000_u128, true), "2h48m30s0ms")
+    }
+    #[test]
+    fn render_time_test_1d() {
+        assert_eq!(render_time(86_400_000_u128, false), "1d0h0m0s")
     }
 
     #[test]