Validate a branch that we parse when running cherry_picker --continue #266
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.
Could also validate the sha against git (like it's done with a commit in config)
5ffb9e7 to
b88b167
Compare
|
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.
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.
Nitpick: CherryPicker.check_repo() has 99% same code. I'd reuse that.
There was a problem hiding this comment.
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?
|
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 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: python#265 (which does the work of one of the validations)
1b467bc to
4eba21f
Compare
|
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)