-
Notifications
You must be signed in to change notification settings - Fork 384
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
Expose git_merge_file function from libgit2 to repo.rs of git2-rs #635
base: master
Are you sure you want to change the base?
Conversation
cc83880
to
4a39ad8
Compare
4a39ad8
to
57766bc
Compare
libgit2-sys/lib.rs
Outdated
pub len: size_t, | ||
} | ||
|
||
extern "C" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this get merged (hah!) with the general block of function definitions elsewhere in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worrying to make it mess at the beginning so they were put together, now they've been moved to the right places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
src/lib.rs
Outdated
} | ||
} | ||
|
||
impl Into<u32> for FileMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this is typically phrase as From<FileMode> for u32
, but if FileMode
is an enum
I think it would be better to set the discriminants to the raw::*
values and that way enum_value as u32
should work for this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need Into<u32>
trait implementation but just use file_mode as u32
when needed?
src/merge.rs
Outdated
|
||
impl MergeFileResult { | ||
/// Create MergeFileResult from C | ||
pub fn from_raw(raw: raw::git_merge_file_result) -> MergeFileResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this'll want to be unsafe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used unsafe
at the specified lines, is that ok?
impl MergeFileResult {
/// Create MergeFileResult from C
pub fn from_raw(raw: raw::git_merge_file_result) -> MergeFileResult {
let c_str: &CStr = unsafe { CStr::from_ptr(raw.path) }; // unsafe
let str_slice: &str = c_str.to_str().unwrap_or("unknown path");
let path: String = str_slice.to_owned();
let content =
unsafe { slice::from_raw_parts(raw.ptr as *const u8, raw.len as usize).to_vec() }; // unsafe
MergeFileResult {
automergeable: raw.automergeable > 0,
path: Some(path),
mode: raw.mode.into(),
content: Some(content),
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the unsafe
part is that this is taking a raw C structure where the pointers aren't guaranteeed to be valid, so the unsafe
signals "the caller needs to take care of that validity"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, than you!
src/repo.rs
Outdated
let result = MergeFileResult::from_raw(ret); | ||
|
||
// FIXME: need to free???? | ||
raw::git_merge_file_result_free(&mut ret as *mut _); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically this is modeled by MergeFileResult
taking ownership of the git_merge_file_result
and is then responsible for any deallocation necessary, would that be possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So raw::git_merge_file_result_free(&mut ret as *mut _);
is not needed?
@alexcrichton BTW, could you point out how I can fix the issue in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh the windows error I think has to do with the fact that enums in C have a different signededness than other platforms. I think you can probably fix things with some casts?
src/merge.rs
Outdated
|
||
impl MergeFileInput { | ||
/// Create from Repository and IndexEntry | ||
pub fn from(repo: &Repository, index_entry: &IndexEntry) -> MergeFileInput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I've personally shied away from helper functions like this in the past for this crate. This crate tries to stick pretty close to libgit2's own API without adding too many Rust helpers and such. Is there a particular reason though that we should have this when libgit2 doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, it's removed now.
src/merge.rs
Outdated
|
||
impl MergeFileResult { | ||
/// Create MergeFileResult from C | ||
pub fn from_raw(raw: raw::git_merge_file_result) -> MergeFileResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the unsafe
part is that this is taking a raw C structure where the pointers aren't guaranteeed to be valid, so the unsafe
signals "the caller needs to take care of that validity"
src/merge.rs
Outdated
pub mode: FileMode, | ||
|
||
/// The contents of the merge. | ||
pub content: Option<Vec<u8>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think this would probably be best done by (privately) containing raw::git_merge_file_result
and providing accessors for each field. That way Rust doesn't need to allocate anything unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, improved.
src/merge.rs
Outdated
} | ||
|
||
/// File content, text or binary | ||
pub fn content(&mut self, content: Option<Vec<u8>>) -> &mut MergeFileInput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might actually be best to take &[u8]
here and tie the lifetime to MergeFileInput
so that way users won't have to copy data around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, improved.
src/merge.rs
Outdated
path: Option<CString>, | ||
|
||
/// File mode of the conflicted file, or `0` to not merge the mode. | ||
pub mode: Option<FileMode>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields should be private so alterations are forced to go through public functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think it's ok to eschew storing mode
here, we can just store it directly in the raw
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed that
src/repo.rs
Outdated
ours_raw = ptr::null(); | ||
} | ||
if let Some(theirs) = theirs { | ||
theirs_input = MergeFileInput::from(&self, theirs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think it would be best to take theirs: Option<&MergeFileInput>
in this function instead of doing this computation internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, now it accepts Option<&MergeFileInput>
490bafa
to
80bcd0b
Compare
I tried this, but doesn't work, any idea: trait FileModeConvert {
type ValueType;
fn from(mode: Self::ValueType) -> Self;
}
impl FileModeConvert for FileMode {
#[cfg(target_env = "msvc")]
type ValueType = i32;
#[cfg(not(target_env = "msvc"))]
type ValueType = u32;
fn from(mode: Self::ValueType) -> Self {
match mode {
raw::GIT_FILEMODE_UNREADABLE => FileMode::Unreadable,
raw::GIT_FILEMODE_TREE => FileMode::Tree,
raw::GIT_FILEMODE_BLOB => FileMode::Blob,
raw::GIT_FILEMODE_BLOB_EXECUTABLE => FileMode::BlobExecutable,
raw::GIT_FILEMODE_LINK => FileMode::Link,
raw::GIT_FILEMODE_COMMIT => FileMode::Commit,
mode => panic!("unknown file mode: {}", mode),
}
}
} |
80bcd0b
to
c6f0fca
Compare
c6f0fca
to
2b5b1dd
Compare
478bc5b
to
0736d2c
Compare
72aa4af
to
ed71b2d
Compare
@alexcrichton Can we merge this PR now? Thank you! |
No description provided.