fix sorting of commits in diff view (#1747)

CHANGELOG.md

@@ -77,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * fix expansion of `~` in `commit.template` ([#1745](https://github.com/extrawurst/gitui/pull/1745))
 * fix hunk (un)staging/reset for # of context lines != 3 ([#1746](https://github.com/extrawurst/gitui/issues/1746))
 * fix delay when opening external editor ([#1506](https://github.com/extrawurst/gitui/issues/1506))
+* fix ordering of commits in diff view [[@Joshix-1](https://github.com/Joshix-1)]([#1747](https://github.com/extrawurst/gitui/issues/1747))
 
 ### Changed
 * Copy full Commit Hash by default [[@AmmarAbouZor](https://github.com/AmmarAbouZor)] ([#1836](https://github.com/extrawurst/gitui/issues/1836))

asyncgit/src/commit_files.rs

@@ -1,6 +1,6 @@
 use crate::{
 	error::Result,
-	sync::{self, CommitId, RepoPath},
+	sync::{self, commit_files::OldNew, CommitId, RepoPath},
 	AsyncGitNotification, StatusItem,
 };
 use crossbeam_channel::Sender;
@@ -36,6 +36,15 @@ impl From<(CommitId, CommitId)> for CommitFilesParams {
 	}
 }
 
+impl From<OldNew<CommitId>> for CommitFilesParams {
+	fn from(old_new: OldNew<CommitId>) -> Self {
+		Self {
+			id: old_new.new,
+			other: Some(old_new.old),
+		}
+	}
+}
+
 ///
 pub struct AsyncCommitFiles {
 	current:

asyncgit/src/diff.rs

@@ -1,7 +1,10 @@
 use crate::{
 	error::Result,
 	hash,
-	sync::{self, diff::DiffOptions, CommitId, RepoPath},
+	sync::{
+		self, commit_files::OldNew, diff::DiffOptions, CommitId,
+		RepoPath,
+	},
 	AsyncGitNotification, FileDiff,
 };
 use crossbeam_channel::Sender;
@@ -17,7 +20,7 @@ use std::{
 #[derive(Debug, Hash, Clone, PartialEq, Eq)]
 pub enum DiffType {
 	/// diff two commits
-	Commits((CommitId, CommitId)),
+	Commits(OldNew<CommitId>),
 	/// diff in a given commit
 	Commit(CommitId),
 	/// diff against staged file

asyncgit/src/sync/commit_files.rs

@@ -8,7 +8,37 @@ use crate::{
 };
 use git2::{Diff, Repository};
 use scopetime::scope_time;
-use std::{cmp::Ordering, collections::HashSet};
+use std::collections::HashSet;
+
+/// struct containing a new and an old version
+#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
+pub struct OldNew<T> {
+	/// The old version
+	pub old: T,
+	/// The new version
+	pub new: T,
+}
+
+/// Sort two commits.
+pub fn sort_commits(
+	repo: &Repository,
+	commits: (CommitId, CommitId),
+) -> Result<OldNew<CommitId>> {
+	if repo.graph_descendant_of(
+		commits.0.get_oid(),
+		commits.1.get_oid(),
+	)? {
+		Ok(OldNew {
+			old: commits.1,
+			new: commits.0,
+		})
+	} else {
+		Ok(OldNew {
+			old: commits.0,
+			new: commits.1,
+		})
+	}
+}
 
 /// get all files that are part of a commit
 pub fn get_commit_files(
@@ -21,7 +51,12 @@ pub fn get_commit_files(
 	let repo = repo(repo_path)?;
 
 	let diff = if let Some(other) = other {
-		get_compare_commits_diff(&repo, (id, other), None, None)?
+		get_compare_commits_diff(
+			&repo,
+			sort_commits(&repo, (id, other))?,
+			None,
+			None,
+		)?
 	} else {
 		get_commit_diff(
 			&repo,
@@ -55,26 +90,20 @@ pub fn get_commit_files(
 #[allow(clippy::needless_pass_by_value)]
 pub fn get_compare_commits_diff(
 	repo: &Repository,
-	ids: (CommitId, CommitId),
+	ids: OldNew<CommitId>,
 	pathspec: Option<String>,
 	options: Option<DiffOptions>,
 ) -> Result<Diff<'_>> {
 	// scope_time!("get_compare_commits_diff");
-
-	let commits = (
-		repo.find_commit(ids.0.into())?,
-		repo.find_commit(ids.1.into())?,
-	);
-
-	let commits = if commits.0.time().cmp(&commits.1.time())
-		== Ordering::Greater
-	{
-		(commits.1, commits.0)
-	} else {
-		commits
+	let commits = OldNew {
+		old: repo.find_commit(ids.old.into())?,
+		new: repo.find_commit(ids.new.into())?,
 	};
 
-	let trees = (commits.0.tree()?, commits.1.tree()?);
+	let trees = OldNew {
+		old: commits.old.tree()?,
+		new: commits.new.tree()?,
+	};
 
 	let mut opts = git2::DiffOptions::new();
 	if let Some(options) = options {
@@ -86,9 +115,9 @@ pub fn get_compare_commits_diff(
 		opts.pathspec(p.clone());
 	}
 
-	let diff = repo.diff_tree_to_tree(
-		Some(&trees.0),
-		Some(&trees.1),
+	let diff: Diff<'_> = repo.diff_tree_to_tree(
+		Some(&trees.old),
+		Some(&trees.new),
 		Some(&mut opts),
 	)?;
 

asyncgit/src/sync/diff.rs

@@ -1,7 +1,9 @@
 //! sync git api for fetching a diff
 
 use super::{
-	commit_files::{get_commit_diff, get_compare_commits_diff},
+	commit_files::{
+		get_commit_diff, get_compare_commits_diff, OldNew,
+	},
 	utils::{get_head_repo, work_dir},
 	CommitId, RepoPath,
 };
@@ -240,7 +242,7 @@ pub fn get_diff_commit(
 /// get file changes of a diff between two commits
 pub fn get_diff_commits(
 	repo_path: &RepoPath,
-	ids: (CommitId, CommitId),
+	ids: OldNew<CommitId>,
 	p: String,
 	options: Option<DiffOptions>,
 ) -> Result<FileDiff> {
@@ -248,12 +250,8 @@ pub fn get_diff_commits(
 
 	let repo = repo(repo_path)?;
 	let work_dir = work_dir(&repo)?;
-	let diff = get_compare_commits_diff(
-		&repo,
-		(ids.0, ids.1),
-		Some(p),
-		options,
-	)?;
+	let diff =
+		get_compare_commits_diff(&repo, ids, Some(p), options)?;
 
 	raw_diff_to_file_diff(&diff, work_dir)
 }

src/components/commit_details/compare_details.rs

@@ -13,7 +13,9 @@ use crate::{
 	ui::style::SharedTheme,
 };
 use anyhow::Result;
-use asyncgit::sync::{self, CommitDetails, CommitId, RepoPathRef};
+use asyncgit::sync::{
+	self, commit_files::OldNew, CommitDetails, CommitId, RepoPathRef,
+};
 use crossterm::event::Event;
 use ratatui::{
 	backend::Backend,
@@ -24,7 +26,7 @@ use ratatui::{
 
 pub struct CompareDetailsComponent {
 	repo: RepoPathRef,
-	data: Option<(CommitDetails, CommitDetails)>,
+	data: Option<OldNew<CommitDetails>>,
 	theme: SharedTheme,
 	focused: bool,
 }
@@ -40,24 +42,20 @@ impl CompareDetailsComponent {
 		}
 	}
 
-	pub fn set_commits(&mut self, ids: Option<(CommitId, CommitId)>) {
+	pub fn set_commits(&mut self, ids: Option<OldNew<CommitId>>) {
 		self.data = ids.and_then(|ids| {
-			let c1 =
-				sync::get_commit_details(&self.repo.borrow(), ids.0)
-					.ok();
-			let c2 =
-				sync::get_commit_details(&self.repo.borrow(), ids.1)
-					.ok();
+			let old = sync::get_commit_details(
+				&self.repo.borrow(),
+				ids.old,
+			)
+			.ok()?;
+			let new = sync::get_commit_details(
+				&self.repo.borrow(),
+				ids.new,
+			)
+			.ok()?;
 
-			c1.and_then(|c1| {
-				c2.map(|c2| {
-					if c1.author.time < c2.author.time {
-						(c1, c2)
-					} else {
-						(c2, c1)
-					}
-				})
-			})
+			Some(OldNew { old, new })
 		});
 	}
 
@@ -122,9 +120,9 @@ impl DrawableComponent for CompareDetailsComponent {
 				dialog_paragraph(
 					&strings::commit::compare_details_info_title(
 						true,
-						data.0.short_hash(),
+						data.old.short_hash(),
 					),
-					Text::from(self.get_commit_text(&data.0)),
+					Text::from(self.get_commit_text(&data.old)),
 					&self.theme,
 					false,
 				),
@@ -135,9 +133,9 @@ impl DrawableComponent for CompareDetailsComponent {
 				dialog_paragraph(
 					&strings::commit::compare_details_info_title(
 						false,
-						data.1.short_hash(),
+						data.new.short_hash(),
 					),
-					Text::from(self.get_commit_text(&data.1)),
+					Text::from(self.get_commit_text(&data.new)),
 					&self.theme,
 					false,
 				),

src/components/commit_details/mod.rs

@@ -14,7 +14,8 @@ use crate::{
 };
 use anyhow::Result;
 use asyncgit::{
-	sync::CommitTags, AsyncCommitFiles, CommitFilesParams,
+	sync::{commit_files::OldNew, CommitTags},
+	AsyncCommitFiles, CommitFilesParams,
 };
 use compare_details::CompareDetailsComponent;
 use crossterm::event::Event;
@@ -81,8 +82,10 @@ impl CommitDetailsComponent {
 			self.file_tree.set_commit(Some(id.id));
 
 			if let Some(other) = id.other {
-				self.compare_details
-					.set_commits(Some((id.id, other)));
+				self.compare_details.set_commits(Some(OldNew {
+					new: id.id,
+					old: other,
+				}));
 			} else {
 				self.single_details
 					.set_commit(Some(id.id), tags.clone());

src/components/compare_commits.rs

@@ -13,7 +13,7 @@ use crate::{
 };
 use anyhow::Result;
 use asyncgit::{
-	sync::{self, CommitId, RepoPathRef},
+	sync::{self, commit_files::OldNew, CommitId, RepoPathRef},
 	AsyncDiff, AsyncGitNotification, CommitFilesParams, DiffParams,
 	DiffType,
 };
@@ -222,16 +222,19 @@ impl CompareCommitsComponent {
 		Ok(())
 	}
 
-	fn get_ids(&self) -> Option<(CommitId, CommitId)> {
+	fn get_ids(&self) -> Option<OldNew<CommitId>> {
 		let other = self
 			.open_request
 			.as_ref()
 			.and_then(|open| open.compare_id);
 
-		self.open_request
-			.as_ref()
-			.map(|open| open.commit_id)
-			.zip(other)
+		let this =
+			self.open_request.as_ref().map(|open| open.commit_id);
+
+		Some(OldNew {
+			old: other?,
+			new: this?,
+		})
 	}
 
 	/// called when any tree component changed selection