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

compute_stats: raise exception if tree.count_recursive > MAX_TREE_SIZE #4038

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adelcast
Copy link

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 have included a new sample for the misclassified language:
      • Sample source(s):
        • [URL to each sample source, if applicable]
      • Sample license(s):
    • I have included a change to the heuristics to distinguish my language from others using the same extension.
  • I am changing the source of a syntax highlighting grammar

  • I am adding new or changing current functionality

    • I have added or updated the tests for the new or changed functionality.

On deep repos, liguist fails silently, which is confusing. Raise an
exception instead.

Signed-off-by: Alejandro del Castillo <alejandro.delcastillo@ni.com>
@pchaigno
Copy link
Contributor

LGTM, but this will almost certainly require changes on GitHub's side.

/cc @lildude

@lildude
Copy link
Member

lildude commented Aug 11, 2018

LGTM, but this will almost certainly require changes on GitHub's side.

Yup. I'll need to find time to catch the exception as the code currently expects the empty hash.

@stale
Copy link

stale bot commented Nov 6, 2018

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.

@stale stale bot added the Stale label Nov 6, 2018
@lildude lildude removed the Stale label Nov 6, 2018
@stale
Copy link

stale bot commented Dec 6, 2018

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.

@stale stale bot added the Stale label Dec 6, 2018
@lildude lildude removed the Stale label Dec 6, 2018
@lildude
Copy link
Member

lildude commented Dec 6, 2018

Still don't have the bandwidth for this work on the GitHub side - will label to stop the pings.

@aallrd
Copy link

aallrd commented Jan 23, 2020

Hello,
We bumped into this issue today, it took us some time to figure out what was wrong.
The error message proposed by this PR would have saved us quite a lot of time.
Btw, what's the reason for this hardcoded MAX_TREE_SIZE limitation?
Since there is no way to override it we need to patch the gem code in order to be able to use this project on our repo :/

@pchaigno
Copy link
Contributor

@aallrd I'd welcome a pull request to make this threshold a parameter that defaults to the current value.

@aallrd
Copy link

aallrd commented Jan 23, 2020

@pchaigno would you be open to a PR to simply remove this threshold limitation?

@lildude
Copy link
Member

lildude commented Jan 23, 2020

@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.

@aallrd
Copy link

aallrd commented Jan 23, 2020

@lildude makes sense, thank you for the explanation behind this limitation 👍
I'll see what I can do to add a flag to override this default value.

Copy link

@ehariyatie2478 ehariyatie2478 left a 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
Copy link
Collaborator

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.

@lildude lildude requested a review from a team as a code owner January 13, 2021 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants