Add function: git_merge_file_from_index #1062
Conversation
src/merge.rs
Outdated
| 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
There was a problem hiding this comment.
I went ahead and removed the Copy/Clone. My concern is that it could make it a little too easy to copy the struct, which is definitely not something you want to do (since these need to be manually freed, and you don't want to have two copies lying around). It's fine to mark this as unimplemented.
In general, I'm not always certain when it is best to implement Binding versus just making associated functions of the struct itself. Probably would be good to document that someday.
src/merge.rs
Outdated
| /// 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.
src/merge.rs
Outdated
| } | ||
|
|
||
| /// 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.
There was a problem hiding this comment.
Hm, yea. I'm comfortable not including the lifetime here, so I pushed an update to drop it. git_merge_file_result seems to own its data, and I'm not too worried about it gaining internal references in the future.
For now I feel more comfortable not exposing this unless it is needed.
I don't feel comfortable making this copy, since it could accidentally lead to creating multiple copies, which could then be confused as the memory of these needs to be managed.
I feel comfortable not tying the lifetime here, since libgit2 fairly clearly keeps only owned data in git_merge_file_result, without any pointers to anything outside of it.
Some overlap with #635 but that PR has been open for years.