-
-
Notifications
You must be signed in to change notification settings - Fork 256
fix: fixing bug identified in [Issue #1080](https://github.com/orhun/… #1227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
755f216
164aefd
1d8ee9b
8db59b6
568f15e
16f526a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -466,7 +466,15 @@ impl Repository { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if !topo_order { | ||||||||||||||||||||||||
| tags.sort_by(|a, b| a.0.time().seconds().cmp(&b.0.time().seconds())); | ||||||||||||||||||||||||
| tags.sort_by(|a, b| { | ||||||||||||||||||||||||
| let time_cmp = a.0.time().seconds().cmp(&b.0.time().seconds()); | ||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| time_cmp | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Ok(tags | ||||||||||||||||||||||||
| .into_iter() | ||||||||||||||||||||||||
|
|
@@ -510,6 +518,73 @@ impl Repository { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Compares two version strings semantically. | ||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// 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 | ||||||||||||||||||||||||
|
Comment on lines
+523
to
+527
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// If semantic parsing fails, it falls back to alphabetical comparison. | ||||||||||||||||||||||||
| fn semantic_version_compare(a: &str, b: &str) -> std::cmp::Ordering { | ||||||||||||||||||||||||
| use std::cmp::Ordering; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Helper function to extract version parts and pre-release info | ||||||||||||||||||||||||
| let parse_version = |version: &str| -> Option<(Vec<u64>, Option<String>)> { | ||||||||||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, |
||||||||||||||||||||||||
| // Remove common prefixes like 'v' or 'version-' | ||||||||||||||||||||||||
| let cleaned = version | ||||||||||||||||||||||||
| .strip_prefix('v') | ||||||||||||||||||||||||
| .or_else(|| version.strip_prefix("version-")) | ||||||||||||||||||||||||
| .unwrap_or(version); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Split by '-' to separate main version from pre-release | ||||||||||||||||||||||||
| let parts: Vec<&str> = cleaned.split('-').collect(); | ||||||||||||||||||||||||
| let main_version = parts.first()?; | ||||||||||||||||||||||||
| let pre_release = if parts.len() > 1 { | ||||||||||||||||||||||||
| Some(parts[1..].join("-")) | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| None | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let version_parts = main_version | ||||||||||||||||||||||||
| .split('.') | ||||||||||||||||||||||||
| .map(|part| part.parse::<u64>().ok()) | ||||||||||||||||||||||||
| .collect::<Option<Vec<u64>>>()?; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Some((version_parts, pre_release)) | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| match (parse_version(a), parse_version(b)) { | ||||||||||||||||||||||||
| (Some((parts_a, pre_a)), Some((parts_b, pre_b))) => { | ||||||||||||||||||||||||
| // First compare the main version parts numerically | ||||||||||||||||||||||||
| for (part_a, part_b) in parts_a.iter().zip(parts_b.iter()) { | ||||||||||||||||||||||||
| match part_a.cmp(part_b) { | ||||||||||||||||||||||||
| Ordering::Equal => continue, | ||||||||||||||||||||||||
| other => return other, | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // If main version parts are equal, compare lengths | ||||||||||||||||||||||||
| match parts_a.len().cmp(&parts_b.len()) { | ||||||||||||||||||||||||
| Ordering::Equal => { | ||||||||||||||||||||||||
| // If main versions are identical, compare pre-release | ||||||||||||||||||||||||
| match (pre_a, pre_b) { | ||||||||||||||||||||||||
| (None, None) => Ordering::Equal, | ||||||||||||||||||||||||
| (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 */ | ||||||||||||||||||||||||
|
Comment on lines
+574
to
+577
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comments seem a bit off here. Maybe use this format: // pre-release < release
(Some(_), None) => Ordering::Less,
etc. |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| other => other, | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| // If semantic parsing fails for either version, fall back to alphabetical | ||||||||||||||||||||||||
| _ => a.cmp(b), | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| fn find_remote(url: &str) -> Result<Remote> { | ||||||||||||||||||||||||
| url_path_segments(url).or_else(|err| { | ||||||||||||||||||||||||
| if url.contains("@") && url.contains(":") && url.contains("/") { | ||||||||||||||||||||||||
|
|
@@ -805,6 +880,20 @@ mod test { | |||||||||||||||||||||||
| (repo, temp_dir) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| fn create_tag(repo: &Repository, tag_name: &str, commit_id: Option<&str>) { | ||||||||||||||||||||||||
| let mut args = vec!["tag", tag_name]; | ||||||||||||||||||||||||
| if let Some(id) = commit_id { | ||||||||||||||||||||||||
| args.push(id); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let output = Command::new("git") | ||||||||||||||||||||||||
| .args(&args) | ||||||||||||||||||||||||
| .current_dir(&repo.path) | ||||||||||||||||||||||||
| .output() | ||||||||||||||||||||||||
| .expect("failed to execute git tag"); | ||||||||||||||||||||||||
| assert!(output.status.success(), "git tag failed {:?}", output); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn open_jujutsu_repo() { | ||||||||||||||||||||||||
| let (repo, _temp_dir) = create_temp_repo(); | ||||||||||||||||||||||||
|
|
@@ -976,4 +1065,137 @@ mod test { | |||||||||||||||||||||||
| assert!(!retain, "exclude: **/*.txt"); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Test the semantic version comparison function | ||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn test_semantic_version_compare() { | ||||||||||||||||||||||||
| use std::cmp::Ordering; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Test basic semantic version comparison | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v0.9.0", "v0.10.0"), | ||||||||||||||||||||||||
| Ordering::Less | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v0.10.0", "v0.9.0"), | ||||||||||||||||||||||||
| Ordering::Greater | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v1.0.0", "v1.0.0"), | ||||||||||||||||||||||||
| Ordering::Equal | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Test without 'v' prefix | ||||||||||||||||||||||||
| assert_eq!(semantic_version_compare("0.9.0", "0.10.0"), Ordering::Less); | ||||||||||||||||||||||||
| assert_eq!(semantic_version_compare("1.2.3", "1.2.4"), Ordering::Less); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Test with pre-release versions | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v1.0.0-alpha.1", "v1.0.0-alpha.2"), | ||||||||||||||||||||||||
| Ordering::Less | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Test pre-release vs release (lines 574-575) | ||||||||||||||||||||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pointing out to lines that might change in the future does not make sense, can you remove them and reference to the function instead? |
||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v1.0.0-alpha.1", "v1.0.0"), | ||||||||||||||||||||||||
| Ordering::Less | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v1.0.0", "v1.0.0-beta.1"), | ||||||||||||||||||||||||
| Ordering::Greater | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Test different version lengths | ||||||||||||||||||||||||
| assert_eq!(semantic_version_compare("v1.0", "v1.0.0"), Ordering::Less); | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| semantic_version_compare("v1.0.0", "v1.0"), | ||||||||||||||||||||||||
| Ordering::Greater | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
Comment on lines
+1115
to
+1123
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear which GitHub issue we are referring to here, it should be linked or the comment should be made more clear |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Test fallback to alphabetical for non-semantic versions | ||||||||||||||||||||||||
| assert_eq!(semantic_version_compare("abc", "def"), Ordering::Less); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Test that reproduces and verifies the fix for the issue where tags with | ||||||||||||||||||||||||
| /// double-digit version numbers are sorted alphabetically instead of semantically. | ||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// 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. | ||||||||||||||||||||||||
|
Comment on lines
+1132
to
+1133
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn test_semantic_version_tag_sorting_issue() -> Result<()> { | ||||||||||||||||||||||||
| let (repo, _temp_dir) = create_temp_repo(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Create three separate commits to avoid tag conflicts | ||||||||||||||||||||||||
| let commit1 = create_commit_with_files(&repo, vec![("file1.txt", "content1")]); | ||||||||||||||||||||||||
| let commit1_id = commit1.id().to_string(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let commit2 = create_commit_with_files(&repo, vec![("file2.txt", "content2")]); | ||||||||||||||||||||||||
| let commit2_id = commit2.id().to_string(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let commit3 = create_commit_with_files(&repo, vec![("file3.txt", "content3")]); | ||||||||||||||||||||||||
| let commit3_id = commit3.id().to_string(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Create tags that would be problematic when sorted alphabetically | ||||||||||||||||||||||||
| // Tag them in reverse chronological order to test the sorting | ||||||||||||||||||||||||
| create_tag(&repo, "v0.9.0", Some(&commit1_id)); // Oldest commit | ||||||||||||||||||||||||
| create_tag(&repo, "v0.10.0", Some(&commit2_id)); // Middle commit | ||||||||||||||||||||||||
| create_tag(&repo, "v0.11.0", Some(&commit3_id)); // Newest commit | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Get tags with time-based sorting (topo_order = false) | ||||||||||||||||||||||||
| let tags = repo.tags(&None, false, false)?; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Convert to vector to check ordering | ||||||||||||||||||||||||
| let tag_names: Vec<String> = tags.values().map(|tag| tag.name.clone()).collect(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // With the fix, tags should be sorted semantically when commit times are equal | ||||||||||||||||||||||||
| // Expected order: v0.9.0, v0.10.0, v0.11.0 | ||||||||||||||||||||||||
| let _expected_order = ["v0.9.0", "v0.10.0", "v0.11.0"]; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Find positions of our test tags | ||||||||||||||||||||||||
| let v090_pos = tag_names.iter().position(|name| name == "v0.9.0"); | ||||||||||||||||||||||||
| let v0100_pos = tag_names.iter().position(|name| name == "v0.10.0"); | ||||||||||||||||||||||||
| let v0110_pos = tag_names.iter().position(|name| name == "v0.11.0"); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| assert!(v090_pos.is_some(), "v0.9.0 tag should be found"); | ||||||||||||||||||||||||
| assert!(v0100_pos.is_some(), "v0.10.0 tag should be found"); | ||||||||||||||||||||||||
| assert!(v0110_pos.is_some(), "v0.11.0 tag should be found"); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let v090_pos = v090_pos.expect("Value v0.9.0 is expected"); | ||||||||||||||||||||||||
| let v0100_pos = v0100_pos.expect("Value 0.10.0 is expected"); | ||||||||||||||||||||||||
| let v0110_pos = v0110_pos.expect("Value 0.11.0 is expected"); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Verify semantic ordering: v0.9.0 < v0.10.0 < v0.11.0 | ||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||
| v090_pos < v0100_pos, | ||||||||||||||||||||||||
| "v0.9.0 (pos {}) should come before v0.10.0 (pos {}) in semantic order", | ||||||||||||||||||||||||
| v090_pos, | ||||||||||||||||||||||||
| v0100_pos | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||
| v0100_pos < v0110_pos, | ||||||||||||||||||||||||
| "v0.10.0 (pos {}) should come before v0.11.0 (pos {}) in semantic order", | ||||||||||||||||||||||||
| v0100_pos, | ||||||||||||||||||||||||
| v0110_pos | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // The last tag should be v0.11.0 (semantically latest) | ||||||||||||||||||||||||
| let last_tag = tags.last().expect("should have at least one tag"); | ||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||
| last_tag.1.name, "v0.11.0", | ||||||||||||||||||||||||
| "Last tag should be v0.11.0 (semantically latest), but got {}", | ||||||||||||||||||||||||
| last_tag.1.name | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?