Adding tree walk

[mzhang28]

Just wanted to get some code down and start some discussion. I haven't worked much with bindings before so I'd appreciate some tips.

#267

[alexcrichton]

Thanks for the PR! Looks like tests on MSVC may be failing though? Could a bitflags type be used instead perhaps?

Other than that with a small test this should be good to go!

[mzhang28]

What should the callback function return? I actually only put bool cuz the filter function I referenced did but doesn't really make sense in this context.

[alexcrichton]

According to libgit2:

If the callback returns a positive value, the passed entry will be skipped on the traversal (in pre mode). A negative value stops the walk.

Could errors translate to -1 and the boolean be used to skip/not skip?

[neithernut]

Libgit2 exposes a specific error number for such cases (stopping an iteration):
https://github.com/libgit2/libgit2/blob/9913e2df077fd9ed4c0fd5f81075ca5a153cd34b/include/git2/errors.h#L34

[mzhang28]

How about using a custom enum type that's kind of like Option but also includes an option to stop? Seems like there's three kinds of actions that could take place here:

enum TreeWalkResult {
    Ok,
    Skip,
    Stop,
}

Not sure about the exact naming but then we could implement either From<TreeWalkResult> for i32 or Into<i32> for TreeWalkResult and convert it to an integer for git_tree_walk.

Thoughts?

[mzhang28]

@alexcrichton what are your thoughts on this implementation?

[alexcrichton]

I think this shouldn't be necessary in that panic::wrap already checks it

[mzhang28]

ok 👍

[alexcrichton]

Looks good to me! Could some documentation be added to the function to mention the enum types as well?

[alexcrichton]

Thanks!

[asayers]

I'm seeing some strange behaviour with this feature. I have a callback which returns TreeWalkResult::Ok unconditionally, and I expect every node in the tree to be visited. Instead I see a subset of the top-level nodes only, and this subset is not deterministic across runs! Looks like something nasty is happening. Returning 0 from the callback instead gives the expected behaviour.

[mzhang28]

That's not good! I'll take a look after classes today.

…

On Mon, Sep 10, 2018, 10:32 Alex Sayers ***@***.***> wrote: I'm seeing some strange behaviour with this feature. I have a callback which returns TreeWalkResult::Ok unconditionally, and I expect every node in the tree to be visited. Instead I see a subset of the top-level nodes only, and this subset is not deterministic across runs! Looks like something nasty is happening. Returning 0 from the callback instead gives the expected behaviour. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/alexcrichton/git2-rs/pull/343#issuecomment-419956222>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnx-sfN6A5NY0zrujht65AvfQuDcZ4Xks5uZoYEgaJpZM4V3mia> .

[asayers]

Also, reading libgit2/libgit2#4703 makes it seem like we should be passing GIT_EUSER (-7) rather than -1 to abort the traversal. (Unrelated to previous comment.)

EDIT: Just noticed that @neithernut pointed out the same thing.