-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
compute_stats: raise exception if tree.count_recursive > MAX_TREE_SIZE #4038
compute_stats: raise exception if tree.count_recursive > MAX_TREE_SIZE #4038
Conversation
On deep repos, liguist fails silently, which is confusing. Raise an exception instead. Signed-off-by: Alejandro del Castillo <alejandro.delcastillo@ni.com>
LGTM, but this will almost certainly require changes on GitHub's side. /cc @lildude |
Yup. I'll need to find time to catch the exception as the code currently expects the empty hash. |
This pull request has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this pull request was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions. |
This pull request has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this pull request was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions. |
Still don't have the bandwidth for this work on the GitHub side - will label to stop the pings. |
Hello, |
@aallrd I'd welcome a pull request to make this threshold a parameter that defaults to the current value. |
@pchaigno would you be open to a PR to simply remove this threshold limitation? |
I'd reject that 😁 The limit is in place because of the impact it has on performance and leads to timeouts on github.com. One repo of more than 100k files on one machine is fine. Millions of them like on github.com may become a problem very quickly. |
@lildude makes sense, thank you for the explanation behind this limitation 👍 |
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.
Loom
@@ -132,7 +132,7 @@ def current_tree | |||
MAX_TREE_SIZE = 100_000 | |||
|
|||
def compute_stats(old_commit_oid, cache = nil) | |||
return {} if current_tree.count_recursive(MAX_TREE_SIZE) >= MAX_TREE_SIZE | |||
raise "Your repo tree size exceeds the MAX_LIMIT" if current_tree.count_recursive(MAX_TREE_SIZE) >= MAX_TREE_SIZE |
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.
I'd reword this to be:
raise "Repository tree size exceeds MAX_LIMIT (#{MAX_LIMIT})"
Also, did you mean MAX_TREE_SIZE
instead of MAX_LIMIT
? I don't see the latter being used here.
On deep repos, liguist fails silently, which is confusing. Raise an
exception instead.
Signed-off-by: Alejandro del Castillo alejandro.delcastillo@ni.com
Checklist:
I am associating a language with a new file extension.
I am adding a new language.
I am fixing a misclassified language
I am changing the source of a syntax highlighting grammar
I am adding new or changing current functionality