Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork60
Validate a branch that we parse when running cherry_picker --continue#266
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if prefix != 'backport': | ||
| raise ValueError('branch name is not prefixed with "backport-". Is this a cherry_picker branch?') | ||
| if not re.match('[0-9a-f]{7,40}', sha): |
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.
Could also validate the sha against git (like it's done with a commit in config)
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.
Done.
Ready for review |
| except AttributeError as attr_err: | ||
| raise ValueError(f'Branch {branch} seems to not have a version in its name.') from attr_err | ||
| """Return the branches to cherry-pick to, sorted by version""" |
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.
Nitpick: I prefer PEP 257 compatible docstrings. Please add a period at the end of the sentence.
| if not re.match('[0-9a-f]{7,40}', sha): | ||
| raise ValueError(f'branch name has an invalid sha: {sha}') | ||
| cmd = ['git', 'log', '-r', sha] |
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.
Nitpick:CherryPicker.check_repo() has 99% same code. I'd reuse that.
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 thought about this but something struck me as off about doing that and I couldn't quite put my finger on it yesterday. This morning I realized it's because check_repo() is not well encapsulated. The sha is contained as an attribute on the CherryPicker object but the repository is coming from the current working directory. This wasn't immediately obvious because cherry_picker doesn't give you the option to work on a repository that's not the current working directory but when I thought about making the code generic to both places it triggered my sense that something was wrong.
Now that I know that's what struck me as wrong, I'd like to go ahead and move this to its own function in this PR and submit a second PR that allows setting the working directory from which all the git commands are run. When I do that, though, functions like get_base_branch() will either need to become methods of CherryPicker or I'll have to pass the path to the repository all the way through the stack. Does that future plan sound okay?
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.
Sounds good
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.
Looks perfect! (besides those nitpicks, it's up to you whether to fix them)
Hi@Mariatta, could you please review this? It seems complete to me. |
There is merge conflict now. |
When running cherry_picker --continue we count on being able to getcertain information from the branch's name which cherry_pickerconstructed earlier. Verify as best we can that the branch name is onewhich cherry_picker could have constructed.Relies on the changes here:python#265(which does the work of one of the validations)
Updated. |
Thanks for the patience! Just finally have a little extra time to review this :) |
Thanks Mariatta! |
When running cherry_picker --continue we count on being able to get
certain information from the branch's name which cherry_picker
constructed earlier. Verify as best we can that the branch name is one
which cherry_picker could have constructed.
Relies on the changes here:#265
(which does the work of one of the validations)