fix: fixing bug identified in [Issue #1080](https://github.com/orhun/… #1227

mbabauer · 2025-07-28T13:36:28Z

…git-cliff/issues/1080)

Description

This is an attempt to fix the issue described in Issue #1080.

Motivation and Context

When multiple tags are included in the next release and the tags jump up in digit size, ex moving from 1.9.0 to 1.10.0, the "highest" version was being identified as 1.9.0 due to the tags being sorted alphabetically instead of by the actual version. This causes the next version to be misidentified as 1.10.0 instead of 1.11.0.

The semantic_version_compare function attempts to sort by version. If it fails, it should fall back to alphabetical sort (see like 589). All my changes were to repo.rs. The other files are the results of me running the cargo +nightly fmt. I am fairly new to the Rust toolchain so if that's not correct and you need me to revert those changes I can.

This is an attempt to fix Issue #1080.

How Has This Been Tested?

First, I create a unit test to recreate the issue. The test creates tags v0.9.0, v0.10.0 and v0.11.0 on a temporary repo. The test breaks because it sorted the v0.9.0 tag would sort to the end of the vector. Once I could see it happening in a repeatable test I...
Created the fix so that the test passed. In the test I should expect the sorted order to be: v0.9.0, v0.10.0 then v0.11.0.
I also included a unit test to test the new semantic_version_compare function to have a complete set of tests. It tries to test some "edge" cases, like ensuring v1.0 is less than v1.0.0 and how to handle things like alpha releases and the fall-back to alpha-sorting when the comparison is between 2 non- Semver strings.

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other - formatting changes due to running the formatter tool

Checklist:

My code follows the code style of this project.
I have updated the documentation accordingly.
I have formatted the code with rustfmt.
I checked the lints with clippy.
I have added tests to cover my changes.
All new and existing tests passed.

welcome · 2025-07-28T13:36:31Z

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

codecov-commenter · 2025-07-28T13:41:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 42.41%. Comparing base (8b5a547) to head (16f526a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1227      +/-   ##
==========================================
+ Coverage   41.59%   42.41%   +0.82%     
==========================================
  Files          22       22              
  Lines        1919     1948      +29     
==========================================
+ Hits          798      826      +28     
- Misses       1121     1122       +1

Flag	Coverage Δ
unit-tests	`42.41% <100.00%> (+0.82%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

orhun · 2025-07-28T16:35:23Z

Thanks for the PR! I might need to get myself up-to-date with that issue again to understand the changes here so give me some time.

In the meantime, there seems to be unrelated formatting changes in this PR. Can you run cargo +nightly fmt instead and commit the results?

mbabauer · 2025-07-29T20:32:46Z

Ok, took a few attempts to get my toolchain setup right apparently, but I think I have all the changes in. Let me know if you see any further updates needed.

orhun

This is looking pretty good! I had some questions and suggestions.

Also, can you please adjust the PR title accordingly? It should mention what is being added/changed in this PR instead of a general message like the current one.

orhun · 2025-08-02T17:04:45Z

git-cliff-core/src/repo.rs

+    /// This test simulates the scenario described in GitHub issue #1080 where v0.10.0
+    /// was incorrectly considered "less than" v0.9.0 due to alphabetical sorting.


Suggested change

/// This test simulates the scenario described in GitHub issue #1080 where v0.10.0

/// was incorrectly considered "less than" v0.9.0 due to alphabetical sorting.

/// This test simulates the scenario described in GitHub issue [#1080] where v0.10.0

/// was incorrectly considered "less than" v0.9.0 due to alphabetical sorting.

///

/// [#1080]: https://github.com/orhun/git-cliff/issues/1080

orhun · 2025-08-02T17:06:24Z

git-cliff-core/src/repo.rs

+            Ordering::Less
+        );
+
+        // Test pre-release vs release (lines 574-575)


Pointing out to lines that might change in the future does not make sense, can you remove them and reference to the function instead?

orhun · 2025-08-02T17:07:03Z

git-cliff-core/src/repo.rs

+        // Test the specific case from the GitHub issue
+        assert_eq!(
+            semantic_version_compare("v0.9.0", "v0.10.0"),
+            Ordering::Less
+        );
+        assert_eq!(
+            semantic_version_compare("v0.10.0", "v0.11.0"),
+            Ordering::Less
+        );


It's not clear which GitHub issue we are referring to here, it should be linked or the comment should be made more clear

orhun · 2025-08-02T17:08:16Z

git-cliff-core/src/repo.rs

+                if time_cmp == std::cmp::Ordering::Equal {
+                    // When commit times are equal, sort by semantic version
+                    semantic_version_compare(&a.1.name, &b.1.name)


I'm not sure if I understood how this fixes #1080... In that issue we didn't talk about commit times being equal etc. but we are doing a a check here.

Can you explain it further?

orhun · 2025-08-02T17:08:30Z

git-cliff-core/src/repo.rs

+/// This function attempts to parse version strings and compare them semantically
+/// rather than alphabetically. It handles common version formats like:
+/// - v1.2.3
+/// - 1.2.3
+/// - v1.2.3-alpha.1


nit:

Suggested change

/// This function attempts to parse version strings and compare them semantically

/// rather than alphabetically. It handles common version formats like:

/// - v1.2.3

/// - 1.2.3

/// - v1.2.3-alpha.1

/// This function attempts to parse version strings and compare them semantically

/// rather than alphabetically. It handles common version formats like:

///

/// - v1.2.3

/// - 1.2.3

/// - v1.2.3-alpha.1

orhun · 2025-08-02T17:10:23Z

git-cliff-core/src/repo.rs

+    use std::cmp::Ordering;
+
+    // Helper function to extract version parts and pre-release info
+    let parse_version = |version: &str| -> Option<(Vec<u64>, Option<String>)> {


Hmm, Option<(Vec<u64>, Option<String>)> is a bit complex. Do you think defining an inner struct would make the code better in terms of readability? 🤔

orhun · 2025-08-02T17:11:38Z

git-cliff-core/src/repo.rs

+                        (Some(_), None) => Ordering::Less, // pre-release < release
+                        (None, Some(_)) => Ordering::Greater, // release > pre-release
+                        (Some(pre_a), Some(pre_b)) => pre_a.cmp(&pre_b), /* alphabetical for
+                                                             * pre-release */


The comments seem a bit off here. Maybe use this format:

// pre-release < release (Some(_), None) => Ordering::Less, etc.

fix: fixing bug identified in [Issue orhun#1080](orhun#1080)

755f216

mbabauer requested a review from orhun as a code owner July 28, 2025 13:36

Michael Bauer added 4 commits July 29, 2025 15:24

chore: re-running fmt with nightly

164aefd

chore: re-re-running fmt with nightly

1d8ee9b

chore: switching out unwrap() with expect() per linter

8db59b6

chore: swapping to plain array per linter

568f15e

chore: adding additional scenarios in to cover missing code lines

16f526a

orhun reviewed Aug 2, 2025

View reviewed changes

		/// This test simulates the scenario described in GitHub issue #1080 where v0.10.0
		/// was incorrectly considered "less than" v0.9.0 due to alphabetical sorting.

Uh oh!

fix: fixing bug identified in [Issue #1080](https://github.com/orhun/… #1227

Are you sure you want to change the base?

fix: fixing bug identified in [Issue #1080](https://github.com/orhun/… #1227

Uh oh!

Conversation

mbabauer commented Jul 28, 2025

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

Uh oh!

welcome bot commented Jul 28, 2025

Uh oh!

codecov-commenter commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orhun commented Jul 28, 2025

Uh oh!

mbabauer commented Jul 29, 2025

Uh oh!

orhun left a comment

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

orhun Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 28, 2025 •

edited

Loading