Skip to content

Conversation

@MrDwarf7
Copy link
Contributor

@MrDwarf7 MrDwarf7 commented Nov 15, 2025

Summary

Closes: #351

This pull request includes the following changes:

Details

This pull request addresses a bug identified in #351, where the commitish regex did not enforce the presence of dots between version numbers. The updated regex now requires dots, ensuring that version strings like "1.0.0" are correctly recognized.

This was done by changing the regex pattern from using a (\.)? to (\.)+, ensuring that at least one dot is present between version segments. Reaming repeats of dots are handled by the appended {_,_} number quantifier.

@blob42
Copy link

blob42 commented Nov 15, 2025

@MrDwarf7 I did not notice your PR. I also did a fix but simply switches an if statment: parse the commit first then the version. #353

@MrDwarf7
Copy link
Contributor Author

This was my initial call too.

I figured to check other sections of the CodeBase as I've been working with it on and off for awhile now & figured I'd triple check the actual RegEx that we're compiling. I would say it's a bug as we're not (originally) enforcing the existence of the full stop during parsing.

Rust aims to move as much of its validation logic, or really any logic that it can, to compile time, over procedural validations and relying on ordering of checks. Procedural validations can be dangerous in that they allow for invalid states to exist (often way too far into the program lifespan).

Rust has ample tools to enforce this via enums & matches-even things like the Ord trait are available if so desired.
I tend to favour the 'write your program in such a way that "invalid states are unpresentable"' approach as you get a logically congruent program that tends to be fairly easy to follow too.

Relying on match check ordering without a way to enforce it at compile time is bugs waiting to happen for sure.

@MrDwarf7 MrDwarf7 force-pushed the fix/from-commit-hash branch from e2866d7 to c0c74dc Compare November 15, 2025 15:57
@MrDwarf7 MrDwarf7 changed the title [FIX] install via commit(-ish)/hash fix(#351) install via commit(-ish)/hash Nov 15, 2025
@MrDwarf7 MrDwarf7 force-pushed the fix/from-commit-hash branch from c0c74dc to 686e882 Compare November 15, 2025 16:07
@MrDwarf7 MrDwarf7 force-pushed the fix/from-commit-hash branch from 686e882 to 4306b5e Compare November 15, 2025 16:21
@MordechaiHadad
Copy link
Owner

Just so im up to date with the approach used to fix the issue, to clarify the issue, the thing that failed identifying the hash was because the previous regex pattern didn't enforce dots?

@MrDwarf7
Copy link
Contributor Author

Correct, I've tested this locally with all commands that make use of the call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installing from <commit-hash> does not work

3 participants