Skip to content
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

fix(bug): Using annotated tags now returns the correct commit SHA #598

Merged
merged 7 commits into from
Sep 22, 2019

Conversation

iProgramStuff
Copy link
Contributor

A customer was experiencing an issue when trying to specify commits using an annotated tag. The find_matching_rev function was returning the SHA of the tag instead of getting the SHA of it's associated commit.

This PR fixes that, and adds two tests (one against a lightweight tag, and one against an annotated tag) to ensure it's correctness and keep it correct.

src/utils/vcs.rs Show resolved Hide resolved
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let the Rustaceans review it too.

@iProgramStuff iProgramStuff marked this pull request as ready for review September 18, 2019 15:53
Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. 👍

Just need to simplify the function. Maybe the process of converting Object to a SHA1 can be refactored into its own function.

src/utils/vcs.rs Outdated Show resolved Hide resolved
src/utils/vcs.rs Show resolved Hide resolved
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Thanks for the fix and especially the two tests!

@jan-auer jan-auer merged commit ae8288d into master Sep 22, 2019
@jan-auer jan-auer deleted the fix/SEN-1027 branch September 22, 2019 16:04
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.

4 participants