-
-
Notifications
You must be signed in to change notification settings - Fork 709
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
check-vcs-permalinks
- Add support for different branch names
#581
Comments
I'm happy expanding the list of branches the issue is it has to not false-positive on:
|
Short shas can collide and as the repository grows, they're probably bound to collide so I don't quite agree, though I think that just matching a hexadecimal number with a sensible minimum number of characters (SHAs can be abbreviated to as low as 4 characters, with the tooling generally shortening to a minimum of 7 so one of those lengths could be used) would work alright here. It could lead to some false negatives but it's not all that likely considering the branch name would have to only contain characters from 0 to f.
This would be covered by the short SHAs since there could either be no maximum or it could be set to 64 which is the max length of SHA256.
Tags are indeed gonna be an issue, as they're not standardized in any way 😕 What do you think about adding a (disabled by default) option for more aggressive matching? Perhaps with a regex for exclusions though that would start to complicate this, relatively simple, check... |
afaik this is irrelevant for git which uses SHA1 --
"contains a ." is what pre-commit uses as a decent heuristic for a tag -- it's of course impossible to get this correct |
Right now yes, but Git team is making a continuous effort towards SHA256 support (Git 2.29 introduced experimental support for it, I'm not following it that closely to know how far they are with it now) so I figured it might be worth supporting it already since I don't think it would cause any issues.
That seems fine, do you think it's worth providing a custom regex option, or is that just an unnecessary complication (until proven it's actually needed anyway)? I can look into PRing this if that's fine with you. |
YAGNI imo |
Not all repositories use
master
for their branch name so it would be great if this hook could support other branch names.Preferably, this would match any blob link that doesn't contain a full hash (so only matching 0-f with len of 40 or 64) but if you don't consider that to be a good solution, then being able to specify a list of branches to match would also work, though it would be a lot more likely to lead to non-permalinks not getting matched.
The text was updated successfully, but these errors were encountered: