From 9d83ce358eb85e26d69f47f8555a986a914ee7c0 Mon Sep 17 00:00:00 2001 From: extrawurst <776816+extrawurst@users.noreply.github.com> Date: Sat, 18 Feb 2023 20:47:24 +0000 Subject: [PATCH] Reword commit (#1553) * reuse commit popup for reword * switch to status after reword * show command * prepopulate with old msg * changelog Closes #829 --- CHANGELOG.md | 1 + asyncgit/src/error.rs | 8 ++ asyncgit/src/sync/mod.rs | 2 + asyncgit/src/sync/reword.rs | 170 ++++++++++++++++++++++++++++++++++++ src/app.rs | 3 + src/components/commit.rs | 130 +++++++++++++++++---------- src/keys/key_list.rs | 2 + src/queue.rs | 2 + src/strings.rs | 15 ++++ src/tabs/revlog.rs | 18 ++++ 10 files changed, 303 insertions(+), 48 deletions(-) create mode 100644 asyncgit/src/sync/reword.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc296d4..05bf7ddc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * allow reset (soft,mixed,hard) from commit log ([#1500](https://github.com/extrawurst/gitui/issues/1500)) * allow `copy` file path on revision files and status tree [[@yanganto]](https://github.com/yanganto) ([#1516](https://github.com/extrawurst/gitui/pull/1516)) * print message of where log will be written if `-l` is set ([#1472](https://github.com/extrawurst/gitui/pull/1472)) +* support **reword** of commit from log ([#829](https://github.com/extrawurst/gitui/pull/829)) ### Fixes * commit msg history ordered the wrong way ([#1445](https://github.com/extrawurst/gitui/issues/1445)) diff --git a/asyncgit/src/error.rs b/asyncgit/src/error.rs index 9078e314..e171d04a 100644 --- a/asyncgit/src/error.rs +++ b/asyncgit/src/error.rs @@ -76,6 +76,14 @@ pub enum Error { /// #[error("path string error")] PathString, + + /// + #[error("no parent of commit found")] + NoParent, + + /// + #[error("not on a branch")] + NoBranch, } /// diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 07a955a8..5bd4b0b7 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -23,6 +23,7 @@ mod rebase; pub mod remotes; mod repository; mod reset; +mod reword; mod staging; mod stash; mod state; @@ -76,6 +77,7 @@ pub use remotes::{ pub(crate) use repository::repo; pub use repository::{RepoPath, RepoPathRef}; pub use reset::{reset_repo, reset_stage, reset_workdir}; +pub use reword::reword; pub use staging::{discard_lines, stage_lines}; pub use stash::{ get_stashes, stash_apply, stash_drop, stash_pop, stash_save, diff --git a/asyncgit/src/sync/reword.rs b/asyncgit/src/sync/reword.rs new file mode 100644 index 00000000..4259498d --- /dev/null +++ b/asyncgit/src/sync/reword.rs @@ -0,0 +1,170 @@ +use git2::{Oid, RebaseOptions, Repository}; + +use super::{ + commit::signature_allow_undefined_name, + repo, + utils::{bytes2string, get_head_refname}, + CommitId, RepoPath, +}; +use crate::error::{Error, Result}; + +/// This is the same as reword, but will abort and fix the repo if something goes wrong +pub fn reword( + repo_path: &RepoPath, + commit: CommitId, + message: &str, +) -> Result { + let repo = repo(repo_path)?; + let cur_branch_ref = get_head_refname(&repo)?; + + match reword_internal(&repo, commit.get_oid(), message) { + Ok(id) => Ok(id.into()), + // Something went wrong, checkout the previous branch then error + Err(e) => { + if let Ok(mut rebase) = repo.open_rebase(None) { + rebase.abort()?; + repo.set_head(&cur_branch_ref)?; + repo.checkout_head(None)?; + } + Err(e) + } + } +} + +/// Gets the current branch the user is on. +/// Returns none if they are not on a branch +/// and Err if there was a problem finding the branch +fn get_current_branch( + repo: &Repository, +) -> Result> { + for b in repo.branches(None)? { + let branch = b?.0; + if branch.is_head() { + return Ok(Some(branch)); + } + } + Ok(None) +} + +/// Changes the commit message of a commit with a specified oid +/// +/// While this function is most commonly associated with doing a +/// reword opperation in an interactive rebase, that is not how it +/// is implemented in git2rs +/// +/// This is dangerous if it errors, as the head will be detached so this should +/// always be wrapped by another function which aborts the rebase if something goes wrong +fn reword_internal( + repo: &Repository, + commit: Oid, + message: &str, +) -> Result { + let sig = signature_allow_undefined_name(repo)?; + + let parent_commit_oid = repo + .find_commit(commit)? + .parent(0) + .map_or(None, |parent_commit| Some(parent_commit.id())); + + let commit_to_change = if let Some(pc_oid) = parent_commit_oid { + // Need to start at one previous to the commit, so + // first rebase.next() points to the actual commit we want to change + repo.find_annotated_commit(pc_oid)? + } else { + return Err(Error::NoParent); + }; + + // If we are on a branch + if let Ok(Some(branch)) = get_current_branch(repo) { + let cur_branch_ref = bytes2string(branch.get().name_bytes())?; + let cur_branch_name = bytes2string(branch.name_bytes()?)?; + let top_branch_commit = repo.find_annotated_commit( + branch.get().peel_to_commit()?.id(), + )?; + + let mut rebase = repo.rebase( + Some(&top_branch_commit), + Some(&commit_to_change), + None, + Some(&mut RebaseOptions::default()), + )?; + + let mut target; + + rebase.next(); + if parent_commit_oid.is_none() { + return Err(Error::NoParent); + } + target = rebase.commit(None, &sig, Some(message))?; + let reworded_commit = target; + + // Set target to top commit, don't know when the rebase will end + // so have to loop till end + while rebase.next().is_some() { + target = rebase.commit(None, &sig, None)?; + } + rebase.finish(None)?; + + // Now override the previous branch + repo.branch( + &cur_branch_name, + &repo.find_commit(target)?, + true, + )?; + + // Reset the head back to the branch then checkout head + repo.set_head(&cur_branch_ref)?; + repo.checkout_head(None)?; + return Ok(reworded_commit); + } + // Repo is not on a branch, possibly detached head + Err(Error::NoBranch) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::sync::{ + get_commit_info, + tests::{repo_init_empty, write_commit_file}, + }; + use pretty_assertions::assert_eq; + + #[test] + fn test_reword() { + let (_td, repo) = repo_init_empty().unwrap(); + let root = repo.path().parent().unwrap(); + + let repo_path: &RepoPath = + &root.as_os_str().to_str().unwrap().into(); + + write_commit_file(&repo, "foo", "a", "commit1"); + + let oid2 = write_commit_file(&repo, "foo", "ab", "commit2"); + + let branch = + repo.branches(None).unwrap().next().unwrap().unwrap().0; + let branch_ref = branch.get(); + let commit_ref = branch_ref.peel_to_commit().unwrap(); + let message = commit_ref.message().unwrap(); + + assert_eq!(message, "commit2"); + + let reworded = + reword(repo_path, oid2.into(), "NewCommitMessage") + .unwrap(); + + // Need to get the branch again as top oid has changed + let branch = + repo.branches(None).unwrap().next().unwrap().unwrap().0; + let branch_ref = branch.get(); + let commit_ref_new = branch_ref.peel_to_commit().unwrap(); + let message_new = commit_ref_new.message().unwrap(); + assert_eq!(message_new, "NewCommitMessage"); + + assert_eq!( + message_new, + get_commit_info(repo_path, &reworded).unwrap().message + ); + } +} diff --git a/src/app.rs b/src/app.rs index 98ebd348..cb90c515 100644 --- a/src/app.rs +++ b/src/app.rs @@ -815,6 +815,9 @@ impl App { } InternalEvent::Update(u) => flags.insert(u), InternalEvent::OpenCommit => self.commit.show()?, + InternalEvent::RewordCommit(id) => { + self.commit.open(Some(id))?; + } InternalEvent::PopupStashing(opts) => { self.stashmsg_popup.options(opts); self.stashmsg_popup.show()?; diff --git a/src/components/commit.rs b/src/components/commit.rs index 820d3e7d..605c70c9 100644 --- a/src/components/commit.rs +++ b/src/components/commit.rs @@ -10,7 +10,7 @@ use crate::{ strings, try_or_popup, ui::style::SharedTheme, }; -use anyhow::Result; +use anyhow::{bail, Ok, Result}; use asyncgit::{ cached, message_prettify, sync::{ @@ -42,6 +42,7 @@ enum Mode { Amend(CommitId), Merge(Vec), Revert, + Reword(CommitId), } pub struct CommitComponent { @@ -289,6 +290,13 @@ impl CommitComponent { Mode::Revert => { sync::commit_revert(&self.repo.borrow(), msg)? } + Mode::Reword(id) => { + let commit = + sync::reword(&self.repo.borrow(), *id, msg)?; + self.queue.push(InternalEvent::TabSwitchStatus); + + commit + } }; Ok(()) } @@ -332,6 +340,78 @@ impl CommitComponent { fn toggle_verify(&mut self) { self.verify = !self.verify; } + + pub fn open(&mut self, reword: Option) -> Result<()> { + //only clear text if it was not a normal commit dlg before, so to preserve old commit msg that was edited + if !matches!(self.mode, Mode::Normal) { + self.input.clear(); + } + + self.mode = Mode::Normal; + + let repo_state = sync::repo_state(&self.repo.borrow())?; + + self.mode = + if repo_state != RepoState::Clean && reword.is_some() { + bail!("cannot reword while repo is not in a clean state"); + } else if let Some(reword_id) = reword { + self.input.set_text( + sync::get_commit_details( + &self.repo.borrow(), + reword_id, + )? + .message + .unwrap_or_default() + .combine(), + ); + self.input.set_title(strings::commit_reword_title()); + Mode::Reword(reword_id) + } else { + match repo_state { + 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) + } + 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()); + + if self.is_empty() { + if let Some(s) = &self.commit_template { + self.input.set_text(s.clone()); + } + } + self.input.set_title(strings::commit_title()); + Mode::Normal + } + } + }; + + self.commit_msg_history_idx = 0; + self.input.show()?; + + Ok(()) + } } impl DrawableComponent for CommitComponent { @@ -466,53 +546,7 @@ impl Component for CommitComponent { } fn show(&mut self) -> Result<()> { - //only clear text if it was not a normal commit dlg before, so to preserve old commit msg that was edited - if !matches!(self.mode, Mode::Normal) { - self.input.clear(); - } - - self.mode = Mode::Normal; - - let repo_state = sync::repo_state(&self.repo.borrow())?; - - self.mode = match repo_state { - 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) - } - 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()); - - if self.is_empty() { - if let Some(s) = &self.commit_template { - self.input.set_text(s.clone()); - } - } - - self.input.set_title(strings::commit_title()); - Mode::Normal - } - }; - - self.commit_msg_history_idx = 0; - self.input.show()?; - + self.open(None)?; Ok(()) } } diff --git a/src/keys/key_list.rs b/src/keys/key_list.rs index 98f93ecf..7e1406e5 100644 --- a/src/keys/key_list.rs +++ b/src/keys/key_list.rs @@ -86,6 +86,7 @@ pub struct KeysList { pub log_mark_commit: GituiKeyEvent, pub log_checkout_commit: GituiKeyEvent, pub log_reset_comit: GituiKeyEvent, + pub log_reword_comit: GituiKeyEvent, pub commit_amend: GituiKeyEvent, pub toggle_verify: GituiKeyEvent, pub copy: GituiKeyEvent, @@ -168,6 +169,7 @@ impl Default for KeysList { log_mark_commit: GituiKeyEvent::new(KeyCode::Char(' '), KeyModifiers::empty()), log_checkout_commit: GituiKeyEvent { code: KeyCode::Char('S'), modifiers: KeyModifiers::SHIFT }, log_reset_comit: GituiKeyEvent { code: KeyCode::Char('R'), modifiers: KeyModifiers::SHIFT }, + log_reword_comit: GituiKeyEvent { code: KeyCode::Char('r'), modifiers: KeyModifiers::empty() }, commit_amend: GituiKeyEvent::new(KeyCode::Char('a'), KeyModifiers::CONTROL), toggle_verify: GituiKeyEvent::new(KeyCode::Char('f'), KeyModifiers::CONTROL), copy: GituiKeyEvent::new(KeyCode::Char('y'), KeyModifiers::empty()), diff --git a/src/queue.rs b/src/queue.rs index 6f958410..81282b29 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -128,6 +128,8 @@ pub enum InternalEvent { OpenRepo { path: PathBuf }, /// OpenResetPopup(CommitId), + /// + RewordCommit(CommitId), } /// single threaded simple queue for components to communicate with each other diff --git a/src/strings.rs b/src/strings.rs index f02cc883..07d33e89 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -104,6 +104,9 @@ pub fn msg_title_info(_key_config: &SharedKeyConfig) -> String { pub fn commit_title() -> String { "Commit".to_string() } +pub fn commit_reword_title() -> String { + "Reword Commit".to_string() +} pub fn commit_title_merge() -> String { "Commit (Merge)".to_string() @@ -1255,6 +1258,18 @@ pub mod commands { CMD_GROUP_LOG, ) } + pub fn log_reword_commit( + key_config: &SharedKeyConfig, + ) -> CommandText { + CommandText::new( + format!( + "Reword [{}]", + key_config.get_hint(key_config.keys.log_reword_comit), + ), + "reword commit message", + CMD_GROUP_LOG, + ) + } pub fn reset_commit(key_config: &SharedKeyConfig) -> CommandText { CommandText::new( format!( diff --git a/src/tabs/revlog.rs b/src/tabs/revlog.rs index 45be59b9..b1faf69c 100644 --- a/src/tabs/revlog.rs +++ b/src/tabs/revlog.rs @@ -340,6 +340,19 @@ impl Component for Revlog { Ok(EventState::Consumed) }, ); + } else if key_match( + k, + self.key_config.keys.log_reword_comit, + ) { + return self.selected_commit().map_or( + Ok(EventState::NotConsumed), + |id| { + self.queue.push( + InternalEvent::RewordCommit(id), + ); + Ok(EventState::Consumed) + }, + ); } else if key_match( k, self.key_config.keys.compare_commits, @@ -467,6 +480,11 @@ impl Component for Revlog { self.selected_commit().is_some(), self.visible || force_all, )); + out.push(CommandInfo::new( + strings::commands::log_reword_commit(&self.key_config), + self.selected_commit().is_some(), + self.visible || force_all, + )); visibility_blocking(self) }