Conversation
|
You may also want to test for |
|
Sure, added a test for it. |
| cl_git_pass(git_revwalk_add_hide_cb(walk, hide_every_commit_cb, NULL)); | ||
| cl_git_fail(git_revwalk_add_hide_cb(walk, hide_every_commit_cb, NULL)); | ||
| cl_git_pass(git_revwalk_add_hide_cb(walk, hide_every_commit_cb, NULL)); | ||
| cl_git_pass(git_revwalk_add_hide_cb(walk, NULL, NULL)); |
There was a problem hiding this comment.
Good. But you don't verify that the revwalk actually works (e.g. by calling git_revwalk_next()) for this special case.
There was a problem hiding this comment.
Ah, gotcha. Updated again.
pks-t
left a comment
There was a problem hiding this comment.
Thanks for your pull request! Two small things that require change, but otherwise this looks good to me.
| walk->limited = 1; | ||
|
|
||
| if (hide_cb) | ||
| walk->limited = 1; |
There was a problem hiding this comment.
Mh. We don't zero out the struct members, so you'd probably also have to unlimit the revwalk in case there is no hide_cb. So:
walk_limited = !!hide_cb;
There was a problem hiding this comment.
I'm not so sure about that, the limited flag is set in a few other places, for example in git_revwalk_sorting. There, like here, it's only ever set, never unset. I can't find anywhere that explains what it means precisely, so if you say that's correct I'll believe you.
There was a problem hiding this comment.
Oops, you're right. I somehow mistook this change for applying to git_revwalk_reset, but we obviously already set the flag to 0 there. So yeah, your change is indeed perfectly fine: If we didn't limit the revwalk previous to unsetting the hide callback, then we know that we do not put an additional limit to the revwalk. Otherwise, we do.
Since git_revwalk objects are encouraged to be reused, a public interface for changing hide_cb is desirable.
|
/rebuild |
|
Okay, @pks-t, I started to rebuild this pull request. |
|
Thanks a lot for this change, @TheBB! |
Remove the check for existing
hide_cband conditionally set thelimitedflag.Fixes #4887