Skip to content

revwalk: make mark_unintersting use a loop#1791

Merged
arrbee merged 1 commit intodevelopmentfrom
cmn/revwalk-recursive
Sep 6, 2013
Merged

revwalk: make mark_unintersting use a loop#1791
arrbee merged 1 commit intodevelopmentfrom
cmn/revwalk-recursive

Conversation

@carlosmn
Copy link
Member

Using a recursive function can blow the stack when dealing with long
histories. Use a loop instead to limit the call chain depth.

This fixes #1223.


Repeating the vector pop seems a bit icky, but I'm not sure how to refactor it other than setting i to commit->out_degree so we skip the loop, which also seems a bit icky.

src/revwalk.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This looks like the ideal case to use @arrbee's new array type.

@vmg
Copy link
Member

vmg commented Aug 17, 2013

I don't think the double pops are particularly ugly. Let's move this over to the array_t type and see how it goes. :)

@carlosmn
Copy link
Member Author

Here it is with git_array_t. I'm not sure it makes it better, as we have to make sure we've enough memory by hand this way, and git_array_last returns a pointer to our pointer, as git_array_t expects to be used to store "full" elements but what we need here are the pointers to the commits in the list which is already allocated elsewhere.

src/revwalk.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't pop just pop and return the value here? You can use the coma operator in the macro. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Because git_vector_pop doesn't either, but if that's not good enough reason, I'll make it so.

Using a recursive function can blow the stack when dealing with long
histories. Use a loop instead to limit the call chain depth.

This fixes #1223.
@arrbee
Copy link
Member

arrbee commented Sep 6, 2013

Looks good to me.

We don't need it for this PR, but just to make a note of it, we could at some point add a git_array_calloc that would let you allocate a block of items and then simplify this by just memcpy'ing the parents array in one call. That can be done in the future, though...

arrbee added a commit that referenced this pull request Sep 6, 2013
revwalk: make mark_unintersting use a loop
@arrbee arrbee merged commit 32e4992 into development Sep 6, 2013
@ben ben mentioned this pull request Feb 24, 2014
34 tasks
phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014
revwalk: make mark_unintersting use a loop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants