fix: version prefix detection#5943
Conversation
|
The test for this fails, but I didn't change any, so I guess it was broken since the original PR #2889.
I'm not sure why we need this, but can I fix the test to match the current behaviour? |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes version prefix detection by replacing a regex-based approach with a more sophisticated parsing mechanism that handles complex version prefixes like "mountpoint-s3-". The change addresses an issue where tools with multi-part prefixes weren't being correctly parsed.
- Introduces a new
semvermodule withsplit_version_prefixfunction to properly handle complex version prefixes - Replaces regex-based prefix detection with character-by-character parsing that respects delimiters
- Moves the
chunkify_versionfunction fromoutdated_info.rsto the newsemvermodule for better organization
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/semver.rs | New module containing version parsing utilities with comprehensive test coverage |
| src/toolset/outdated_info.rs | Removes duplicated chunkify_version function and updates imports |
| src/aqua/aqua_registry.rs | Replaces regex-based prefix detection with new split_version_prefix function |
| src/main.rs | Adds the new semver module declaration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| /// split a version number into chunks | ||
| /// given v: "1.2-3a4" return ["1", ".2", "-3a4"] |
There was a problem hiding this comment.
The comment example shows incorrect output. According to the test on line 90, the actual output for "1.2-3a4" is ["1", ".2", "-3a4"], but the comment shows "-3a4" as the last element instead of "-3a4".
|
bugbot run |
| return Some(i); | ||
| } | ||
| // If the previous char is a delimiter or 'v', we found a split point. | ||
| let prev_char = version.chars().nth(i - 1).unwrap(); |
There was a problem hiding this comment.
Bug: Unicode Indexing Mismatch Causes Panic
The split_version_prefix function incorrectly mixes byte indices from char_indices() with character indices for chars().nth(). When char_indices() returns a byte index i for a digit, version.chars().nth(i - 1) attempts to access the character at character position i - 1. For strings containing multi-byte Unicode characters, this can cause unwrap() to panic because i - 1 (a byte index) may exceed the actual character count, leading chars().nth() to return None. It can also result in incorrect prefix detection if a panic does not occur.
There was a problem hiding this comment.
I don't think this is a concern, version numbers probably don't support unicode anyways
There was a problem hiding this comment.
Yes, and also I think it's incorrect.
char_indices doesn't return byte indices.
fn main() {
let str = "😀1㐂";
println!("{:?}", str.char_indices());
println!("{:?}", str.chars());
}CharIndices { front_offset: 0, iter: Chars(['😀', '1', '㐂']) }
Chars(['😀', '1', '㐂'])
### 📦 Registry - enable kubecolor test by [@risu729](https://github.com/risu729) in [#6008](#6008) - fix os specific backends for usage by [@risu729](https://github.com/risu729) in [#6007](#6007) - use aqua backend for restish by [@risu729](https://github.com/risu729) in [#5986](#5986) - add cfssljson ([aqua:cloudflare/cfssl/cfssljson](https://github.com/cloudflare/cfssl/cfssljson)) by [@disintegrator](https://github.com/disintegrator) in [#6013](#6013) - add claude-squad ([aqua:smtg-ai/claude-squad](https://github.com/smtg-ai/claude-squad)) by [@TyceHerrman](https://github.com/TyceHerrman) in [#5894](#5894) ### 🚀 Features - **(aqua)** make bin paths executable by [@risu729](https://github.com/risu729) in [#6010](#6010) - added header bar during `mise install` by [@jdx](https://github.com/jdx) in [#6022](#6022) ### 🐛 Bug Fixes - **(aqua)** improve warnings for packages without repo_owner and repo_name (2nd attempt) by [@risu729](https://github.com/risu729) in [#6009](#6009) - version prefix detection by [@risu729](https://github.com/risu729) in [#5943](#5943) - respect MISE_DEFAULT_CONFIG_FILENAME by [@risu729](https://github.com/risu729) in [#5899](#5899) ### New Contributors - @disintegrator made their first contribution in [#6013](#6013)
Resolves #5935 (comment).
Since we have this, we cannot simply treat all non-numeric characters in the front as a prefix.
https://github.com/aquaproj/aqua-registry/blob/4775b3bb05d5526199c87270227e3e1dc189b16e/pkgs/awslabs/mountpoint-s3/registry.yaml#L8
The prefix regexes are introduced in #4340 and #1392. I think this can safely replace those.