Segfault in `Index::add_all()` with empty `pathspecs` slice and `Some(cb)`

[cpick]

If Index::add_all() is called with an empty (&[]) pathspecs slice and Some(...) callback then it will crash.

If pathspecs is set to anything (eg &[*]) or cb is set to None then it runs without a crash.
I suspect the issue is that it's crashing trying to create the matched_pathspec argument for the callback and that the type of that argument should be changed to be an Option<&[u8]>.

Full steps to reproduce:

$ cargo new git2-crash
$ cd git2-crash
$ echo 'git2 = "0.13.6"' >> Cargo.toml
$ cat >|src/main.rs <<EOF
fn main() {
    let pathspecs: &[&str] = &[];
    git2::Repository::open(".")
        .unwrap()
        .index()
        .unwrap()
        .add_all(pathspecs, Default::default(), Some(&mut |_path, _matched_pathspec| 0))
        .unwrap();
}
EOF
$ cargo run
    Updating crates.io index
...
   Compiling git2-crash v0.1.0 (/Users/cpick/src/delme/git2-crash)
    Finished dev [unoptimized + debuginfo] target(s) in 14.64s
     Running `target/debug/git2-crash`
Segmentation fault: 11

[ehuss]

Yea, it looks like the matched_pathspec should allow NULL as a value, per the behavior of git_pathspec__match which uses NULL if none of the pathspecs match.

[alexcrichton]

Thanks for digging in @ehuss!

I've posted #587 to at least panic instead of segfaulting, but a breaking change will be needed to fully fix this to use Option instead.

[cpick]

I definitely understand the hesitation to introduce a breaking change and panicking is strictly better than segfaulting, but I think there may be some room for improvement on that too.

Since in previous/current versions the segfault is "guaranteed", callers can't be relying on any behavior currently. That gives a bit of freedom to choose a new behavior. Perhaps add_all() could pass in an empty matched_pathspec in this case? Or it could return an Err?

I'm considering making the arguments to this function dynamic and would love to avoid pulling down the process if possible.

[alexcrichton]

Either way is fine by me, I figured it'd be good to get rid of the segfault, but I unfortunately don't have a lot of time to work on this right now.