implement reverting commit from revlog (#1057)

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Added
+- allow reverting a commit from the commit log ([#927](https://github.com/extrawurst/gitui/issues/927))
+
 ### Fixed
 - Keep commit message when pre-commit hook fails ([#1035](https://github.com/extrawurst/gitui/issues/1035))
 - honor `pushurl` when checking credentials for pushing ([#953](https://github.com/extrawurst/gitui/issues/953))

README.md

@@ -79,7 +79,6 @@ These are the high level goals before calling out `1.0`:
 * notify-based change detection ([#1](https://github.com/extrawurst/gitui/issues/1))
 * interactive rebase ([#32](https://github.com/extrawurst/gitui/issues/32))
 * popup history and back button ([#846](https://github.com/extrawurst/gitui/issues/846))
-* support reverting a commit ([#927](https://github.com/extrawurst/gitui/issues/927))
 
 ## 5. <a name="limitations"></a> Known Limitations <small><sup>[Top ▲](#table-of-contents)</sup></small>
 

asyncgit/src/sync/commit_revert.rs

@@ -0,0 +1,51 @@
+use super::{CommitId, RepoPath};
+use crate::{
+	error::Result,
+	sync::{repository::repo, utils::read_file},
+};
+use scopetime::scope_time;
+
+const GIT_REVERT_HEAD_FILE: &str = "REVERT_HEAD";
+
+///
+pub fn revert_commit(
+	repo_path: &RepoPath,
+	commit: CommitId,
+) -> Result<()> {
+	scope_time!("revert");
+
+	let repo = repo(repo_path)?;
+
+	let commit = repo.find_commit(commit.into())?;
+
+	repo.revert(&commit, None)?;
+
+	Ok(())
+}
+
+///
+pub fn revert_head(repo_path: &RepoPath) -> Result<CommitId> {
+	scope_time!("revert_head");
+
+	let path = repo(repo_path)?.path().join(GIT_REVERT_HEAD_FILE);
+
+	let file_content = read_file(&path)?;
+
+	let id = git2::Oid::from_str(file_content.trim())?;
+
+	Ok(id.into())
+}
+
+///
+pub fn commit_revert(
+	repo_path: &RepoPath,
+	msg: &str,
+) -> Result<CommitId> {
+	scope_time!("commit_revert");
+
+	let id = crate::sync::commit(repo_path, msg)?;
+
+	repo(repo_path)?.cleanup_state()?;
+
+	Ok(id)
+}

asyncgit/src/sync/merge.rs

@@ -36,8 +36,8 @@ pub fn mergehead_ids(repo_path: &RepoPath) -> Result<Vec<CommitId>> {
 /// * reset all staged changes,
 /// * revert all changes in workdir
 /// * cleanup repo merge state
-pub fn abort_merge(repo_path: &RepoPath) -> Result<()> {
+pub fn abort_pending_state(repo_path: &RepoPath) -> Result<()> {
-	scope_time!("cleanup_state");
+	scope_time!("abort_pending_state");
 
 	let repo = repo(repo_path)?;
 

asyncgit/src/sync/mod.rs

@@ -8,6 +8,7 @@ pub mod branch;
 mod commit;
 mod commit_details;
 mod commit_files;
+mod commit_revert;
 mod commits_info;
 mod config;
 pub mod cred;
@@ -44,6 +45,7 @@ pub use commit_details::{
 	get_commit_details, CommitDetails, CommitMessage, CommitSignature,
 };
 pub use commit_files::get_commit_files;
+pub use commit_revert::{commit_revert, revert_commit, revert_head};
 pub use commits_info::{
 	get_commit_info, get_commits_info, CommitId, CommitInfo,
 };
@@ -60,9 +62,9 @@ pub use hunks::{reset_hunk, stage_hunk, unstage_hunk};
 pub use ignore::add_to_ignore;
 pub use logwalker::{LogWalker, LogWalkerFilter};
 pub use merge::{
-	abort_merge, abort_pending_rebase, continue_pending_rebase,
+	abort_pending_rebase, abort_pending_state,
-	merge_branch, merge_commit, merge_msg, mergehead_ids,
+	continue_pending_rebase, merge_branch, merge_commit, merge_msg,
-	rebase_progress,
+	mergehead_ids, rebase_progress,
 };
 pub use rebase::rebase_branch;
 pub use remotes::{

asyncgit/src/sync/state.rs

@@ -13,6 +13,8 @@ pub enum RepoState {
 	///
 	Rebase,
 	///
+	Revert,
+	///
 	Other,
 }
 
@@ -21,6 +23,7 @@ impl From<RepositoryState> for RepoState {
 		match state {
 			RepositoryState::Clean => Self::Clean,
 			RepositoryState::Merge => Self::Merge,
+			RepositoryState::Revert => Self::Revert,
 			RepositoryState::RebaseMerge => Self::Rebase,
 			_ => {
 				log::warn!("state not supported yet: {:?}", state);
@@ -38,7 +41,5 @@ pub fn repo_state(repo_path: &RepoPath) -> Result<RepoState> {
 
 	let state = repo.state();
 
-	// dbg!(&state);
-
 	Ok(state.into())
 }

asyncgit/src/sync/utils.rs

@@ -187,6 +187,17 @@ pub(crate) fn repo_write_file(
 	Ok(())
 }
 
+///
+pub fn read_file(path: &Path) -> Result<String> {
+	use std::io::Read;
+
+	let mut file = File::open(path)?;
+	let mut buffer = Vec::new();
+	file.read_to_end(&mut buffer)?;
+
+	Ok(String::from_utf8(buffer)?)
+}
+
 #[cfg(test)]
 pub(crate) fn repo_read_file(
 	repo: &Repository,

src/app.rs

@@ -701,7 +701,7 @@ impl App {
 			InternalEvent::Tags => {
 				self.tags_popup.open()?;
 			}
-			InternalEvent::TabSwitch => self.set_tab(0)?,
+			InternalEvent::TabSwitchStatus => self.set_tab(0)?,
 			InternalEvent::InspectCommit(id, tags) => {
 				self.inspect_commit_popup.open(id, tags)?;
 				flags
@@ -879,8 +879,8 @@ impl App {
 				self.pull_popup.try_conflict_free_merge(rebase);
 				flags.insert(NeedsUpdate::ALL);
 			}
-			Action::AbortMerge => {
+			Action::AbortRevert | Action::AbortMerge => {
-				self.status_tab.abort_merge();
+				self.status_tab.revert_pending_state();
 				flags.insert(NeedsUpdate::ALL);
 			}
 			Action::AbortRebase => {

src/components/branchlist.rs

@@ -446,7 +446,7 @@ impl BranchListComponent {
 
 		if sync::repo_state(&self.repo.borrow())? != RepoState::Clean
 		{
-			self.queue.push(InternalEvent::TabSwitch);
+			self.queue.push(InternalEvent::TabSwitchStatus);
 		}
 
 		Ok(())

src/components/commit.rs

@@ -39,6 +39,7 @@ enum Mode {
 	Normal,
 	Amend(CommitId),
 	Merge(Vec<CommitId>),
+	Revert,
 }
 
 pub struct CommitComponent {
@@ -227,6 +228,9 @@ impl CommitComponent {
 			Mode::Merge(ids) => {
 				sync::merge_commit(&self.repo.borrow(), &msg, ids)?
 			}
+			Mode::Revert => {
+				sync::commit_revert(&self.repo.borrow(), &msg)?
+			}
 		};
 
 		if let HookResult::NotOk(e) =
@@ -380,31 +384,40 @@ impl Component for CommitComponent {
 
 		self.mode = Mode::Normal;
 
-		self.mode = if sync::repo_state(&self.repo.borrow())?
+		let repo_state = sync::repo_state(&self.repo.borrow())?;
-			== RepoState::Merge
-		{
-			let ids = sync::mergehead_ids(&self.repo.borrow())?;
-			self.input.set_title(strings::commit_title_merge());
-			self.input
-				.set_text(sync::merge_msg(&self.repo.borrow())?);
-			Mode::Merge(ids)
-		} else {
-			self.commit_template = get_config_string(
-				&self.repo.borrow(),
-				"commit.template",
-			)
-			.ok()
-			.flatten()
-			.and_then(|path| read_to_string(path).ok());
 
-			if self.is_empty() {
+		self.mode = match repo_state {
-				if let Some(s) = &self.commit_template {
+			RepoState::Merge => {
-					self.input.set_text(s.clone());
+				let ids = sync::mergehead_ids(&self.repo.borrow())?;
-				}
+				self.input.set_title(strings::commit_title_merge());
+				self.input
+					.set_text(sync::merge_msg(&self.repo.borrow())?);
+				Mode::Merge(ids)
 			}
+			RepoState::Revert => {
+				self.input.set_title(strings::commit_title_revert());
+				self.input
+					.set_text(sync::merge_msg(&self.repo.borrow())?);
+				Mode::Revert
+			}
+			_ => {
+				self.commit_template = get_config_string(
+					&self.repo.borrow(),
+					"commit.template",
+				)
+				.ok()
+				.flatten()
+				.and_then(|path| read_to_string(path).ok());
 
-			self.input.set_title(strings::commit_title());
+				if self.is_empty() {
-			Mode::Normal
+					if let Some(s) = &self.commit_template {
+						self.input.set_text(s.clone());
+					}
+				}
+
+				self.input.set_title(strings::commit_title());
+				Mode::Normal
+			}
 		};
 
 		self.input.show()?;

src/components/reset.rs

@@ -199,11 +199,15 @@ impl ConfirmComponent {
                 ),
                 Action::AbortMerge => (
                     strings::confirm_title_abortmerge(),
-                    strings::confirm_msg_abortmerge(),
+                    strings::confirm_msg_revertchanges(),
                 ),
 				Action::AbortRebase => (
                     strings::confirm_title_abortrebase(),
                     strings::confirm_msg_abortrebase(),
+                ),
+				Action::AbortRevert => (
+                    strings::confirm_title_abortrevert(),
+                    strings::confirm_msg_revertchanges(),
                 ),
             };
 		}

src/components/revision_files.rs

@@ -327,7 +327,7 @@ impl Component for RevisionFilesComponent {
 				if let Some(file) = self.selected_file() {
 					//Note: switch to status tab so its clear we are
 					// not altering a file inside a revision here
-					self.queue.push(InternalEvent::TabSwitch);
+					self.queue.push(InternalEvent::TabSwitchStatus);
 					self.queue.push(
 						InternalEvent::OpenExternalEditor(Some(file)),
 					);

src/queue.rs

@@ -43,6 +43,7 @@ pub enum Action {
 	PullMerge { incoming: usize, rebase: bool },
 	AbortMerge,
 	AbortRebase,
+	AbortRevert,
 }
 
 ///
@@ -62,7 +63,7 @@ pub enum InternalEvent {
 	///
 	PopupStashing(StashingOptions),
 	///
-	TabSwitch,
+	TabSwitchStatus,
 	///
 	InspectCommit(CommitId, Option<CommitTags>),
 	///

src/strings.rs

@@ -90,9 +90,13 @@ pub fn msg_title_error(_key_config: &SharedKeyConfig) -> String {
 pub fn commit_title() -> String {
 	"Commit".to_string()
 }
+
 pub fn commit_title_merge() -> String {
 	"Commit (Merge)".to_string()
 }
+pub fn commit_title_revert() -> String {
+	"Commit (Revert)".to_string()
+}
 pub fn commit_title_amend() -> String {
 	"Commit (Amend)".to_string()
 }
@@ -156,7 +160,10 @@ pub fn confirm_msg_merge(
 pub fn confirm_title_abortmerge() -> String {
 	"Abort merge?".to_string()
 }
-pub fn confirm_msg_abortmerge() -> String {
+pub fn confirm_title_abortrevert() -> String {
+	"Abort revert?".to_string()
+}
+pub fn confirm_msg_revertchanges() -> String {
 	"This will revert all uncommitted changes. Are you sure?"
 		.to_string()
 }
@@ -647,6 +654,17 @@ pub mod commands {
 		)
 	}
 
+	pub fn abort_revert(key_config: &SharedKeyConfig) -> CommandText {
+		CommandText::new(
+			format!(
+				"Abort revert [{}]",
+				key_config.get_hint(key_config.keys.abort_merge),
+			),
+			"abort ongoing revert",
+			CMD_GROUP_GENERAL,
+		)
+	}
+
 	pub fn continue_rebase(
 		key_config: &SharedKeyConfig,
 	) -> CommandText {
@@ -1035,6 +1053,19 @@ pub mod commands {
 			CMD_GROUP_LOG,
 		)
 	}
+	pub fn revert_commit(
+		key_config: &SharedKeyConfig,
+	) -> CommandText {
+		CommandText::new(
+			format!(
+				"Revert [{}]",
+				key_config
+					.get_hint(key_config.keys.status_reset_item),
+			),
+			"revert commit",
+			CMD_GROUP_LOG,
+		)
+	}
 	pub fn tag_commit_confirm_msg(
 		key_config: &SharedKeyConfig,
 	) -> CommandText {

src/tabs/revlog.rs

@@ -6,7 +6,7 @@ use crate::{
 	},
 	keys::SharedKeyConfig,
 	queue::{InternalEvent, Queue},
-	strings,
+	strings, try_or_popup,
 	ui::style::SharedTheme,
 };
 use anyhow::Result;
@@ -190,6 +190,15 @@ impl Revlog {
 			anyhow::bail!("Could not select commit in revlog. It might not be loaded yet or it might be on a different branch.");
 		}
 	}
+
+	fn revert_commit(&self) -> Result<()> {
+		if let Some(c) = self.selected_commit() {
+			sync::revert_commit(&self.repo.borrow(), c)?;
+			self.queue.push(InternalEvent::TabSwitchStatus);
+		}
+
+		Ok(())
+	}
 }
 
 impl DrawableComponent for Revlog {
@@ -267,6 +276,15 @@ impl Component for Revlog {
 					);
 				} else if k == self.key_config.keys.select_branch {
 					self.queue.push(InternalEvent::SelectBranch);
+					return Ok(EventState::Consumed);
+				} else if k == self.key_config.keys.status_reset_item
+				{
+					try_or_popup!(
+						self,
+						"revert error:",
+						self.revert_commit()
+					);
+
 					return Ok(EventState::Consumed);
 				} else if k == self.key_config.keys.open_file_tree {
 					return self.selected_commit().map_or(
@@ -385,6 +403,12 @@ impl Component for Revlog {
 			self.visible || force_all,
 		));
 
+		out.push(CommandInfo::new(
+			strings::commands::revert_commit(&self.key_config),
+			self.selected_commit().is_some(),
+			self.visible || force_all,
+		));
+
 		visibility_blocking(self)
 	}
 

src/tabs/stashlist.rs

@@ -63,7 +63,7 @@ impl StashList {
 			match sync::stash_apply(&self.repo.borrow(), e.id, false)
 			{
 				Ok(_) => {
-					self.queue.push(InternalEvent::TabSwitch);
+					self.queue.push(InternalEvent::TabSwitchStatus);
 				}
 				Err(e) => {
 					self.queue.push(InternalEvent::ShowErrorMsg(

src/tabs/status.rs

@@ -276,6 +276,16 @@ impl Status {
 					String::new()
 				}
 			}
+			RepoState::Revert => {
+				format!(
+					"Revert {}",
+					sync::revert_head(repo)
+						.ok()
+						.as_ref()
+						.map(CommitId::get_short_string)
+						.unwrap_or_default(),
+				)
+			}
 			_ => format!("{:?}", state),
 		}
 	}
@@ -611,11 +621,17 @@ impl Status {
 			== RepoState::Rebase
 	}
 
-	pub fn abort_merge(&self) {
+	fn pending_revert(&self) -> bool {
+		sync::repo_state(&self.repo.borrow())
+			.unwrap_or(RepoState::Clean)
+			== RepoState::Revert
+	}
+
+	pub fn revert_pending_state(&self) {
 		try_or_popup!(
 			self,
-			"abort merge",
+			"revert pending state",
-			sync::abort_merge(&self.repo.borrow())
+			sync::abort_pending_state(&self.repo.borrow())
 		);
 	}
 
@@ -754,11 +770,18 @@ impl Component for Status {
 				true,
 				self.pending_rebase() || force_all,
 			));
+
 			out.push(CommandInfo::new(
 				strings::commands::abort_rebase(&self.key_config),
 				true,
 				self.pending_rebase() || force_all,
 			));
+
+			out.push(CommandInfo::new(
+				strings::commands::abort_revert(&self.key_config),
+				true,
+				self.pending_revert() || force_all,
+			));
 		}
 
 		{
@@ -863,20 +886,26 @@ impl Component for Status {
 						NeedsUpdate::ALL,
 					));
 					Ok(EventState::Consumed)
-				} else if k == self.key_config.keys.abort_merge
+				} else if k == self.key_config.keys.abort_merge {
-					&& self.can_abort_merge()
+					if self.can_abort_merge() {
-				{
+						self.queue.push(
-					self.queue.push(InternalEvent::ConfirmAction(
+							InternalEvent::ConfirmAction(
-						Action::AbortMerge,
+								Action::AbortMerge,
-					));
+							),
-
+						);
-					Ok(EventState::Consumed)
+					} else if self.pending_rebase() {
-				} else if k == self.key_config.keys.abort_merge
+						self.queue.push(
-					&& self.pending_rebase()
+							InternalEvent::ConfirmAction(
-				{
+								Action::AbortRebase,
-					self.queue.push(InternalEvent::ConfirmAction(
+							),
-						Action::AbortRebase,
+						);
-					));
+					} else if self.pending_revert() {
+						self.queue.push(
+							InternalEvent::ConfirmAction(
+								Action::AbortRevert,
+							),
+						);
+					}
 
 					Ok(EventState::Consumed)
 				} else if k == self.key_config.keys.rebase_branch