WIP: Add wrappers for git_index_conflict_{add,remove,get,cleanup}

[fin-ger]

In response to #778 I added wrapper functions for

• git_index_conflict_add
• git_index_conflict_remove
• git_index_conflict_cleanup
• git_index_conflict_get

However, I do not know if this trivial addition is actually safe, as conflicts are already exposed via an iterator. I do not know whether it is safe to modify the index while iterating over the conflicts.

Suggestions welcome!

[alexcrichton]

The general intention of this crate is to be a lightweight binding over libgit2 itself, but this is a pretty heavyweight method. Would it be possible to avoid the movement around of all the data internally?

[fin-ger]

I based this implementation on all the other methods taking a IndexEntry (e.g. Index::add()). This one is particularly long because it takes three IndexEntry parameters. I don't know if the handling of IndexEntrys can be more concise, my intention was more to handle them exactly the same as in the existing methods.

[fin-ger]

Maybe std::mem::transmute can be used here, but I have no clue if git_index_entry is even remotely binary compatible with an IndexEntry especially regarding the path: Vec<u8>. Also, this would be incredibly easy to mess up and hard to debug 🙈

Maybe someone has a better idea 😄

[alexcrichton]

Ah ok, well in that case can the conversion into a git_index_entry be refactored into a helper function instead of being duplicated?

[fin-ger]

Sure, will do so. Should I also refactor all conversions in other methods using IndexEntry?

[alexcrichton]

I guess? Sort of depends, I'm not intimately familiar with all this code.

[fin-ger]

Alright, my solution is to use a helper function which supplies pointers to raw::git_index_entry instances via a callback. This is needed as the construction of a git_index_entry requires the construction of a CString to which the git_index_entry points to. I went for a callback solution as it is (in my opinion) the easiest approach to make sure that the CString is available while using the raw git_index_entry. The procedure is implemented in try_raw_entries()@src/index.rs:92. The implementation is a bit messy and requires the construction of three Vecs. Suggestions welcome!

[alexcrichton]

I think this function would be easier to use as:

impl IndexEntry {
    fn to_raw(&self, storage: &mut Vec<CString>) -> raw::git_index_entry {
        // ...
    }
}

which would avoid the vector, allocations internally, and callback style

[fin-ger]

In my approach the Vecs are necessary as I need multiple entries available in the callback, therefore try_raw_entries can handle multiple entries.

With your approach I think something like this would work:

impl IndexEntry {
    fn to_raw(&self, cpath: &CString) -> raw::git_index_entry {
        // ...
    }
}

Although, I think this would still not ensure that cpath lives longer than the returned value. The usage would look like this:

let cpath = CString::new(&self.path[..])?;
let raw = self.to_raw(&cpath);

My last commit replaces the Vecs with arrays but still contains the callback. If it is safe to have a pointer pointing to the cpath parameter in the return value, I would convert the implementation to that approach. But I'm unsure whether cpath is actually always freed after the return value as it has no lifetime attached to it. I am not that familiar with the internals of memory management in Rust.

[fin-ger]

I implemented your callback-less approach and checked if the tests are passing. Looks good so far, but I'm still not sure whether this API is safe: the return value references data of a parameter but does not carry the lifetime of the parameter to indicate that c_path must live longer than the returned value. Maybe this is not necessary as the c_path binding is always created before the return value in assigned to a variable, I don't know.

[rustbot]

☔ The latest upstream changes (possibly af81314) made this pull request unmergeable. Please resolve the merge conflicts.