Add function: git_merge_file_from_index #1062
Conversation
libgit2-sys/lib.rs
Outdated
| impl Clone for git_merge_file_result { | ||
| fn clone(&self) -> git_merge_file_result { | ||
| *self | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you say why this is manually implemented instead of being derive?
src/repo.rs
Outdated
| opts: Option<&mut MergeFileOptions>, | ||
| ) -> Result<MergeFileResult<'_>, Error> { | ||
| let create_raw_entry = |entry: &IndexEntry| -> Result<raw::git_index_entry, Error> { | ||
| let path = CString::new(&entry.path[..])?; |
There was a problem hiding this comment.
This does not look correct to me because the CString is freed at the end of the closure, and thus the pointer in the git_index_entry is dangling.
Also, would it be possible to factor this code in some way (either a function or macro) to avoid duplicating it? And preferably in a way that would safely manage the allocation here.
src/repo.rs
Outdated
| let mut ret = raw::git_merge_file_result { | ||
| automergeable: 0, | ||
| path: ptr::null_mut(), | ||
| mode: 0, | ||
| ptr: ptr::null_mut(), | ||
| len: 0, | ||
| }; |
There was a problem hiding this comment.
I think it's fine to just use a zeroed struct (at least that's what libgit2 seems to do).
| let mut ret = raw::git_merge_file_result { | |
| automergeable: 0, | |
| path: ptr::null_mut(), | |
| mode: 0, | |
| ptr: ptr::null_mut(), | |
| len: 0, | |
| }; | |
| let mut ret = unsafe { mem::zeroed() }; |
src/merge.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<'repo> std::fmt::Display for MergeFileResult<'repo> { |
There was a problem hiding this comment.
Can this impl Debug instead of Display?
| fn raw(&self) -> raw::git_merge_file_result { | ||
| self.raw | ||
| } |
There was a problem hiding this comment.
This generally doesn't look correct to me, or at least looks concerning. I don't think git_merge_file_result should be copy/clone, since this looks like it is erasing the lifetime.
There was a problem hiding this comment.
The raw func is required to impl Binding? Not sure what you mean here
| /// Information about file-level merging. | ||
| pub struct MergeFileResult<'repo> { | ||
| raw: raw::git_merge_file_result, | ||
| _marker: marker::PhantomData<&'repo str>, |
There was a problem hiding this comment.
Why does this have a PhantomData?
From what I can tell, the struct owns its own pointers (which are freed with git_merge_file_result_free).
There was a problem hiding this comment.
Might be unnecessary, see comment below.
| } | ||
|
|
||
| /// Acquire a pointer to the underlying raw options. | ||
| pub unsafe fn raw(&mut self) -> *const raw::git_merge_file_options { |
There was a problem hiding this comment.
Does this need to be pub?
Also, it seems like this could be done as a Binding impl. Is there any reason that was not done that way?
There was a problem hiding this comment.
Well, there could well be cargo cult issues going on here - I believe I copied this from MergeOptions. I'm not clear on the difference with implementing Binding, but it does need to be pub, since it is used in the merge_commits func in repo.rs. If impl Binding is the preferred way of doing this I can change over to use that?
More generally, I think the reasoning in my head was that structs like AnnotatedCommit or MergeFileResult are returned via libgit2 functions, and could be linked to the repository and should have a lifetime as such. Structs like MergeOptions or MergeFileOptions are created manually and consumed by the libgit2 functions, and as such would not have those same lifetime requirements.
I could be wrong, I am not all that familiar with how things work, but that is why I have copied from AnnotatedCommit to write MergeFileResult, and from MergeOptions to write MergeFileOptions.
Some overlap with #635 but that PR has been open for years.