cleaning up and adding error messages

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<String>,
 ) -> eyre::Result<Option<Vec<Commit<'repo>>>> {
  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<String>,
- },
 }
 
 /// 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<BuildRewordMessageResult> {
  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<NonZeroOid, String> = commits
  .iter()
  .map(|commit| (commit.get_oid(), message.clone()))

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<NonZeroOid, MaybeZeroOid>,
  },
 
@@ -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) => {

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(())
  }