Use semver fallback sort when tag timestamps are equal (#1579)

Fixes #1533

---------

Co-authored-by: Jo <10510431+j178@users.noreply.github.com>

Author: shaanmajid

crates/prek/src/cli/auto_update.rs

@@ -11,6 +11,7 @@ use owo_colors::OwoColorize;
 use prek_consts::PRE_COMMIT_HOOKS_YAML;
 use rustc_hash::FxHashMap;
 use rustc_hash::FxHashSet;
+use semver::Version;
 use toml_edit::DocumentMut;
 use tracing::{debug, trace};
 
@@ -289,6 +290,9 @@ async fn resolve_bleeding_edge(repo_path: &Path) -> Result<Option<String>> {
 }
 
 /// Returns all tags and their Unix timestamps (newest first).
+///
+/// Within groups of tags sharing the same timestamp, semver-parseable tags
+/// are sorted highest version first; non-semver tags sort after them.
 async fn get_tag_timestamps(repo: &Path) -> Result<Vec<(String, u64)>> {
     let output = git::git_cmd("git for-each-ref")?
         .arg("for-each-ref")
@@ -303,7 +307,7 @@ async fn get_tag_timestamps(repo: &Path) -> Result<Vec<(String, u64)>> {
         .output()
         .await?;
 
-    Ok(String::from_utf8_lossy(&output.stdout)
+    let mut tags: Vec<(String, u64)> = String::from_utf8_lossy(&output.stdout)
         .lines()
         .filter_map(|line| {
             let mut parts = line.split_whitespace();
@@ -312,7 +316,26 @@ async fn get_tag_timestamps(repo: &Path) -> Result<Vec<(String, u64)>> {
             let ts: u64 = ts_str.parse().ok()?;
             Some((tag.to_string(), ts))
         })
-        .collect())
+        .collect();
+
+    // Deterministic sort: primary key is timestamp (newest first).
+    // Within equal timestamps, prefer higher semver versions; non-semver tags
+    // sort after semver ones. As a final tie-breaker, compare the tag refname
+    // so ordering is stable across platforms/filesystems.
+    tags.sort_by(|(tag_a, ts_a), (tag_b, ts_b)| {
+        ts_b.cmp(ts_a).then_with(|| {
+            let ver_a = Version::parse(tag_a.strip_prefix('v').unwrap_or(tag_a));
+            let ver_b = Version::parse(tag_b.strip_prefix('v').unwrap_or(tag_b));
+            match (ver_a, ver_b) {
+                (Ok(a), Ok(b)) => b.cmp(&a).then_with(|| tag_a.cmp(tag_b)),
+                (Ok(_), Err(_)) => std::cmp::Ordering::Less,
+                (Err(_), Ok(_)) => std::cmp::Ordering::Greater,
+                (Err(_), Err(_)) => tag_a.cmp(tag_b),
+            }
+        })
+    });
+
+    Ok(tags)
 }
 
 async fn resolve_revision(
@@ -908,4 +931,40 @@ mod tests {
 
         assert_eq!(rev, Some("v1.2.0".to_string()));
     }
+
+    #[tokio::test]
+    async fn test_get_tag_timestamps_stable_order_for_equal_timestamps() {
+        let tmp = setup_test_repo().await;
+        let repo = tmp.path();
+
+        // Create multiple tags on the same commit (same timestamp)
+        create_backdated_commit(repo, "release", 5).await;
+        create_lightweight_tag(repo, "v1.0.0").await;
+        create_lightweight_tag(repo, "v1.0.3").await;
+        create_lightweight_tag(repo, "v1.0.5").await;
+        create_lightweight_tag(repo, "v1.0.2").await;
+
+        let timestamps = get_tag_timestamps(repo).await.unwrap();
+
+        // All timestamps are equal (tags on same commit).
+        // Within equal timestamps, semver tags should sort highest version first.
+        let tags: Vec<&str> = timestamps.iter().map(|(t, _)| t.as_str()).collect();
+        assert_eq!(tags, vec!["v1.0.5", "v1.0.3", "v1.0.2", "v1.0.0"]);
+    }
+
+    #[tokio::test]
+    async fn test_get_tag_timestamps_deterministic_order_for_equal_timestamp_non_semver() {
+        let tmp = setup_test_repo().await;
+        let repo = tmp.path();
+
+        // Lightweight tags on the same commit share a timestamp.
+        create_backdated_commit(repo, "release", 5).await;
+        create_lightweight_tag(repo, "beta").await;
+        create_lightweight_tag(repo, "alpha").await;
+        create_lightweight_tag(repo, "gamma").await;
+
+        let timestamps = get_tag_timestamps(repo).await.unwrap();
+        let tags: Vec<&str> = timestamps.iter().map(|(t, _)| t.as_str()).collect();
+        assert_eq!(tags, vec!["alpha", "beta", "gamma"]);
+    }
 }

crates/prek/tests/auto_update.rs

@@ -9,8 +9,31 @@ use crate::common::{TestContext, cmd_snapshot, git_cmd};
 
 mod common;
 
-/// Helper function to create a local git repository with hooks
+const BASE_TIMESTAMP: u64 = 1_000_000_000;
+const INCREMENTING_STEP_SECS: u64 = 100;
+const FIXED_STEP_SECS: u64 = 0;
+
+/// Helper function to create a local git repository with hooks and incrementing timestamps.
 fn create_local_git_repo(context: &TestContext, repo_name: &str, tags: &[&str]) -> Result<String> {
+    create_local_git_repo_with_timestamps(context, repo_name, tags, INCREMENTING_STEP_SECS)
+}
+
+/// Like `create_local_git_repo`, but all commits and tags share a single fixed timestamp.
+/// Simulates mirror repos where all tags are imported simultaneously.
+fn create_local_git_repo_fixed_ts(
+    context: &TestContext,
+    repo_name: &str,
+    tags: &[&str],
+) -> Result<String> {
+    create_local_git_repo_with_timestamps(context, repo_name, tags, FIXED_STEP_SECS)
+}
+
+fn create_local_git_repo_with_timestamps(
+    context: &TestContext,
+    repo_name: &str,
+    tags: &[&str],
+    timestamp_step_secs: u64,
+) -> Result<String> {
     let repo_dir = context.home_dir().child(format!("test-repos/{repo_name}"));
     repo_dir.create_dir_all()?;
 
@@ -20,24 +43,6 @@ fn create_local_git_repo(context: &TestContext, repo_name: &str, tags: &[&str])
         .arg("init")
         .assert()
         .success();
-    git_cmd(&repo_dir)
-        .arg("config")
-        .arg("user.name")
-        .arg("Prek Test")
-        .assert()
-        .success();
-    git_cmd(&repo_dir)
-        .arg("config")
-        .arg("user.email")
-        .arg("test@prek.dev")
-        .assert()
-        .success();
-    git_cmd(&repo_dir)
-        .arg("config")
-        .arg("core.autocrlf")
-        .arg("false")
-        .assert()
-        .success();
 
     // Create .pre-commit-hooks.yaml
     repo_dir
@@ -55,7 +60,7 @@ fn create_local_git_repo(context: &TestContext, repo_name: &str, tags: &[&str])
 
     git_cmd(&repo_dir).arg("add").arg(".").assert().success();
 
-    let mut timestamp = 1_000_000_000;
+    let mut timestamp = BASE_TIMESTAMP;
 
     git_cmd(&repo_dir)
         .arg("commit")
@@ -68,7 +73,7 @@ fn create_local_git_repo(context: &TestContext, repo_name: &str, tags: &[&str])
 
     // Create tags
     for tag in tags {
-        timestamp += 100;
+        timestamp += timestamp_step_secs;
         git_cmd(&repo_dir)
             .arg("commit")
             .arg("-m")
@@ -89,7 +94,7 @@ fn create_local_git_repo(context: &TestContext, repo_name: &str, tags: &[&str])
             .success();
     }
 
-    timestamp += 100;
+    timestamp += timestamp_step_secs;
     // Add an extra commit to the tip
     git_cmd(&repo_dir)
         .arg("commit")
@@ -1330,6 +1335,174 @@ fn auto_update_freeze_toml() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn auto_update_equal_timestamp_tags_picks_highest_version() -> Result<()> {
+    let context = TestContext::new();
+    context.init_project();
+
+    let repo_path = create_local_git_repo_fixed_ts(
+        &context,
+        "mirror-repo",
+        &["v1.0.0", "v1.0.1", "v1.0.2", "v1.0.3", "v1.0.4", "v1.0.5"],
+    )?;
+
+    context.write_pre_commit_config(&indoc::formatdoc! {r"
+        repos:
+          - repo: {}
+            rev: v1.0.3
+            hooks:
+              - id: test-hook
+    ", repo_path});
+
+    context.git_add(".");
+
+    let filters = context.filters();
+    cmd_snapshot!(filters.clone(), context.auto_update().arg("--cooldown-days").arg("0"), @r#"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    [[HOME]/test-repos/mirror-repo] updating v1.0.3 -> v1.0.5
+
+    ----- stderr -----
+    "#);
+
+    insta::with_settings!(
+        { filters => filters.clone() },
+        {
+            assert_snapshot!(context.read(PRE_COMMIT_CONFIG_YAML), @r#"
+            repos:
+              - repo: [HOME]/test-repos/mirror-repo
+                rev: v1.0.5
+                hooks:
+                  - id: test-hook
+            "#);
+        }
+    );
+
+    Ok(())
+}
+
+// When all tags share a timestamp and some are non-semver (e.g. "latest", "stable"),
+// semver tags should be preferred and sorted highest-first.
+#[test]
+fn auto_update_equal_timestamp_prefers_semver_over_nonsemver() -> Result<()> {
+    let context = TestContext::new();
+    context.init_project();
+
+    let repo_path = create_local_git_repo_fixed_ts(
+        &context,
+        "mixed-tags-repo",
+        &["v1.0.0", "latest", "v2.0.0", "stable"],
+    )?;
+
+    context.write_pre_commit_config(&indoc::formatdoc! {r"
+        repos:
+          - repo: {}
+            rev: v1.0.0
+            hooks:
+              - id: test-hook
+    ", repo_path});
+
+    context.git_add(".");
+
+    let filters = context.filters();
+
+    cmd_snapshot!(filters.clone(), context.auto_update().arg("--cooldown-days").arg("0"), @r#"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    [[HOME]/test-repos/mixed-tags-repo] updating v1.0.0 -> v2.0.0
+
+    ----- stderr -----
+    "#);
+
+    insta::with_settings!(
+        { filters => filters.clone() },
+        {
+            assert_snapshot!(context.read(PRE_COMMIT_CONFIG_YAML), @r#"
+            repos:
+              - repo: [HOME]/test-repos/mixed-tags-repo
+                rev: v2.0.0
+                hooks:
+                  - id: test-hook
+            "#);
+        }
+    );
+
+    Ok(())
+}
+
+// When tags span multiple timestamp groups, the newest group should be selected first.
+// Within an equal-timestamp group, semver tiebreaker picks the highest version.
+#[test]
+fn auto_update_mixed_timestamps_with_equal_subgroups() -> Result<()> {
+    let context = TestContext::new();
+    context.init_project();
+
+    // Create base repo with v1.0.x tags at incrementing timestamps.
+    let repo_path = create_local_git_repo(&context, "mixed-ts-repo", &["v1.0.0", "v1.0.1"])?;
+
+    // Add a second group of tags sharing a single newer timestamp
+    // (must be in the past so the cooldown filter doesn't exclude them).
+    let newer_ts = "1500000000 +0000";
+    for tag in &["v2.0.1", "v2.0.0"] {
+        git_cmd(&repo_path)
+            .arg("commit")
+            .arg("-m")
+            .arg(format!("Release {tag}"))
+            .arg("--allow-empty")
+            .env("GIT_AUTHOR_DATE", newer_ts)
+            .env("GIT_COMMITTER_DATE", newer_ts)
+            .assert()
+            .success();
+        git_cmd(&repo_path)
+            .arg("tag")
+            .arg(tag)
+            .arg("-m")
+            .arg(tag)
+            .env("GIT_AUTHOR_DATE", newer_ts)
+            .env("GIT_COMMITTER_DATE", newer_ts)
+            .assert()
+            .success();
+    }
+
+    context.write_pre_commit_config(&indoc::formatdoc! {r"
+        repos:
+          - repo: {}
+            rev: v1.0.0
+            hooks:
+              - id: test-hook
+    ", repo_path});
+
+    context.git_add(".");
+
+    let filters = context.filters();
+
+    cmd_snapshot!(filters.clone(), context.auto_update().arg("--cooldown-days").arg("0"), @r#"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    [[HOME]/test-repos/mixed-ts-repo] updating v1.0.0 -> v2.0.1
+
+    ----- stderr -----
+    "#);
+
+    insta::with_settings!(
+        { filters => filters.clone() },
+        {
+            assert_snapshot!(context.read(PRE_COMMIT_CONFIG_YAML), @r#"
+            repos:
+              - repo: [HOME]/test-repos/mixed-ts-repo
+                rev: v2.0.1
+                hooks:
+                  - id: test-hook
+            "#);
+        }
+    );
+
+    Ok(())
+}
+
 #[test]
 fn auto_update_freeze_toml_with_comment() -> Result<()> {
     let context = TestContext::new();