From 2eb8c84dce9be72d2863021f7c41737b31a109ed Mon Sep 17 00:00:00 2001
From: Mike Jerred <mjerred.work@gmail.com>
Date: Tue, 5 Nov 2024 10:52:10 +0000
Subject: [PATCH] fix: review comments

---
 libgit2-sys/lib.rs |  7 +-----
 src/index.rs       | 46 ++++++++++++++++++++++++++++++++++++++
 src/merge.rs       |  2 +-
 src/repo.rs        | 55 +++++-----------------------------------------
 4 files changed, 53 insertions(+), 57 deletions(-)

diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs
index 1b83b05d70..bc4129a1a7 100644
--- a/libgit2-sys/lib.rs
+++ b/libgit2-sys/lib.rs
@@ -1362,7 +1362,7 @@ pub struct git_merge_file_options {
 }
 
 #[repr(C)]
-#[derive(Copy)]
+#[derive(Clone, Copy)]
 pub struct git_merge_file_result {
     pub automergeable: c_uint,
     pub path: *const c_char,
@@ -1370,11 +1370,6 @@ pub struct git_merge_file_result {
     pub ptr: *const c_char,
     pub len: size_t,
 }
-impl Clone for git_merge_file_result {
-    fn clone(&self) -> git_merge_file_result {
-        *self
-    }
-}
 
 git_enum! {
     pub enum git_merge_flag_t {
diff --git a/src/index.rs b/src/index.rs
index 0291d3cb95..fcd82d4e9f 100644
--- a/src/index.rs
+++ b/src/index.rs
@@ -614,6 +614,52 @@ impl Index {
     }
 }
 
+impl IndexEntry {
+    /// Create a raw index entry.
+    ///
+    /// The returned `raw::git_index_entry` contains a pointer to a `CString` path, which is also
+    /// returned because it's lifetime must exceed the lifetime of the `raw::git_index_entry`.
+    pub fn to_raw(&self) -> Result<(raw::git_index_entry, CString), Error> {
+        let path = CString::new(&self.path[..])?;
+
+        // libgit2 encodes the length of the path in the lower bits of the
+        // `flags` entry, so mask those out and recalculate here to ensure we
+        // don't corrupt anything.
+        let mut flags = self.flags & !raw::GIT_INDEX_ENTRY_NAMEMASK;
+
+        if self.path.len() < raw::GIT_INDEX_ENTRY_NAMEMASK as usize {
+            flags |= self.path.len() as u16;
+        } else {
+            flags |= raw::GIT_INDEX_ENTRY_NAMEMASK;
+        }
+
+        unsafe {
+            let raw = raw::git_index_entry {
+                dev: self.dev,
+                ino: self.ino,
+                mode: self.mode,
+                uid: self.uid,
+                gid: self.gid,
+                file_size: self.file_size,
+                id: *self.id.raw(),
+                flags,
+                flags_extended: self.flags_extended,
+                path: path.as_ptr(),
+                mtime: raw::git_index_time {
+                    seconds: self.mtime.seconds(),
+                    nanoseconds: self.mtime.nanoseconds(),
+                },
+                ctime: raw::git_index_time {
+                    seconds: self.ctime.seconds(),
+                    nanoseconds: self.ctime.nanoseconds(),
+                },
+            };
+
+            Ok((raw, path))
+        }
+    }
+}
+
 impl Binding for Index {
     type Raw = *mut raw::git_index;
     unsafe fn from_raw(raw: *mut raw::git_index) -> Index {
diff --git a/src/merge.rs b/src/merge.rs
index 64880603fa..06c3c1e223 100644
--- a/src/merge.rs
+++ b/src/merge.rs
@@ -402,7 +402,7 @@ impl<'repo> Drop for MergeFileResult<'repo> {
     }
 }
 
-impl<'repo> std::fmt::Display for MergeFileResult<'repo> {
+impl<'repo> std::fmt::Debug for MergeFileResult<'repo> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let mut ds = f.debug_struct("MergeFileResult");
         if let Some(path) = &self.path() {
diff --git a/src/repo.rs b/src/repo.rs
index a8e5afb60c..9f19bbe799 100644
--- a/src/repo.rs
+++ b/src/repo.rs
@@ -2516,58 +2516,12 @@ impl Repository {
         theirs: &IndexEntry,
         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[..])?;
-
-            // libgit2 encodes the length of the path in the lower bits of the
-            // `flags` entry, so mask those out and recalculate here to ensure we
-            // don't corrupt anything.
-            let mut flags = entry.flags & !raw::GIT_INDEX_ENTRY_NAMEMASK;
-
-            if entry.path.len() < raw::GIT_INDEX_ENTRY_NAMEMASK as usize {
-                flags |= entry.path.len() as u16;
-            } else {
-                flags |= raw::GIT_INDEX_ENTRY_NAMEMASK;
-            }
-
-            unsafe {
-                let raw = raw::git_index_entry {
-                    dev: entry.dev,
-                    ino: entry.ino,
-                    mode: entry.mode,
-                    uid: entry.uid,
-                    gid: entry.gid,
-                    file_size: entry.file_size,
-                    id: *entry.id.raw(),
-                    flags,
-                    flags_extended: entry.flags_extended,
-                    path: path.as_ptr(),
-                    mtime: raw::git_index_time {
-                        seconds: entry.mtime.seconds(),
-                        nanoseconds: entry.mtime.nanoseconds(),
-                    },
-                    ctime: raw::git_index_time {
-                        seconds: entry.ctime.seconds(),
-                        nanoseconds: entry.ctime.nanoseconds(),
-                    },
-                };
-
-                Ok(raw)
-            }
-        };
-
-        let mut ret = raw::git_merge_file_result {
-            automergeable: 0,
-            path: ptr::null_mut(),
-            mode: 0,
-            ptr: ptr::null_mut(),
-            len: 0,
-        };
-        let ancestor = create_raw_entry(ancestor)?;
-        let ours = create_raw_entry(ours)?;
-        let theirs = create_raw_entry(theirs)?;
+        let (ancestor, _ancestor_path) = ancestor.to_raw()?;
+        let (ours, _ours_path) = ours.to_raw()?;
+        let (theirs, _theirs_path) = theirs.to_raw()?;
 
         unsafe {
+            let mut ret = mem::zeroed();
             try_call!(raw::git_merge_file_from_index(
                 &mut ret,
                 self.raw(),
@@ -4104,6 +4058,7 @@ mod tests {
             .unwrap();
 
         assert!(!merge_file_result.is_automergeable());
+        assert_eq!(merge_file_result.path(), Some("file"));
         assert_eq!(
             String::from_utf8_lossy(merge_file_result.content()).to_string(),
             r"<<<<<<< ours