Add refname rule checking to branch prefix input dialogue #153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
||
| fn validate_branch_prefix(branch_prefix: &str) -> Result<()> { | ||
| // They can include slash / for hierarchical (directory) grouping, but no slash-separated component can begin with a dot . or end with the sequence .lock. | ||
| if branch_prefix.contains("/.") |
There was a problem hiding this comment.
Do you need to check branch_prefix.starts_with("."), too?
There was a problem hiding this comment.
Good point. I missed the fact that due to the prefix, we get a "/" at the beginning anyway and thus it's not enough to just check for "/..
I amended the commit.
cadolphs
force-pushed
the
spr_init_prefix_ref_check_pr
branch
from
b68fde2
to
109560a
Compare
spacedentist
approved these changes
Apr 23, 2023
clarityflowers
pushed a commit
to clarityflowers/spr
that referenced
this pull request
Mar 19, 2024
Fixes spacedentist#152 This PR addresses the potential invalid refnames resulting from a bad input to the `branchPrefix` question during `spr init`. I debated between two approaches. 1. We could just construct a refname based off the branch prefix and run a git command to check if it's a valid refname. I thought that was too hacky and I didn't want to deal with spawning an extra command during the setup. 2. We could just implement the relevant rules directly. Of course if future git versions change those rules, we'll be out of sync with them. I consider that somewhat unlikely though. In setting up the code for the rules, I opted for a more verbose approach. We could certainly mash together everything into one big regex with lots of branches. Due to all those escaped special chars, the regex is already ugly enough. One caveat: Because the refname rules disallow multiple consecutive forward slashes `/`, our branch prefix must not start with even a single `/`. # Testing Unit tests make sure the rules are actually implemented. They should be comprehensive, but they are not exhaustive. For example, there isn't a single test for each and every control character. I trust that the `is_control` method on `char` works as advertised... I also manually tested that the dialogue repeats until an acceptable refname is added. Reviewers: spacedentist Reviewed By: spacedentist Pull Request: spacedentist#153
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #152
This PR addresses the potential invalid refnames resulting from a bad input to the
branchPrefixquestion duringspr init. I debated between two approaches.In setting up the code for the rules, I opted for a more verbose approach. We could certainly mash together everything into one big regex with lots of branches. Due to all those escaped special chars, the regex is already ugly enough.
One caveat: Because the refname rules disallow multiple consecutive forward slashes
/, our branch prefix must not start with even a single/.Testing
Unit tests make sure the rules are actually implemented. They should be comprehensive, but they are not exhaustive. For example, there isn't a single test for each and every control character. I trust that the
is_controlmethod oncharworks as advertised...I also manually tested that the dialogue repeats until an acceptable refname is added.