From 2572c23144e5fa6b722ab0481d636b05c2de2774 Mon Sep 17 00:00:00 2001 From: Mike Jerred Date: Sun, 23 Jun 2024 18:03:41 +0100 Subject: [PATCH 1/9] feat(merge): add merge_file_from_index --- libgit2-sys/lib.rs | 42 ++++++++- src/lib.rs | 2 +- src/merge.rs | 224 ++++++++++++++++++++++++++++++++++++++++++++- src/repo.rs | 174 ++++++++++++++++++++++++++++++++++- 4 files changed, 435 insertions(+), 7 deletions(-) diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs index 06b75cbc4d..e7bfefc1cb 100644 --- a/libgit2-sys/lib.rs +++ b/libgit2-sys/lib.rs @@ -4,7 +4,7 @@ // This is required to link libz when libssh2-sys is not included. extern crate libz_sys as libz; -use libc::{c_char, c_int, c_uchar, c_uint, c_void, size_t}; +use libc::{c_char, c_int, c_uchar, c_uint, c_ushort, c_void, size_t}; #[cfg(feature = "ssh")] use libssh2_sys as libssh2; use std::ffi::CStr; @@ -1361,6 +1361,32 @@ pub struct git_merge_options { pub file_flags: u32, } +#[repr(C)] +pub struct git_merge_file_options { + pub version: c_uint, + pub ancestor_label: *const c_char, + pub our_label: *const c_char, + pub their_label: *const c_char, + pub favor: git_merge_file_favor_t, + pub flags: u32, + pub marker_size: c_ushort, +} + +#[repr(C)] +#[derive(Copy)] +pub struct git_merge_file_result { + pub automergeable: c_uint, + pub path: *const c_char, + pub mode: c_uint, + 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 { GIT_MERGE_FIND_RENAMES = 1 << 0, @@ -1390,6 +1416,8 @@ git_enum! { GIT_MERGE_FILE_IGNORE_WHITESPACE_EOL = 1 << 5, GIT_MERGE_FILE_DIFF_PATIENCE = 1 << 6, GIT_MERGE_FILE_DIFF_MINIMAL = 1 << 7, + GIT_MERGE_FILE_STYLE_ZDIFF3 = 1 << 8, + GIT_MERGE_FILE_ACCEPT_CONFLICTS = 1 << 9, } } @@ -3395,6 +3423,7 @@ extern "C" { their_tree: *const git_tree, opts: *const git_merge_options, ) -> c_int; + pub fn git_merge_file_options_init(opts: *mut git_merge_file_options, version: c_uint) -> c_int; pub fn git_repository_state_cleanup(repo: *mut git_repository) -> c_int; // merge analysis @@ -3543,6 +3572,17 @@ extern "C" { input_array: *const git_oid, ) -> c_int; + pub fn git_merge_file_from_index( + out: *mut git_merge_file_result, + repo: *mut git_repository, + ancestor: *const git_index_entry, + ours: *const git_index_entry, + theirs: *const git_index_entry, + opts: *const git_merge_file_options, + ) -> c_int; + + pub fn git_merge_file_result_free(file_result: *mut git_merge_file_result); + // pathspec pub fn git_pathspec_free(ps: *mut git_pathspec); pub fn git_pathspec_match_diff( diff --git a/src/lib.rs b/src/lib.rs index fd2db63432..25bc9872e8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -101,7 +101,7 @@ pub use crate::index::{ pub use crate::indexer::{Indexer, IndexerProgress, Progress}; pub use crate::mailmap::Mailmap; pub use crate::mempack::Mempack; -pub use crate::merge::{AnnotatedCommit, MergeOptions}; +pub use crate::merge::{AnnotatedCommit, MergeOptions, MergeFileOptions, MergeFileResult}; pub use crate::message::{ message_prettify, message_trailers_bytes, message_trailers_strs, MessageTrailersBytes, MessageTrailersBytesIterator, MessageTrailersStrs, MessageTrailersStrsIterator, diff --git a/src/merge.rs b/src/merge.rs index 6bd30c10d1..1892638437 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -1,11 +1,14 @@ -use libc::c_uint; +use libc::{c_uint, c_ushort}; +use std::ffi::CString; use std::marker; use std::mem; +use std::ptr; use std::str; use crate::call::Convert; use crate::util::Binding; use crate::{raw, Commit, FileFavor, Oid}; +use crate::IntoCString; /// A structure to represent an annotated commit, the input to merge and rebase. /// @@ -22,6 +25,20 @@ pub struct MergeOptions { raw: raw::git_merge_options, } +/// Options for merging a file. +pub struct MergeFileOptions { + ancestor_label: Option, + our_label: Option, + their_label: Option, + raw: raw::git_merge_file_options, +} + +/// Information about file-level merging. +pub struct MergeFileResult<'repo> { + raw: raw::git_merge_file_result, + _marker: marker::PhantomData<&'repo str>, +} + impl<'repo> AnnotatedCommit<'repo> { /// Gets the commit ID that the given git_annotated_commit refers to pub fn id(&self) -> Oid { @@ -192,3 +209,208 @@ impl<'repo> Drop for AnnotatedCommit<'repo> { unsafe { raw::git_annotated_commit_free(self.raw) } } } + +impl Default for MergeFileOptions { + fn default() -> Self { + Self::new() + } +} + +impl MergeFileOptions { + /// Creates a default set of merge file options. + pub fn new() -> MergeFileOptions { + let mut opts = MergeFileOptions { + ancestor_label: None, + our_label: None, + their_label: None, + raw: unsafe { mem::zeroed() }, + }; + assert_eq!(unsafe { raw::git_merge_file_options_init(&mut opts.raw, 1) }, 0); + opts + } + + /// Label for the ancestor file side of the conflict which will be prepended + /// to labels in diff3-format merge files. + pub fn ancestor_label(&mut self, t: T) -> &mut MergeFileOptions { + self.ancestor_label = Some(t.into_c_string().unwrap()); + + self.raw.ancestor_label = self + .ancestor_label + .as_ref() + .map(|s| s.as_ptr()) + .unwrap_or(ptr::null()); + + self + } + + /// Label for our file side of the conflict which will be prepended to labels + /// in merge files. + pub fn our_label(&mut self, t: T) -> &mut MergeFileOptions { + self.our_label = Some(t.into_c_string().unwrap()); + + self.raw.our_label = self + .our_label + .as_ref() + .map(|s| s.as_ptr()) + .unwrap_or(ptr::null()); + + self + } + + /// Label for their file side of the conflict which will be prepended to labels + /// in merge files. + pub fn their_label(&mut self, t: T) -> &mut MergeFileOptions { + self.their_label = Some(t.into_c_string().unwrap()); + + self.raw.their_label = self + .their_label + .as_ref() + .map(|s| s.as_ptr()) + .unwrap_or(ptr::null()); + + self + } + + /// Specify a side to favor for resolving conflicts + pub fn favor(&mut self, favor: FileFavor) -> &mut MergeFileOptions { + self.raw.favor = favor.convert(); + self + } + + fn flag(&mut self, opt: u32, val: bool) -> &mut MergeFileOptions { + if val { + self.raw.flags |= opt; + } else { + self.raw.flags &= !opt; + } + self + } + + /// Create standard conflicted merge files + pub fn style_standard(&mut self, standard: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_STYLE_MERGE as u32, standard) + } + + /// Create diff3-style file + pub fn style_diff3(&mut self, diff3: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_STYLE_DIFF3 as u32, diff3) + } + + /// Condense non-alphanumeric regions for simplified diff file + pub fn simplify_alnum(&mut self, simplify: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_SIMPLIFY_ALNUM as u32, simplify) + } + + /// Ignore all whitespace + pub fn ignore_whitespace(&mut self, ignore: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE as u32, ignore) + } + + /// Ignore changes in amount of whitespace + pub fn ignore_whitespace_change(&mut self, ignore: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE_CHANGE as u32, ignore) + } + + /// Ignore whitespace at end of line + pub fn ignore_whitespace_eol(&mut self, ignore: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE_EOL as u32, ignore) + } + + /// Use the "patience diff" algorithm + pub fn patience(&mut self, patience: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_DIFF_PATIENCE as u32, patience) + } + + /// Take extra time to find minimal diff + pub fn minimal(&mut self, minimal: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_DIFF_MINIMAL as u32, minimal) + } + + /// Create zdiff3 ("zealous diff3")-style files + pub fn style_zdiff3(&mut self, zdiff3: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_STYLE_ZDIFF3 as u32, zdiff3) + } + + /// Do not produce file conflicts when common regions have changed + pub fn accept_conflicts(&mut self, accept: bool) -> &mut MergeFileOptions { + self.flag(raw::GIT_MERGE_FILE_ACCEPT_CONFLICTS as u32, accept) + } + + /// The size of conflict markers (eg, "<<<<<<<"). Default is 7. + pub fn marker_size(&mut self, size: u16) -> &mut MergeFileOptions { + self.raw.marker_size = size as c_ushort; + self + } + + /// Acquire a pointer to the underlying raw options. + pub unsafe fn raw(&mut self) -> *const raw::git_merge_file_options { + &self.raw as *const _ + } +} + +impl<'repo> MergeFileResult<'repo> { + /// True if the output was automerged, false if the output contains + /// conflict markers. + pub fn is_automergeable(&self) -> bool { + self.raw.automergeable > 0 + } + + /// The path that the resultant merge file should use. + /// + /// returns `None` if a filename conflict would occur, + /// or if the path is not valid utf-8 + pub fn path(&self) -> Option<&str> { + self.path_bytes().and_then(|bytes| str::from_utf8(bytes).ok()) + } + + /// Gets the path as a byte slice. + pub fn path_bytes(&self) -> Option<&[u8]> { + unsafe { crate::opt_bytes(self, self.raw.path) } + } + + /// The mode that the resultant merge file should use. + pub fn mode(&self) -> u32 { + self.raw.mode as u32 + } + + /// The contents of the merge. + pub fn content(&self) -> &'repo [u8] { + unsafe { + std::slice::from_raw_parts( + self.raw.ptr as *const u8, + self.raw.len as usize, + ) + } + } +} + +impl<'repo> Binding for MergeFileResult<'repo> { + type Raw = raw::git_merge_file_result; + unsafe fn from_raw(raw: raw::git_merge_file_result) -> MergeFileResult<'repo> { + MergeFileResult { + raw, + _marker: marker::PhantomData, + } + } + fn raw(&self) -> raw::git_merge_file_result { + self.raw + } +} + +impl<'repo> Drop for MergeFileResult<'repo> { + fn drop(&mut self) { + unsafe { raw::git_merge_file_result_free(&mut self.raw) } + } +} + +impl<'repo> std::fmt::Display 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() { + ds.field("path", path); + } + ds.field("automergeable", &self.is_automergeable()); + ds.field("mode", &self.mode()); + ds.finish() + } +} diff --git a/src/repo.rs b/src/repo.rs index 19f8c1f511..169b931832 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -24,12 +24,12 @@ use crate::{ StashFlags, }; use crate::{ - AnnotatedCommit, MergeAnalysis, MergeOptions, MergePreference, SubmoduleIgnore, - SubmoduleStatus, SubmoduleUpdate, + AnnotatedCommit, MergeAnalysis, MergeOptions, MergeFileOptions, MergeFileResult, MergePreference, + SubmoduleIgnore, SubmoduleStatus, SubmoduleUpdate, }; use crate::{ApplyLocation, ApplyOptions, Rebase, RebaseOptions}; use crate::{Blame, BlameOptions, Reference, References, ResetType, Signature, Submodule}; -use crate::{Blob, BlobWriter, Branch, BranchType, Branches, Commit, Config, Index, Oid, Tree}; +use crate::{Blob, BlobWriter, Branch, BranchType, Branches, Commit, Config, Index, IndexEntry, Oid, Tree}; use crate::{Describe, IntoCString, Reflog, RepositoryInitMode, RevparseMode}; use crate::{DescribeOptions, Diff, DiffOptions, Odb, PackBuilder, TreeBuilder}; use crate::{Note, Notes, ObjectType, Revwalk, Status, StatusOptions, Statuses, Tag, Transaction}; @@ -2566,6 +2566,79 @@ impl Repository { } } + /// Merge two files as they exist in the index, using the given common ancestor + /// as the baseline. + pub fn merge_file_from_index( + &self, + ancestor: &IndexEntry, + ours: &IndexEntry, + theirs: &IndexEntry, + opts: Option<&mut MergeFileOptions>, + ) -> Result, Error> { + let create_raw_entry = |entry: &IndexEntry| -> Result { + 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)?; + + unsafe { + try_call!(raw::git_merge_file_from_index( + &mut ret, + self.raw(), + &ancestor, + &ours, + &theirs, + opts.map(|o| o.raw()).unwrap_or(ptr::null()) + )); + Ok(Binding::from_raw(ret)) + } + } + /// Count the number of unique commits between two commit objects /// /// There is no need for branches containing the commits to have any @@ -3519,7 +3592,7 @@ impl RepositoryInitOptions { #[cfg(test)] mod tests { use crate::build::CheckoutBuilder; - use crate::CherrypickOptions; + use crate::{CherrypickOptions, MergeFileOptions}; use crate::{ ObjectType, Oid, Repository, ResetType, Signature, SubmoduleIgnore, SubmoduleUpdate, }; @@ -4025,6 +4098,99 @@ mod tests { assert_eq!(merge_bases.len(), 2); } + #[test] + fn smoke_merge_file_from_index() { + let (_td, repo) = crate::test::repo_init(); + + let head_commit = { + let head = t!(repo.head()).target().unwrap(); + t!(repo.find_commit(head)) + }; + + let file_path = Path::new("file"); + let author = t!(Signature::now("committer", "committer@email")); + + let base_commit = { + t!(fs::write(repo.workdir().unwrap().join(&file_path), "base")); + let mut index = t!(repo.index()); + t!(index.add_path(&file_path)); + let tree_id = t!(index.write_tree()); + let tree = t!(repo.find_tree(tree_id)); + + let commit_id = t!(repo.commit( + Some("HEAD"), + &author, + &author, + r"Add file with contents 'base'", + &tree, + &[&head_commit], + )); + t!(repo.find_commit(commit_id)) + }; + + let foo_commit = { + t!(fs::write(repo.workdir().unwrap().join(&file_path), "foo")); + let mut index = t!(repo.index()); + t!(index.add_path(&file_path)); + let tree_id = t!(index.write_tree()); + let tree = t!(repo.find_tree(tree_id)); + + let commit_id = t!(repo.commit( + Some("refs/heads/foo"), + &author, + &author, + r"Update file with contents 'foo'", + &tree, + &[&base_commit], + )); + t!(repo.find_commit(commit_id)) + }; + + let bar_commit = { + t!(fs::write(repo.workdir().unwrap().join(&file_path), "bar")); + let mut index = t!(repo.index()); + t!(index.add_path(&file_path)); + let tree_id = t!(index.write_tree()); + let tree = t!(repo.find_tree(tree_id)); + + let commit_id = t!(repo.commit( + Some("refs/heads/bar"), + &author, + &author, + r"Update file with contents 'bar'", + &tree, + &[&base_commit], + )); + t!(repo.find_commit(commit_id)) + }; + + let index = t!(repo.merge_commits(&foo_commit, &bar_commit, None)); + + let base = index.get_path(file_path, 1).unwrap(); + let ours = index.get_path(file_path, 2).unwrap(); + let theirs = index.get_path(file_path, 3).unwrap(); + + let mut opts = MergeFileOptions::new(); + opts.ancestor_label("ancestor"); + opts.our_label("ours"); + opts.their_label("theirs"); + opts.style_diff3(true); + let merge_file_result = repo.merge_file_from_index(&base, &ours, &theirs, Some(&mut opts)).unwrap(); + + assert!(!merge_file_result.is_automergeable()); + assert_eq!( + String::from_utf8_lossy(merge_file_result.content()).to_string(), +r"<<<<<<< ours +foo +||||||| ancestor +base +======= +bar +>>>>>>> theirs +", + ); + } + #[test] fn smoke_revparse_ext() { let (_td, repo) = graph_repo_init(); From e80d1ab0152207454ba4bf897d54f36cdc49c2ec Mon Sep 17 00:00:00 2001 From: Mike Jerred Date: Sun, 23 Jun 2024 18:35:04 +0100 Subject: [PATCH 2/9] style: fix code formatting --- libgit2-sys/lib.rs | 3 ++- src/lib.rs | 2 +- src/merge.rs | 17 ++++++++--------- src/repo.rs | 14 +++++++++----- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs index e7bfefc1cb..50939f0226 100644 --- a/libgit2-sys/lib.rs +++ b/libgit2-sys/lib.rs @@ -3423,7 +3423,8 @@ extern "C" { their_tree: *const git_tree, opts: *const git_merge_options, ) -> c_int; - pub fn git_merge_file_options_init(opts: *mut git_merge_file_options, version: c_uint) -> c_int; + pub fn git_merge_file_options_init(opts: *mut git_merge_file_options, version: c_uint) + -> c_int; pub fn git_repository_state_cleanup(repo: *mut git_repository) -> c_int; // merge analysis diff --git a/src/lib.rs b/src/lib.rs index 25bc9872e8..4b26b8c023 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -101,7 +101,7 @@ pub use crate::index::{ pub use crate::indexer::{Indexer, IndexerProgress, Progress}; pub use crate::mailmap::Mailmap; pub use crate::mempack::Mempack; -pub use crate::merge::{AnnotatedCommit, MergeOptions, MergeFileOptions, MergeFileResult}; +pub use crate::merge::{AnnotatedCommit, MergeFileOptions, MergeFileResult, MergeOptions}; pub use crate::message::{ message_prettify, message_trailers_bytes, message_trailers_strs, MessageTrailersBytes, MessageTrailersBytesIterator, MessageTrailersStrs, MessageTrailersStrsIterator, diff --git a/src/merge.rs b/src/merge.rs index 1892638437..64880603fa 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -7,8 +7,8 @@ use std::str; use crate::call::Convert; use crate::util::Binding; -use crate::{raw, Commit, FileFavor, Oid}; use crate::IntoCString; +use crate::{raw, Commit, FileFavor, Oid}; /// A structure to represent an annotated commit, the input to merge and rebase. /// @@ -225,7 +225,10 @@ impl MergeFileOptions { their_label: None, raw: unsafe { mem::zeroed() }, }; - assert_eq!(unsafe { raw::git_merge_file_options_init(&mut opts.raw, 1) }, 0); + assert_eq!( + unsafe { raw::git_merge_file_options_init(&mut opts.raw, 1) }, + 0 + ); opts } @@ -360,7 +363,8 @@ impl<'repo> MergeFileResult<'repo> { /// returns `None` if a filename conflict would occur, /// or if the path is not valid utf-8 pub fn path(&self) -> Option<&str> { - self.path_bytes().and_then(|bytes| str::from_utf8(bytes).ok()) + self.path_bytes() + .and_then(|bytes| str::from_utf8(bytes).ok()) } /// Gets the path as a byte slice. @@ -375,12 +379,7 @@ impl<'repo> MergeFileResult<'repo> { /// The contents of the merge. pub fn content(&self) -> &'repo [u8] { - unsafe { - std::slice::from_raw_parts( - self.raw.ptr as *const u8, - self.raw.len as usize, - ) - } + unsafe { std::slice::from_raw_parts(self.raw.ptr as *const u8, self.raw.len as usize) } } } diff --git a/src/repo.rs b/src/repo.rs index 169b931832..a1a3285b44 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -24,12 +24,14 @@ use crate::{ StashFlags, }; use crate::{ - AnnotatedCommit, MergeAnalysis, MergeOptions, MergeFileOptions, MergeFileResult, MergePreference, - SubmoduleIgnore, SubmoduleStatus, SubmoduleUpdate, + AnnotatedCommit, MergeAnalysis, MergeFileOptions, MergeFileResult, MergeOptions, + MergePreference, SubmoduleIgnore, SubmoduleStatus, SubmoduleUpdate, }; use crate::{ApplyLocation, ApplyOptions, Rebase, RebaseOptions}; use crate::{Blame, BlameOptions, Reference, References, ResetType, Signature, Submodule}; -use crate::{Blob, BlobWriter, Branch, BranchType, Branches, Commit, Config, Index, IndexEntry, Oid, Tree}; +use crate::{ + Blob, BlobWriter, Branch, BranchType, Branches, Commit, Config, Index, IndexEntry, Oid, Tree, +}; use crate::{Describe, IntoCString, Reflog, RepositoryInitMode, RevparseMode}; use crate::{DescribeOptions, Diff, DiffOptions, Odb, PackBuilder, TreeBuilder}; use crate::{Note, Notes, ObjectType, Revwalk, Status, StatusOptions, Statuses, Tag, Transaction}; @@ -4175,12 +4177,14 @@ mod tests { opts.our_label("ours"); opts.their_label("theirs"); opts.style_diff3(true); - let merge_file_result = repo.merge_file_from_index(&base, &ours, &theirs, Some(&mut opts)).unwrap(); + let merge_file_result = repo + .merge_file_from_index(&base, &ours, &theirs, Some(&mut opts)) + .unwrap(); assert!(!merge_file_result.is_automergeable()); assert_eq!( String::from_utf8_lossy(merge_file_result.content()).to_string(), -r"<<<<<<< ours + r"<<<<<<< ours foo ||||||| ancestor base From 742c6305c31aa8bc3c3d627f64febd6efcb81448 Mon Sep 17 00:00:00 2001 From: Mike Jerred Date: Tue, 5 Nov 2024 10:52:10 +0000 Subject: [PATCH 3/9] 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 50939f0226..05f71a4ad3 100644 --- a/libgit2-sys/lib.rs +++ b/libgit2-sys/lib.rs @@ -1373,7 +1373,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, @@ -1381,11 +1381,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 5625ba91ac..1f0e79a104 100644 --- a/src/index.rs +++ b/src/index.rs @@ -656,6 +656,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 a1a3285b44..1c7b8c19c9 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -2577,58 +2577,12 @@ impl Repository { theirs: &IndexEntry, opts: Option<&mut MergeFileOptions>, ) -> Result, Error> { - let create_raw_entry = |entry: &IndexEntry| -> Result { - 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(), @@ -4182,6 +4136,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 From b76c8becb0d760bcdcdf230ac90773c312c3edd8 Mon Sep 17 00:00:00 2001 From: Mike Jerred Date: Sun, 5 Jan 2025 09:58:53 +0000 Subject: [PATCH 4/9] fix: use correct typings for git_merge_file_flag_t --- libgit2-sys/lib.rs | 2 +- src/merge.rs | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs index 05f71a4ad3..065be08596 100644 --- a/libgit2-sys/lib.rs +++ b/libgit2-sys/lib.rs @@ -1368,7 +1368,7 @@ pub struct git_merge_file_options { pub our_label: *const c_char, pub their_label: *const c_char, pub favor: git_merge_file_favor_t, - pub flags: u32, + pub flags: git_merge_file_flag_t, pub marker_size: c_ushort, } diff --git a/src/merge.rs b/src/merge.rs index 06c3c1e223..6329b8f597 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -280,7 +280,7 @@ impl MergeFileOptions { self } - fn flag(&mut self, opt: u32, val: bool) -> &mut MergeFileOptions { + fn flag(&mut self, opt: raw::git_merge_file_flag_t, val: bool) -> &mut MergeFileOptions { if val { self.raw.flags |= opt; } else { @@ -291,52 +291,52 @@ impl MergeFileOptions { /// Create standard conflicted merge files pub fn style_standard(&mut self, standard: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_STYLE_MERGE as u32, standard) + self.flag(raw::GIT_MERGE_FILE_STYLE_MERGE, standard) } /// Create diff3-style file pub fn style_diff3(&mut self, diff3: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_STYLE_DIFF3 as u32, diff3) + self.flag(raw::GIT_MERGE_FILE_STYLE_DIFF3, diff3) } /// Condense non-alphanumeric regions for simplified diff file pub fn simplify_alnum(&mut self, simplify: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_SIMPLIFY_ALNUM as u32, simplify) + self.flag(raw::GIT_MERGE_FILE_SIMPLIFY_ALNUM, simplify) } /// Ignore all whitespace pub fn ignore_whitespace(&mut self, ignore: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE as u32, ignore) + self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE, ignore) } /// Ignore changes in amount of whitespace pub fn ignore_whitespace_change(&mut self, ignore: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE_CHANGE as u32, ignore) + self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE_CHANGE, ignore) } /// Ignore whitespace at end of line pub fn ignore_whitespace_eol(&mut self, ignore: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE_EOL as u32, ignore) + self.flag(raw::GIT_MERGE_FILE_IGNORE_WHITESPACE_EOL, ignore) } /// Use the "patience diff" algorithm pub fn patience(&mut self, patience: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_DIFF_PATIENCE as u32, patience) + self.flag(raw::GIT_MERGE_FILE_DIFF_PATIENCE, patience) } /// Take extra time to find minimal diff pub fn minimal(&mut self, minimal: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_DIFF_MINIMAL as u32, minimal) + self.flag(raw::GIT_MERGE_FILE_DIFF_MINIMAL, minimal) } /// Create zdiff3 ("zealous diff3")-style files pub fn style_zdiff3(&mut self, zdiff3: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_STYLE_ZDIFF3 as u32, zdiff3) + self.flag(raw::GIT_MERGE_FILE_STYLE_ZDIFF3, zdiff3) } /// Do not produce file conflicts when common regions have changed pub fn accept_conflicts(&mut self, accept: bool) -> &mut MergeFileOptions { - self.flag(raw::GIT_MERGE_FILE_ACCEPT_CONFLICTS as u32, accept) + self.flag(raw::GIT_MERGE_FILE_ACCEPT_CONFLICTS, accept) } /// The size of conflict markers (eg, "<<<<<<<"). Default is 7. From e1e933b0f4300d48d6f56318d470c6422d0ff362 Mon Sep 17 00:00:00 2001 From: Mike Jerred Date: Sun, 5 Jan 2025 10:26:36 +0000 Subject: [PATCH 5/9] fix: mark to_raw as `unsafe` --- src/index.rs | 2 +- src/repo.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/index.rs b/src/index.rs index 1f0e79a104..c0d9294520 100644 --- a/src/index.rs +++ b/src/index.rs @@ -661,7 +661,7 @@ impl IndexEntry { /// /// 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> { + pub(crate) unsafe 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 diff --git a/src/repo.rs b/src/repo.rs index 1c7b8c19c9..b03aaba350 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -2577,11 +2577,11 @@ impl Repository { theirs: &IndexEntry, opts: Option<&mut MergeFileOptions>, ) -> Result, Error> { - let (ancestor, _ancestor_path) = ancestor.to_raw()?; - let (ours, _ours_path) = ours.to_raw()?; - let (theirs, _theirs_path) = theirs.to_raw()?; - unsafe { + let (ancestor, _ancestor_path) = ancestor.to_raw()?; + let (ours, _ours_path) = ours.to_raw()?; + let (theirs, _theirs_path) = theirs.to_raw()?; + let mut ret = mem::zeroed(); try_call!(raw::git_merge_file_from_index( &mut ret, From 3cc3e25a9d6896690a3f50f1d6dfdb55d6f66f4f Mon Sep 17 00:00:00 2001 From: Mike Jerred Date: Sun, 5 Jan 2025 10:49:16 +0000 Subject: [PATCH 6/9] fix: type mismatch error --- libgit2-sys/lib.rs | 2 +- src/merge.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs index 065be08596..05f71a4ad3 100644 --- a/libgit2-sys/lib.rs +++ b/libgit2-sys/lib.rs @@ -1368,7 +1368,7 @@ pub struct git_merge_file_options { pub our_label: *const c_char, pub their_label: *const c_char, pub favor: git_merge_file_favor_t, - pub flags: git_merge_file_flag_t, + pub flags: u32, pub marker_size: c_ushort, } diff --git a/src/merge.rs b/src/merge.rs index 6329b8f597..69ebd7ca11 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -282,9 +282,9 @@ impl MergeFileOptions { fn flag(&mut self, opt: raw::git_merge_file_flag_t, val: bool) -> &mut MergeFileOptions { if val { - self.raw.flags |= opt; + self.raw.flags |= opt as u32; } else { - self.raw.flags &= !opt; + self.raw.flags &= !opt as u32; } self } From 8381b72453f6b7c38ec3e1c8fed041f3202e17d5 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 17 Mar 2025 14:26:14 -0700 Subject: [PATCH 7/9] Make MergeFileOptions::raw pub(crate) For now I feel more comfortable not exposing this unless it is needed. --- src/merge.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/merge.rs b/src/merge.rs index 69ebd7ca11..bb6ffabd0e 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -346,8 +346,11 @@ impl MergeFileOptions { } /// Acquire a pointer to the underlying raw options. - pub unsafe fn raw(&mut self) -> *const raw::git_merge_file_options { - &self.raw as *const _ + /// + /// # Safety + /// The pointer used here (or its contents) should not outlive self. + pub(crate) unsafe fn raw(&mut self) -> *const raw::git_merge_file_options { + &self.raw } } From 197106b3606f3bfb8a02d5a6c7f422d960334723 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 17 Mar 2025 14:28:07 -0700 Subject: [PATCH 8/9] Drop Copy/Clone from git_merge_file_result 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. --- libgit2-sys/lib.rs | 1 - src/merge.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs index 05f71a4ad3..5b7ae56bd8 100644 --- a/libgit2-sys/lib.rs +++ b/libgit2-sys/lib.rs @@ -1373,7 +1373,6 @@ pub struct git_merge_file_options { } #[repr(C)] -#[derive(Clone, Copy)] pub struct git_merge_file_result { pub automergeable: c_uint, pub path: *const c_char, diff --git a/src/merge.rs b/src/merge.rs index bb6ffabd0e..d91fbed36d 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -395,7 +395,7 @@ impl<'repo> Binding for MergeFileResult<'repo> { } } fn raw(&self) -> raw::git_merge_file_result { - self.raw + unimplemented!() } } From d1b40aa0b7b8f53da56acd49fc99cd1adfa6f644 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 17 Mar 2025 14:30:12 -0700 Subject: [PATCH 9/9] Drop lifetime for MergeFileResult 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. --- src/merge.rs | 20 ++++++++------------ src/repo.rs | 2 +- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/merge.rs b/src/merge.rs index d91fbed36d..bdb32970a9 100644 --- a/src/merge.rs +++ b/src/merge.rs @@ -34,9 +34,8 @@ pub struct MergeFileOptions { } /// Information about file-level merging. -pub struct MergeFileResult<'repo> { +pub struct MergeFileResult { raw: raw::git_merge_file_result, - _marker: marker::PhantomData<&'repo str>, } impl<'repo> AnnotatedCommit<'repo> { @@ -354,7 +353,7 @@ impl MergeFileOptions { } } -impl<'repo> MergeFileResult<'repo> { +impl MergeFileResult { /// True if the output was automerged, false if the output contains /// conflict markers. pub fn is_automergeable(&self) -> bool { @@ -381,31 +380,28 @@ impl<'repo> MergeFileResult<'repo> { } /// The contents of the merge. - pub fn content(&self) -> &'repo [u8] { + pub fn content(&self) -> &[u8] { unsafe { std::slice::from_raw_parts(self.raw.ptr as *const u8, self.raw.len as usize) } } } -impl<'repo> Binding for MergeFileResult<'repo> { +impl Binding for MergeFileResult { type Raw = raw::git_merge_file_result; - unsafe fn from_raw(raw: raw::git_merge_file_result) -> MergeFileResult<'repo> { - MergeFileResult { - raw, - _marker: marker::PhantomData, - } + unsafe fn from_raw(raw: raw::git_merge_file_result) -> MergeFileResult { + MergeFileResult { raw } } fn raw(&self) -> raw::git_merge_file_result { unimplemented!() } } -impl<'repo> Drop for MergeFileResult<'repo> { +impl Drop for MergeFileResult { fn drop(&mut self) { unsafe { raw::git_merge_file_result_free(&mut self.raw) } } } -impl<'repo> std::fmt::Debug for MergeFileResult<'repo> { +impl std::fmt::Debug for MergeFileResult { 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 b03aaba350..464530332e 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -2576,7 +2576,7 @@ impl Repository { ours: &IndexEntry, theirs: &IndexEntry, opts: Option<&mut MergeFileOptions>, - ) -> Result, Error> { + ) -> Result { unsafe { let (ancestor, _ancestor_path) = ancestor.to_raw()?; let (ours, _ours_path) = ours.to_raw()?;