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

feat: added check for valid git commit and warning message #3413

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

sahil9001
Copy link
Contributor

Description:

This PR addresses this issue #3394 , wherein we check the validity of git commit and throw the invalid commit message if commit doesn't match the GitHub SHA standard.

Screenshot 2024-10-15 at 8 56 34 PM

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@sahil9001 sahil9001 requested a review from a team as a code owner October 15, 2024 15:26
main.go Outdated
Comment on lines 316 to 318
if len(commit) != 40 {
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe it's necessary to provide the full SHA.

Git is smart enough to figure out what commit you’re referring to if you provide the first few characters of the SHA-1 hash, as long as that partial hash is at least four characters long and unambiguous; that is, no other object in the object database can have a hash that begins with the same prefix.

https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection#_short_sha_1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you check now, I have fixed the issue, also how to link the issue with the PR? I can't see the option to do so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing basic validation is helpful.

However, I think the only way to properly satisfy #3394 is to dispatch a command to Git. (e.g., https://stackoverflow.com/questions/18515488/how-to-check-if-the-commit-exists-in-a-git-repository-by-its-sha-1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right, I have fixed by dispatching this command from the function.

Copy link
Contributor Author

@sahil9001 sahil9001 Oct 15, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-10-16 at 12 24 52 AM

@ahrav
Copy link
Collaborator

ahrav commented Nov 13, 2024

Thanks @sahil9001, sorry for the delayed review here.

@ahrav ahrav merged commit e7ebdb8 into trufflesecurity:main Nov 13, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants