cli: basic jj gerrit mail implementation

This implements the most basic workflow for submitting changes to Gerrit,
through a verb called 'mail'. The idea is to 'mail in' changes to be read by
another human. This name is taken from the dictionary used by Piper/CitC at
Google, and intended to be distinct from the word 'submit', which for Gerrit
means 'merge a change into the repository.'

Given a list of revsets (specified by multiple `-r` options), this will parse
the footers of every commit, collect them, insert a `Change-Id` (if one doesn't
already exist), and then push them into the given remote.

Because the argument is a revset, you may submit entire trees of changes at
once, including multiple trees of independent changes, e.g.

    jj gerrit mail -r foo:: -r baz::

There are many other improvements that can be applied on top of this, including
a ton of consistency and "does this make sense?" checks. However, it is flexible
and a good starting point, and you can in fact both submit and cycle reviews
with this interface.

Signed-off-by: Austin Seipp <[email protected]>
Change-Id: I12f08dfb0fd3c57f3da34ed44ca1b1000e6ffe74

cli/src/commands/gerrit.rs

@@ -13,15 +13,22 @@
 // limitations under the License.
 
 use std::fmt::Debug;
+use std::io::Write;
 
 use clap::Subcommand;
-use indexmap::IndexSet;
+use hex::ToHex;
+use indexmap::{IndexMap, IndexSet};
 use jj_lib::commit::Commit;
+use jj_lib::content_hash::blake2b_hash;
+use jj_lib::footer::get_footer_lines;
+use jj_lib::git::{self, GitRefUpdate};
+use jj_lib::hex_util::to_reverse_hex;
+use jj_lib::object_id::ObjectId;
 use jj_lib::repo::Repo;
 
 use crate::cli_util::{
- get_git_repo, resolve_multiple_nonempty_revsets, user_error, CommandError, CommandHelper,
- RevisionArg,
+ get_git_repo, resolve_multiple_nonempty_revsets, short_commit_hash, user_error,
+ with_remote_git_callbacks, CommandError, CommandHelper, RevisionArg,
 };
 use crate::ui::Ui;
 
@@ -76,6 +83,11 @@ pub struct MailCommand {
  /// This remote MUST be a Git push URL for your Gerrit instance.
  #[arg(long = "for", short = 'f', default_value = "main@origin")]
  for_: String,
+
+ /// If true, do not actually add `Change-Id`s to commits, and do not push
+ /// the changes to Gerrit.
+ #[arg(long = "dry-run", short = 'n')]
+ dry_run: bool,
 }
 
 pub fn cmd_mail(
@@ -96,8 +108,18 @@ pub fn cmd_mail(
 
  workspace_command.check_rewritable(to_mail.iter())?;
 
- // make sure that the --for parameter is a valid refspec of the
- // form 'branch@remote'
+ let mut tx = workspace_command.start_transaction();
+ let base_repo = tx.base_repo().clone();
+ let mut_repo = tx.mut_repo();
+ let store = base_repo.store();
+ let git_repo = get_git_repo(store)?; // will fail if not a git repo
+
+ // make sure that the --for parameter is a valid refspec of the form
+ // 'branch@remote', then make sure the remote exists
+ //
+ // XXX (aseipp): for yuya -- surely there's a better way to do this whole
+ // thing? maybe we should just let the push fail later after rewriting the
+ // messages?
  let for_parts: Vec<&str> = mail.for_.split('@').collect();
  if for_parts.len() != 2 {
  return Err(user_error(
@@ -108,12 +130,6 @@ pub fn cmd_mail(
  let for_branch = for_parts[0];
  let for_remote = for_parts[1];
 
- let tx = workspace_command.start_transaction();
- let base_repo = tx.base_repo().clone();
- let store = base_repo.store();
- let git_repo = get_git_repo(&store)?; // will fail if not a git repo
-
- // make sure the remote exists
  let mut remote_exists = false;
  for remote in git_repo.remotes()?.iter() {
  if remote == Some(for_remote) {
@@ -123,14 +139,261 @@ pub fn cmd_mail(
  }
 
  if !remote_exists {
- return Err(user_error(&format!(
+ return Err(user_error(format!(
  "The (Gerrit) remote '{}' does not exist",
  for_remote
  )));
  }
 
- for head in base_repo.index().heads(&mut to_mail.iter().map(|c| c.id())) {
- println!("Pushing to {}@{}: {:?}", for_branch, for_remote, head);
+ // immediately error and reject any discardable commits, i.e. the
+ // the empty wcc
+ for commit in to_mail.iter() {
+ if commit.is_discardable() {
+ return Err(user_error(format!(
+ "Refusing to mail in commit {} because it is an empty commit with no \
+ description\n(use 'jj amend' to add a description, or 'jj abandon' to discard it)",
+ short_commit_hash(commit.id())
+ )));
+ }
+ }
+
+ // the mapping is from old -> [new, is_dry_run]; the dry_run flag is used to
+ // disambiguate a later case when printing errors, so we know that if a
+ // commit was mapped to itself, it was because --dry-run was set, and not
+ // because e.g. it had an existing change id already
+ let mut old_to_new: IndexMap<Commit, (Commit, bool)> = IndexMap::new();
+ for commit_id in base_repo
+ .index()
+ .topo_order(&mut to_mail.iter().map(|c| c.id()))
+ .into_iter()
+ {
+ let original_commit = store.get_commit(&commit_id).unwrap();
+ let description = original_commit.description().to_owned();
+ let footer = get_footer_lines(&description);
+
+ if let Some(footer) = footer.clone() {
+ // look up the existing change id footer
+ let change_id = footer.iter().find(|(key, _)| key == &"Change-Id");
+ if let Some((_, values)) = change_id {
+ // map the old commit to itself
+ old_to_new.insert(original_commit.clone(), (original_commit.clone(), false));
+
+ // multiple change-ids are not allowed
+ if values.len() > 1 {
+ writeln!(
+ ui.warning(),
+ "warning: multiple Change-Id footers in commit {}",
+ short_commit_hash(original_commit.clone().id()),
+ )?;
+ continue;
+ }
+
+ // check the change-id format is correct in any case
+ if values[0].len() != 41 || !values[0].starts_with('I') {
+ writeln!(
+ ui.warning(),
+ "warning: invalid Change-Id footer in commit {}",
+ short_commit_hash(original_commit.id()),
+ )?;
+ continue;
+ }
+
+ // XXX (aseipp): should we rewrite these invalid Change-Ids? i
+ // don't think so, but I don't know what gerrit will do with
+ // them, and I realized my old signoff.sh script created invalid
+ // ones, so this is a helpful barrier.
+
+ continue; // fallthrough
+ }
+ }
+
+ if mail.dry_run {
+ // mark the old commit as rewritten to itself, but only because it
+ // was a --dry-run, so we can give better error messages later
+ old_to_new.insert(original_commit.clone(), (original_commit.clone(), true));
+ continue;
+ }
+
+ // NOTE: Gerrit's change ID is not compatible with the alphabet used by
+ // jj, and the needed length of the change-id is different as well.
+ //
+ // for us, we convert to gerrit's format is the character 'I', followed
+ // by 40 characters of the truncated blake2 hash of the change id
+ let change_id = to_reverse_hex(&original_commit.change_id().hex()).unwrap();
+ let hashed_id: String = blake2b_hash(&change_id).encode_hex();
+ let gerrit_change_id = format!("I{}", hashed_id.chars().take(40).collect::<String>());
+
+ // XXX (aseipp): move this description junk for rewriting the description to
+ // footer.rs; improves reusability and makes things a little cleaner
+ let spacing = if let Some(footer) = footer {
+ if footer.is_empty() {
+ "\n\n"
+ } else {
+ "\n"
+ }
+ } else {
+ "\n\n"
+ };
+
+ let new_description = format!(
+ "{}{}Change-Id: {}\n",
+ description.trim(),
+ spacing,
+ gerrit_change_id
+ );
+
+ // rewrite the set of parents to point to the commits that were
+ // previously rewritten in toposort order
+ //
+ // TODO FIXME (aseipp): this whole dance with toposorting, calculating
+ // new_parents, and then doing rewrite_commit is roughly equivalent to
+ // what we do in duplicate.rs as well. we should probably refactor this?
+ let new_parents = original_commit
+ .parents()
+ .iter()
+ .map(|parent| {
+ if let Some((rewritten_parent, _)) = old_to_new.get(parent) {
+ rewritten_parent
+ } else {
+ parent
+ }
+ .id()
+ .clone()
+ })
+ .collect();
+
+ let new_commit = mut_repo
+ .rewrite_commit(command.settings(), &original_commit)
+ .set_description(new_description)
+ .set_parents(new_parents)
+ .write()?;
+ old_to_new.insert(original_commit.clone(), (new_commit.clone(), false));
+ }
+
+ tx.finish(
+ ui,
+ format!(
+ "describing {} commit(s) for mail-in to gerrit",
+ old_to_new.len()
+ ),
+ )?;
+
+ // XXX (aseipp): is this transaction safe to leave open? should it record a
+ // push instead in the op log, even if it can't be meaningfully undone?
+ let tx = workspace_command.start_transaction();
+ let base_repo = tx.base_repo().clone();
+
+ // NOTE(aseipp): write the status report *after* finishing the first
+ // transaction. until we call 'tx.finish', the outstanding tx write set
+ // contains a commit with a duplicated jj change-id, i.e. while the
+ // transaction is open, it is ambiguous whether the change-id refers to the
+ // newly written commit or the old one that already existed.
+ //
+ // this causes an awkward UX interaction, where write_commit_summary will
+ // output a line with a red change-id indicating it's duplicated/conflicted,
+ // AKA "??" status. but then the user will immediately run 'jj log' and not
+ // see any conflicting change-ids, because the transaction was committed by
+ // then and the new commits replaced the old ones! just printing this after
+ // the transaction finishes avoids this weird case.
+ //
+ // XXX (aseipp): ask martin for feedback
+ for (old, (new, is_dry)) in old_to_new.iter() {
+ if old != new {
+ write!(ui.stderr(), "Added Change-Id footer to ")?;
+ } else if *is_dry {
+ write!(ui.stderr(), "Dry-run: would have added Change-Id to ")?;
+ } else {
+ write!(ui.stderr(), "Skipped Change-Id (it already exists) for ")?;
+ }
+ tx.write_commit_summary(ui.stderr_formatter().as_mut(), new)?;
+ writeln!(ui.stderr())?;
+ }
+ writeln!(ui.stderr())?;
+
+ let new_commits = old_to_new.values().map(|x| &x.0).collect::<Vec<&Commit>>();
+ let new_heads = base_repo
+ .index()
+ .heads(&mut new_commits.iter().map(|c| c.id()));
+ let remote_ref = format!("refs/for/{}", for_branch);
+
+ writeln!(
+ ui.stderr(),
+ "Found {} heads to push to Gerrit (remote '{}'), target branch '{}'",
+ new_heads.len(),
+ for_remote,
+ for_branch,
+ )?;
+
+ // split these two loops to keep the output a little nicer; display first,
+ // then push
+ for head in &new_heads {
+ let head_commit = store.get_commit(head).unwrap();
+
+ write!(ui.stderr(), " ")?;
+ tx.write_commit_summary(ui.stderr_formatter().as_mut(), &head_commit)?;
+ writeln!(ui.stderr())?;
+ }
+ writeln!(ui.stderr())?;
+
+ if mail.dry_run {
+ writeln!(
+ ui.stderr(),
+ "Dry-run: Not performing push, as `--dry-run` was requested"
+ )?;
+ return Ok(());
+ }
+
+ // NOTE (aseipp): because we are pushing everything to the same remote ref,
+ // we have to loop and push each commit one at a time, even though
+ // push_updates in theory supports multiple GitRefUpdates at once, because
+ // we obviously can't push multiple heads to the same ref.
+ for head in &new_heads {
+ let head_commit = store.get_commit(head).unwrap();
+ let head_id = head_commit.id().clone();
+
+ write!(ui.stderr(), "Pushing ")?;
+ tx.write_commit_summary(ui.stderr_formatter().as_mut(), &head_commit)?;
+ writeln!(ui.stderr())?;
+
+ // XXX (aseipp): for yuya -- is this roughly correct? i don't think we
+ // want to record or update jj's state on the remote refs when we push,
+ // since it's special and synthetic ref in the first place.
+ //
+ // how do we get better errors from the remote? 'git push' tells us
+ // about rejected refs AND ALSO '(nothing changed)' when there are no
+ // changes to push, but we don't get that here. RefUpdateRejected might
+ // need more context, idk. is this a libgit2 problem?
+ with_remote_git_callbacks(ui, |cb| {
+ git::push_updates(
+ &git_repo,
+ for_remote,
+ &[GitRefUpdate {
+ qualified_name: remote_ref.clone(),
+ force: false,
+ new_target: Some(head_id),
+ }],
+ cb,
+ )
+ })
+ .map_or_else(
+ |err| match err {
+ git::GitPushError::RefUpdateRejected(_) => {
+ // gerrit rejects ref updates when there are no changes, i.e.
+ // you submit a change that is already up to date. just give
+ // the user a light warning and carry on
+ writeln!(
+ ui.warning(),
+ "warning: ref update rejected by gerrit; no changes to push (did you \
+ forget to update, amend, or add new changes?)"
+ )?;
+
+ Ok(())
+ }
+ // XXX (aseipp): more cases to handle here?
+ _ => Err(user_error(err.to_string())),
+ },
+ Ok,
+ )?;
  }
 
  Ok(())