Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

fin-ger
Copy link

@fin-ger fin-ger commented Nov 30, 2021

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!

/// may be `None` to indicate that that file was not present in the trees during
/// the merge. For example, ancestor_entry may be `None` to indicate that a file
/// was added in both branches and must be resolved.
pub fn conflict_add(
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@fin-ger fin-ger Nov 30, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 😄

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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!

src/index.rs Outdated
// a CString which is owned by the function. To make the pointer to the CString
// valid during usage of raw::git_index_entry, we supply the index entry in a
// callback where pointers to the CString are valid.
fn try_raw_entries(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

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.

2 participants