Skip to content

Commit

Permalink
surf: Improve diff binary detection
Browse files Browse the repository at this point in the history
Iterates over the git2::Diff entries to force libgit2 to do the binary detection.
Also toggles the `skip_binary_check` in the `DiffOptions`.

All this to workaround the libgit2 lazy flag setting[^0]

[^0]: libgit2/libgit2#6637
  • Loading branch information
sebastinez authored and FintanH committed Apr 18, 2024
1 parent 15255f2 commit ea76596
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 18 deletions.
36 changes: 20 additions & 16 deletions radicle-surf/src/diff/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,10 @@ impl<'a> TryFrom<git2::Diff<'a>> for Diff {

let mut diff = Diff::new();

// This allows libgit2 to run the binary detection.
// Reference: <https://github.com/libgit2/libgit2/issues/6637>
git_diff.foreach(&mut |_, _| true, None, None, None)?;

for (idx, delta) in git_diff.deltas().enumerate() {
match delta.status() {
Delta::Added => created(&mut diff, &git_diff, idx, &delta)?,
Expand Down Expand Up @@ -268,10 +272,10 @@ fn created(
let new = DiffFile::try_from(diff_file)?;

let patch = git2::Patch::from_diff(git_diff, idx)?;
if let Some(patch) = patch {
diff.insert_added(path, DiffContent::try_from(patch)?, new);
} else if is_binary {
if is_binary {
diff.insert_added(path, DiffContent::Binary, new);
} else if let Some(patch) = patch {
diff.insert_added(path, DiffContent::try_from(patch)?, new);
} else {
return Err(error::Diff::PatchUnavailable(path));
}
Expand All @@ -293,10 +297,10 @@ fn deleted(
let patch = git2::Patch::from_diff(git_diff, idx)?;
let old = DiffFile::try_from(diff_file)?;

if let Some(patch) = patch {
diff.insert_deleted(path, DiffContent::try_from(patch)?, old);
} else if is_binary {
if is_binary {
diff.insert_deleted(path, DiffContent::Binary, old);
} else if let Some(patch) = patch {
diff.insert_deleted(path, DiffContent::try_from(patch)?, old);
} else {
return Err(error::Diff::PatchUnavailable(path));
}
Expand All @@ -318,12 +322,12 @@ fn modified(
let old = DiffFile::try_from(delta.old_file())?;
let new = DiffFile::try_from(delta.new_file())?;

if let Some(patch) = patch {
diff.insert_modified(path, DiffContent::try_from(patch)?, old, new);
Ok(())
} else if diff_file.is_binary() {
if diff_file.is_binary() {
diff.insert_modified(path, DiffContent::Binary, old, new);
Ok(())
} else if let Some(patch) = patch {
diff.insert_modified(path, DiffContent::try_from(patch)?, old, new);
Ok(())
} else {
Err(error::Diff::PatchUnavailable(path))
}
Expand All @@ -349,10 +353,10 @@ fn renamed(
let old = DiffFile::try_from(delta.old_file())?;
let new = DiffFile::try_from(delta.new_file())?;

if let Some(patch) = patch {
diff.insert_moved(old_path, new_path, old, new, DiffContent::try_from(patch)?);
} else if delta.new_file().is_binary() {
if delta.new_file().is_binary() {
diff.insert_moved(old_path, new_path, old, new, DiffContent::Binary);
} else if let Some(patch) = patch {
diff.insert_moved(old_path, new_path, old, new, DiffContent::try_from(patch)?);
} else {
diff.insert_moved(old_path, new_path, old, new, DiffContent::Empty);
}
Expand All @@ -379,10 +383,10 @@ fn copied(
let old = DiffFile::try_from(delta.old_file())?;
let new = DiffFile::try_from(delta.new_file())?;

if let Some(patch) = patch {
diff.insert_copied(old_path, new_path, old, new, DiffContent::try_from(patch)?);
} else if delta.new_file().is_binary() {
if delta.new_file().is_binary() {
diff.insert_copied(old_path, new_path, old, new, DiffContent::Binary);
} else if let Some(patch) = patch {
diff.insert_copied(old_path, new_path, old, new, DiffContent::try_from(patch)?);
} else {
diff.insert_copied(old_path, new_path, old, new, DiffContent::Empty);
}
Expand Down
3 changes: 1 addition & 2 deletions radicle-surf/src/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,8 +522,7 @@ impl Repository {
let mut opts = git2::DiffOptions::new();
if let Some(path) = path {
opts.pathspec(path.to_string_lossy().to_string());
// We're skipping the binary pass because we won't be inspecting deltas.
opts.skip_binary_check(true);
opts.skip_binary_check(false);
}

let mut diff =
Expand Down

0 comments on commit ea76596

Please sign in to comment.