Change diff renamed files (#1040)

Author: Mifom

CHANGELOG.md

@@ -62,6 +62,7 @@ This is was a immediate followup patch release to `0.20` see [release notes](htt
 - support inspecting annotation of tag ([#1076](https://github.com/extrawurst/gitui/issues/1076))
 - support deleting tag on remote ([#1074](https://github.com/extrawurst/gitui/issues/1074))
 - support git credentials helper (https) ([#800](https://github.com/extrawurst/gitui/issues/800))
+- Correct diff of renamed files [[@Mifom](https://github.com/Mifom)] ([#1038](https://github.com/extrawurst/gitui/issues/1038))
 
 ### Fixed
 - Keep commit message when pre-commit hook fails ([#1035](https://github.com/extrawurst/gitui/issues/1035))

asyncgit/src/diff.rs

@@ -29,8 +29,10 @@ pub enum DiffType {
 ///
 #[derive(Debug, Hash, Clone, PartialEq)]
 pub struct DiffParams {
+	///
+	pub src_path: String,
 	/// path to the file to diff
-	pub path: String,
+	pub dst_path: String,
 	/// what kind of diff
 	pub diff_type: DiffType,
 	/// diff options
@@ -161,26 +163,28 @@ impl AsyncDiff {
 		let res = match params.diff_type {
 			DiffType::Stage => sync::diff::get_diff(
 				repo_path,
-				&params.path,
+				&params.src_path,
+				&params.dst_path,
 				true,
 				Some(params.options),
 			)?,
 			DiffType::WorkDir => sync::diff::get_diff(
 				repo_path,
-				&params.path,
+				&params.src_path,
+				&params.dst_path,
 				false,
 				Some(params.options),
 			)?,
 			DiffType::Commit(id) => sync::diff::get_diff_commit(
 				repo_path,
 				id,
-				params.path.clone(),
+				params.dst_path.clone(),
 				Some(params.options),
 			)?,
 			DiffType::Commits(ids) => sync::diff::get_diff_commits(
 				repo_path,
 				ids,
-				params.path.clone(),
+				params.dst_path.clone(),
 				Some(params.options),
 			)?,
 		};

asyncgit/src/sync/commit_files.rs

@@ -26,21 +26,24 @@ pub fn get_commit_files(
 		get_commit_diff(repo_path, &repo, id, None, None)?
 	};
 
-	let res = diff
-		.deltas()
-		.map(|delta| {
-			let status = StatusItemType::from(delta.status());
-
-			StatusItem {
-				path: delta
-					.new_file()
-					.path()
-					.map(|p| p.to_str().unwrap_or("").to_string())
-					.unwrap_or_default(),
-				status,
-			}
-		})
-		.collect::<Vec<_>>();
+	let res =
+		diff.deltas()
+			.map(|delta| {
+				let status = StatusItemType::from(delta.status());
+
+				StatusItem {
+					old_path: delta.old_file().path().map(|p| {
+						p.to_str().unwrap_or("").to_string()
+					}),
+					new_path: delta
+						.new_file()
+						.path()
+						.map(|p| p.to_str().unwrap_or("").to_string())
+						.unwrap_or_default(),
+					status,
+				}
+			})
+			.collect::<Vec<_>>();
 
 	Ok(res)
 }

asyncgit/src/sync/diff.rs

@@ -10,7 +10,8 @@ use crate::{
 };
 use easy_cast::Conv;
 use git2::{
-	Delta, Diff, DiffDelta, DiffFormat, DiffHunk, Patch, Repository,
+	Delta, Diff, DiffDelta, DiffFindOptions, DiffFormat, DiffHunk,
+	Patch, Repository,
 };
 use scopetime::scope_time;
 use std::{cell::RefCell, fs, path::Path, rc::Rc};
@@ -149,7 +150,8 @@ impl Default for DiffOptions {
 
 pub(crate) fn get_diff_raw<'a>(
 	repo: &'a Repository,
-	p: &str,
+	src: &str,
+	dst: &str,
 	stage: bool,
 	reverse: bool,
 	options: Option<DiffOptions>,
@@ -162,10 +164,11 @@ pub(crate) fn get_diff_raw<'a>(
 		opt.ignore_whitespace(options.ignore_whitespace);
 		opt.interhunk_lines(options.interhunk_lines);
 	}
-	opt.pathspec(p);
+	opt.pathspec(src);
+	opt.pathspec(dst);
 	opt.reverse(reverse);
 
-	let diff = if stage {
+	let mut diff = if stage {
 		// diff against head
 		if let Ok(id) = get_head_repo(repo) {
 			let parent = repo.find_commit(id.into())?;
@@ -189,21 +192,25 @@ pub(crate) fn get_diff_raw<'a>(
 		repo.diff_index_to_workdir(None, Some(&mut opt))?
 	};
 
+	diff.find_similar(Some(
+		DiffFindOptions::new().renames(true).for_untracked(true),
+	))?;
 	Ok(diff)
 }
 
 /// returns diff of a specific file either in `stage` or workdir
 pub fn get_diff(
 	repo_path: &RepoPath,
-	p: &str,
+	src: &str,
+	dst: &str,
 	stage: bool,
 	options: Option<DiffOptions>,
 ) -> Result<FileDiff> {
 	scope_time!("get_diff");
 
 	let repo = repo(repo_path)?;
 	let work_dir = work_dir(&repo)?;
-	let diff = get_diff_raw(&repo, p, stage, false, options)?;
+	let diff = get_diff_raw(&repo, src, dst, stage, false, options)?;
 
 	raw_diff_to_file_diff(&diff, work_dir)
 }
@@ -250,8 +257,8 @@ pub fn get_diff_commits(
 ///
 //TODO: refactor into helper type with the inline closures as dedicated functions
 #[allow(clippy::too_many_lines)]
-fn raw_diff_to_file_diff<'a>(
-	diff: &'a Diff,
+fn raw_diff_to_file_diff(
+	diff: &Diff,
 	work_dir: &Path,
 ) -> Result<FileDiff> {
 	let res = Rc::new(RefCell::new(FileDiff::default()));
@@ -422,7 +429,7 @@ mod tests {
 		},
 	};
 	use std::{
-		fs::{self, File},
+		fs::{self, remove_file, File},
 		io::Write,
 		path::Path,
 	};
@@ -444,8 +451,14 @@ mod tests {
 
 		assert_eq!(get_statuses(repo_path), (1, 0));
 
-		let diff =
-			get_diff(repo_path, "foo/bar.txt", false, None).unwrap();
+		let diff = get_diff(
+			repo_path,
+			"foo/bar.txt",
+			"foo/bar.txt",
+			false,
+			None,
+		)
+		.unwrap();
 
 		assert_eq!(diff.hunks.len(), 1);
 		assert_eq!(&*diff.hunks[0].lines[1].content, "test");
@@ -475,6 +488,7 @@ mod tests {
 		let diff = get_diff(
 			repo_path,
 			file_path.to_str().unwrap(),
+			file_path.to_str().unwrap(),
 			true,
 			None,
 		)
@@ -530,7 +544,8 @@ mod tests {
 		let res = get_status(repo_path, StatusType::WorkingDir, None)
 			.unwrap();
 		assert_eq!(res.len(), 1);
-		assert_eq!(res[0].path, "bar.txt");
+		assert_eq!(res[0].new_path, "bar.txt");
+		assert_eq!(res[0].old_path, Some("bar.txt".to_string()));
 
 		stage_add_file(repo_path, Path::new("bar.txt")).unwrap();
 		assert_eq!(get_statuses(repo_path), (0, 1));
@@ -546,7 +561,8 @@ mod tests {
 		assert_eq!(get_statuses(repo_path), (1, 1));
 
 		let res =
-			get_diff(repo_path, "bar.txt", false, None).unwrap();
+			get_diff(repo_path, "bar.txt", "bar.txt", false, None)
+				.unwrap();
 
 		assert_eq!(res.hunks.len(), 2)
 	}
@@ -568,6 +584,7 @@ mod tests {
 		let diff = get_diff(
 			&sub_path.to_str().unwrap().into(),
 			file_path.to_str().unwrap(),
+			file_path.to_str().unwrap(),
 			false,
 			None,
 		)
@@ -596,6 +613,7 @@ mod tests {
 		let diff = get_diff(
 			repo_path,
 			file_path.to_str().unwrap(),
+			file_path.to_str().unwrap(),
 			false,
 			None,
 		)
@@ -622,6 +640,7 @@ mod tests {
 		let diff = get_diff(
 			repo_path,
 			file_path.to_str().unwrap(),
+			file_path.to_str().unwrap(),
 			false,
 			None,
 		)
@@ -665,4 +684,50 @@ mod tests {
 
 		Ok(())
 	}
+
+	#[test]
+	fn test_rename() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
+
+		assert_eq!(get_statuses(repo_path), (0, 0));
+
+		let file_path = root.join("bar.txt");
+
+		{
+			File::create(&file_path)
+				.unwrap()
+				.write_all(HUNK_A.as_bytes())
+				.unwrap();
+		}
+
+		let res = get_status(repo_path, StatusType::WorkingDir, None)
+			.unwrap();
+		assert_eq!(res.len(), 1);
+		assert_eq!(res[0].new_path, "bar.txt");
+		assert_eq!(res[0].old_path, Some("bar.txt".to_string()));
+
+		stage_add_file(repo_path, Path::new("bar.txt")).unwrap();
+		assert_eq!(get_statuses(repo_path), (0, 1));
+
+		// Move file
+		let other_file_path = root.join("baz.txt");
+		{
+			File::create(&other_file_path)
+				.unwrap()
+				.write_all(HUNK_A.as_bytes())
+				.unwrap();
+			remove_file(&file_path).unwrap();
+		}
+
+		assert_eq!(get_statuses(repo_path), (1, 1));
+
+		let res =
+			get_diff(repo_path, "bar.txt", "baz.txt", false, None)
+				.unwrap();
+
+		assert_eq!(res.hunks.len(), 0)
+	}
 }

asyncgit/src/sync/hunks.rs

@@ -13,14 +13,22 @@ use scopetime::scope_time;
 ///
 pub fn stage_hunk(
 	repo_path: &RepoPath,
-	file_path: &str,
+	src_file_path: &str,
+	dst_file_path: &str,
 	hunk_hash: u64,
 ) -> Result<()> {
 	scope_time!("stage_hunk");
 
 	let repo = repo(repo_path)?;
 
-	let diff = get_diff_raw(&repo, file_path, false, false, None)?;
+	let diff = get_diff_raw(
+		&repo,
+		src_file_path,
+		dst_file_path,
+		false,
+		false,
+		None,
+	)?;
 
 	let mut opt = ApplyOptions::new();
 	opt.hunk_callback(|hunk| {
@@ -45,7 +53,9 @@ pub fn reset_hunk(
 
 	let repo = repo(repo_path)?;
 
-	let diff = get_diff_raw(&repo, file_path, false, false, None)?;
+	let diff = get_diff_raw(
+		&repo, file_path, file_path, false, false, None,
+	)?;
 
 	let hunk_index = find_hunk_index(&diff, hunk_hash);
 	if let Some(hunk_index) = hunk_index {
@@ -57,7 +67,9 @@ pub fn reset_hunk(
 			res
 		});
 
-		let diff = get_diff_raw(&repo, file_path, false, true, None)?;
+		let diff = get_diff_raw(
+			&repo, file_path, file_path, false, true, None,
+		)?;
 
 		repo.apply(&diff, ApplyLocation::WorkDir, Some(&mut opt))?;
 
@@ -96,14 +108,22 @@ fn find_hunk_index(diff: &Diff, hunk_hash: u64) -> Option<usize> {
 ///
 pub fn unstage_hunk(
 	repo_path: &RepoPath,
-	file_path: &str,
+	src_file_path: &str,
+	dst_file_path: &str,
 	hunk_hash: u64,
 ) -> Result<bool> {
 	scope_time!("revert_hunk");
 
 	let repo = repo(repo_path)?;
 
-	let diff = get_diff_raw(&repo, file_path, true, false, None)?;
+	let diff = get_diff_raw(
+		&repo,
+		src_file_path,
+		dst_file_path,
+		true,
+		false,
+		None,
+	)?;
 	let diff_count_positive = diff.deltas().len();
 
 	let hunk_index = find_hunk_index(&diff, hunk_hash);
@@ -112,7 +132,14 @@ pub fn unstage_hunk(
 		Ok,
 	)?;
 
-	let diff = get_diff_raw(&repo, file_path, true, true, None)?;
+	let diff = get_diff_raw(
+		&repo,
+		src_file_path,
+		dst_file_path,
+		true,
+		true,
+		None,
+	)?;
 
 	if diff.deltas().len() != diff_count_positive {
 		return Err(Error::Generic(format!(
@@ -174,6 +201,7 @@ mod tests {
 		let diff = get_diff(
 			sub_path,
 			file_path.to_str().unwrap(),
+			file_path.to_str().unwrap(),
 			false,
 			None,
 		)?;

asyncgit/src/sync/merge.rs

@@ -42,7 +42,7 @@ pub fn abort_pending_state(repo_path: &RepoPath) -> Result<()> {
 	let repo = repo(repo_path)?;
 
 	reset_stage(repo_path, "*")?;
-	reset_workdir(repo_path, "*")?;
+	reset_workdir(repo_path, None, "*")?;
 
 	repo.cleanup_state()?;