diff --git a/src/commands/reword.rs b/src/commands/reword.rs index b5e5c7142..be7dd2266 100644 --- a/src/commands/reword.rs +++ b/src/commands/reword.rs @@ -71,16 +71,6 @@ pub fn reword( )?; return Ok(1); } - BuildRewordMessageResult::Failed { message } => { - if message != None { - writeln!(effects.get_error_stream(), "{}", message.unwrap())?; - } - writeln!( - effects.get_error_stream(), - "Error: problem processing commit message. Nothing reworded." - )?; - return Ok(1); - } }; let subtree_roots = find_subtree_roots(&repo, &dag, &commits)?; @@ -93,8 +83,15 @@ pub fn reword( for root_commit in subtree_roots { let only_parent_id = root_commit.get_only_parent().map(|parent| parent.get_oid()); if only_parent_id == None { - // TODO multiple parents or no parents! - // eyre::bail! ?? + eyre::bail!( + "### + Refusing to reword commit {}, which has {} parents. + Rewording only supported for commits with 1 parent. + Stopping; nothing reworded. + ###", + root_commit.get_oid(), + root_commit.get_parents().len(), + ); } builder.move_subtree(root_commit.get_oid(), only_parent_id.unwrap())?; } @@ -121,10 +118,12 @@ pub fn reword( Ok(None) => { writeln!( effects.get_output_stream(), - // FIXME - "No abandoned commits to restack." + "### + BUG: rebase plan indicates nothing to do, but rewording should always so something. + Stopping; nothing reworded. + ###" )?; - return Ok(0); + return Ok(1); } Err(err) => { err.describe(effects, &repo)?; @@ -198,7 +197,12 @@ pub fn reword( 0 } ExecuteRebasePlanResult::DeclinedToMerge { merge_conflict: _ } => { - 0 + writeln!( + effects.get_error_stream(), + "BUG: Merge conflict detected, but rewording shouldn't cause any conflicts." + )?; + + 1 } ExecuteRebasePlanResult::Failed { exit_code } => exit_code, }; @@ -214,8 +218,6 @@ fn resolve_commits_from_hashes<'repo>( hashes: Vec, ) -> eyre::Result>>> { let hashes = if hashes.is_empty() { - // TODO confirm that this rewords the "current" commit even if it's in the middle of a - // stack or branch vec!["HEAD".to_string()] } else { hashes @@ -246,12 +248,6 @@ pub enum BuildRewordMessageResult { /// The reworded message was empty. EmptyMessage, - - /// Misc failure. - Failed { - /// The failue message, if any. - message: Option, - }, } /// Builds the message(s) that will be used for rewording. These are mapped from each commit's @@ -263,27 +259,20 @@ fn build_messages( ) -> eyre::Result { let message = messages.join("\n\n").trim().to_string(); - // TODO(maybe?) stdin? - let (message, load_editor) = if message.is_empty() { let message_for_editor = match commits.to_vec().as_slice() { - [commit] => { - // FIXME lots of error checking to do - let msg = commit.get_message_raw()?; - let msg = msg.into_string(); - match msg { - Ok(msg) => msg, - _ => { - return Ok(BuildRewordMessageResult::Failed { - message: Some(String::from( - "Reword: Error decoding original commit message!", - )), - }) - } - } - } + [commit] => match commit.get_message_raw()?.into_string() { + Ok(msg) => msg, + _ => eyre::bail!( + "### + Error decoding original message for commit: {:?}. + Stopping; nothing reworded. + ###", + commit.get_oid() + ), + }, _ => { - // TODO build a bulk edit message for multiple commits + // TODO(bulk edit) build a bulk edit message for multiple commits format!( "Enter a commit message to apply to {} commits", commits.len() @@ -318,9 +307,15 @@ fn build_messages( return Ok(BuildRewordMessageResult::EmptyMessage); } - // TODO if the message appears to be a "bulk edit" message, break it apart - // TODO FIXME what if the bulk edit message doesn't include messages for != `commits.len` - // TODO FIXME what if the bulk edit message includes messages for commits not in `commits` + // TODO(bulk edit) process message if it looks like a bulk edit message + // - break it apart in separate messages + // - parse the marker lines into hashes + // - map hashes into NonZeroOid + // - create a map with OIDs as keys and relevant messages as values + // - confirm that we have edits for the right set of commits + // - just those spec'd on the CLI, no more no less + // - no duplicates + let messages: HashMap = commits .iter() .map(|commit| (commit.get_oid(), message.clone())) diff --git a/src/core/rewrite/execute.rs b/src/core/rewrite/execute.rs index 61b93bbc3..c7d1c0f85 100644 --- a/src/core/rewrite/execute.rs +++ b/src/core/rewrite/execute.rs @@ -725,6 +725,7 @@ mod on_disk { use tracing::instrument; use crate::core::effects::{Effects, OperationType}; + use crate::core::rewrite::plan::RebaseCommand; use crate::core::rewrite::plan::RebasePlan; use crate::core::rewrite::rewrite_hooks::save_original_head_info; use crate::git::{GitRunInfo, Repo}; @@ -839,6 +840,16 @@ mod on_disk { }, )?; + if rebase_plan.commands.iter().any(|command| match command { + RebaseCommand::Pick { + original_commit_oid, + commit_to_apply_oid, + } => original_commit_oid != commit_to_apply_oid, + _ => false, + }) { + eyre::bail!("Not implemented: replacing commits in an on disk rebase"); + } + let todo_file_path = rebase_state_dir.join("git-rebase-todo"); std::fs::write( &todo_file_path, @@ -964,6 +975,8 @@ pub struct ExecuteRebasePlanOptions<'a> { pub enum ExecuteRebasePlanResult { /// The rebase operation succeeded. Succeeded { + /// Mapping from old OID to new/rewritten OID. Will always be empty for on disk rebases. + // FIXME should this be an Option? rewritten_oids: HashMap, }, @@ -1090,7 +1103,7 @@ pub fn execute_rebase_plan( Ok(0) => { return Ok(ExecuteRebasePlanResult::Succeeded { rewritten_oids: Default::default(), - }) + }); } Ok(exit_code) => return Ok(ExecuteRebasePlanResult::Failed { exit_code }), Err(Error::ChangedFilesInRepository) => { diff --git a/src/core/rewrite/plan.rs b/src/core/rewrite/plan.rs index 8233d14bf..5cca711cb 100644 --- a/src/core/rewrite/plan.rs +++ b/src/core/rewrite/plan.rs @@ -459,19 +459,21 @@ impl<'repo> RebasePlanBuilder<'repo> { Ok(()) } - /// FIXME + /// Instruct the rebase planner to replace the commit at `original_oid` with the commit at + /// `replacement_oid`. pub fn replace_commit( &mut self, - original: NonZeroOid, - replacement: NonZeroOid, + original_oid: NonZeroOid, + replacement_oid: NonZeroOid, ) -> eyre::Result<()> { - if self.replacement_commits.contains_key(&original) { + if self.replacement_commits.contains_key(&original_oid) { eyre::bail!( "Attempting to rewrite commit {}. Refusing to replace a commit twice.", - original + original_oid ); } - self.replacement_commits.insert(original, replacement); + self.replacement_commits + .insert(original_oid, replacement_oid); Ok(()) }