-
Notifications
You must be signed in to change notification settings - Fork 277
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
Add Tree#count_recursive #464
Changes from 13 commits
b6ab69f
4f629c0
1b636a9
62d6952
0933e8f
8589139
e1b7acf
2c70da0
000d0da
759dcfd
272d487
c31d47d
5d3c4da
44117e0
8a765f8
501f69d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,6 +93,68 @@ static VALUE rb_git_tree_entrycount(VALUE self) | |
return INT2FIX(git_tree_entrycount(tree)); | ||
} | ||
|
||
struct rugged_treecount_cb_payload | ||
{ | ||
int count; | ||
int limit; | ||
}; | ||
|
||
static int rugged__treecount_cb(const char *root, const git_tree_entry *entry, void *data) | ||
{ | ||
struct rugged_treecount_cb_payload *payload = data; | ||
|
||
if (payload->limit >= 0 && payload->count >= payload->limit) | ||
return -1; | ||
else if(git_tree_entry_type(entry) == GIT_OBJ_TREE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Add a whitespace after |
||
return 0; | ||
else { | ||
++(payload->count); | ||
return 1; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately in the libgit2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, good point. Rather than just counting blob, I could count all tree entries (including subtrees). This means that providing a limit will give protection from at least one more DOS vector. Would that cover the case you're thinking of? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regrettably, I think not. The issue is actually hard to compensate for except at the libgit2 level (without introducing a waste of time scanning path data) - namely that you can create a 100,000 level deep tree and not hit your walk limit, but burn up a huge amount of stack space internal to libgit2. Let me look over there a bit... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fwiw MRI cannot stack overflow because it protects the end of the stack in the VM and will gracefully raise a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thank you @vmg for bringing actual knowledge to the table. 😽 |
||
|
||
/* | ||
* call-seq: | ||
* tree.count_recursive(limit=nil) -> count | ||
* | ||
* `limit` - The maximum number of blobs to the count in the repository. | ||
* Rugged will stop walking the tree after `limit` items to avoid long | ||
* execution times. | ||
* | ||
* Return the number of blobs (up to the limit) contained in the tree and | ||
* all subtrees. | ||
*/ | ||
static VALUE rb_git_tree_entrycount_recursive(int argc, VALUE* argv, VALUE self) | ||
{ | ||
git_tree *tree; | ||
Data_Get_Struct(self, git_tree, tree); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have to move this statement below other variable declarations, as this generates a warning. (See the travis build logs). |
||
int error; | ||
struct rugged_treecount_cb_payload payload; | ||
VALUE rb_limit; | ||
|
||
rb_scan_args(argc, argv, "01", &rb_limit); | ||
|
||
payload.limit = -1; | ||
payload.count = 0; | ||
|
||
if (!NIL_P(rb_limit)) { | ||
Check_Type(rb_limit, T_FIXNUM); | ||
payload.limit = FIX2INT(rb_limit); | ||
} | ||
|
||
|
||
error = git_tree_walk(tree, GIT_TREEWALK_PRE, &rugged__treecount_cb, (void *)&payload); | ||
|
||
if(error && giterr_last()->klass == GITERR_CALLBACK) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neat. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Add a whitespace after |
||
giterr_clear(); | ||
error = 0; | ||
} | ||
|
||
rugged_exception_check(error); | ||
|
||
return INT2FIX(payload.count); | ||
} | ||
|
||
/* | ||
* call-seq: | ||
* tree[e] -> entry | ||
|
@@ -813,6 +875,7 @@ void Init_rugged_tree(void) | |
*/ | ||
rb_cRuggedTree = rb_define_class_under(rb_mRugged, "Tree", rb_cRuggedObject); | ||
rb_define_method(rb_cRuggedTree, "count", rb_git_tree_entrycount, 0); | ||
rb_define_method(rb_cRuggedTree, "count_recursive", rb_git_tree_entrycount_recursive, -1); | ||
rb_define_method(rb_cRuggedTree, "length", rb_git_tree_entrycount, 0); | ||
rb_define_method(rb_cRuggedTree, "get_entry", rb_git_tree_get_entry, 1); | ||
rb_define_method(rb_cRuggedTree, "get_entry_by_oid", rb_git_tree_get_entry_by_oid, 1); | ||
|
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.
Can you add braces here? I know we generally don't use braces for single-line bodies, but I feel this looks a bit weird because of the last else condition which spans multiple lines.