-
Notifications
You must be signed in to change notification settings - Fork 14
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a simple heuristic to determine if a git_tag is actually a git rev #41
base: main
Are you sure you want to change the base?
Use a simple heuristic to determine if a git_tag is actually a git rev #41
Conversation
If it's 40 characters long hexadecimal string it's very likely a rev Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@@ -851,6 +852,12 @@ def clone_dependency_repo(git: str, git_tag: str, checkout_dir: Path) -> None: | |||
log.debug(f" Repo is not dirty, checking out requested git tag \"{git_tag}\"") | |||
GitInfo.checkout_rev(checkout_dir, git_tag) | |||
else: | |||
# check if git_tag is a 40 character hex string and assume it is a git_rev | |||
if len(git_tag) == 40 and all(character in string.hexdigits for character in git_tag): |
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.
nice expression 👍
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.
What is the main idea behind this PR? From what I understand:
in the dependency.yaml
there is only the property git_tag
, which depending on interpretation might not really be a tag, but could also be a branch or a rev, right?
This change has some heuristic to check if the value contains an SHA-1 commit hash (usually called a rev, I think), and if so it sets internally a git_rev
variable and sets the internal git_tag
variable to None
(but then, directly after that it tries to do a clone_dependency_repo
with git_tag=None
anyway).
Furthermore, I've seen using the abbreviated version of the SHA-1 (only 8 characters) quite a lot, which won't be catched by this.
Could you give some information on what this PR is trying to solve/improve?
It solves the following issues that were reported during
I'll have another look about making it a bit prettier though, was just a relatively quick fix :) |
If it's 40 characters long hexadecimal string it's very likely a rev