From 83b9b3a8fa3d5b0b0b2f8ac284f9205b6c1c226b Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Thu, 18 Jan 2024 00:35:09 -0600 Subject: [PATCH] cli: basic `jj gerrit send` implementation This implements the most basic workflow for submitting changes to Gerrit, through a verb called 'send'. This verb is 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 send -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 --- Cargo.lock | 2 + cli/Cargo.toml | 4 +- cli/src/commands/gerrit/mod.rs | 57 ++++ cli/src/commands/gerrit/send.rs | 431 +++++++++++++++++++++++++++++++ cli/src/commands/mod.rs | 4 + cli/src/config-schema.json | 14 + cli/tests/cli-reference@.md.snap | 41 +++ 7 files changed, 552 insertions(+), 1 deletion(-) create mode 100644 cli/src/commands/gerrit/mod.rs create mode 100644 cli/src/commands/gerrit/send.rs diff --git a/Cargo.lock b/Cargo.lock index 65c17fc34a..b28c4cd927 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1842,6 +1842,7 @@ dependencies = [ "futures 0.3.31", "git2", "gix", + "hex", "indexmap", "indoc", "insta", @@ -1855,6 +1856,7 @@ dependencies = [ "pest", "pest_derive", "pollster", + "rand", "rayon", "regex", "rpassword", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index bf9b4de4c7..cc6b72d90e 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -22,7 +22,7 @@ include = [ "/tests/", "!*.pending-snap", "!*.snap*", - "/tests/cli-reference@.md.snap" + "/tests/cli-reference@.md.snap", ] [[bin]] @@ -64,6 +64,7 @@ esl01-renderdag = { workspace = true } futures = { workspace = true } git2 = { workspace = true } gix = { workspace = true } +hex = { workspace = true } indexmap = { workspace = true } indoc = { workspace = true } itertools = { workspace = true } @@ -74,6 +75,7 @@ once_cell = { workspace = true } pest = { workspace = true } pest_derive = { workspace = true } pollster = { workspace = true } +rand = { workspace = true } rayon = { workspace = true } regex = { workspace = true } rpassword = { workspace = true } diff --git a/cli/src/commands/gerrit/mod.rs b/cli/src/commands/gerrit/mod.rs new file mode 100644 index 0000000000..60abdb6702 --- /dev/null +++ b/cli/src/commands/gerrit/mod.rs @@ -0,0 +1,57 @@ +// Copyright 2024 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::fmt::Debug; + +use clap::Subcommand; + +use crate::cli_util::CommandHelper; +use crate::command_error::CommandError; +use crate::commands::gerrit; +use crate::ui::Ui; + +/// Interact with Gerrit Code Review. +#[derive(Subcommand, Clone, Debug)] +pub enum GerritCommand { + /// Send changes to Gerrit for code review, or update existing changes. + /// + /// Sending in a set of revisions to Gerrit creates a single "change" for + /// each revision included in the revset. This change is then available for + /// review on your Gerrit instance. + /// + /// This command modifies each commit in the revset to include a `Change-Id` + /// footer in its commit message if one does not already exist. Note that + /// this ID is NOT compatible with jj IDs, and is Gerrit-specific. + /// + /// If a change already exists for a given revision (i.e. it contains the + /// same `Change-Id`), this command will update the contents of the existing + /// change to match. + /// + /// Note: this command takes 1-or-more revsets arguments, each of which can + /// resolve to multiple revisions; so you may post trees or ranges of + /// commits to Gerrit for review all at once. + Send(gerrit::send::SendArgs), +} + +pub fn cmd_gerrit( + ui: &mut Ui, + command: &CommandHelper, + subcommand: &GerritCommand, +) -> Result<(), CommandError> { + match subcommand { + GerritCommand::Send(review) => gerrit::send::cmd_send(ui, command, review), + } +} + +mod send; diff --git a/cli/src/commands/gerrit/send.rs b/cli/src/commands/gerrit/send.rs new file mode 100644 index 0000000000..6bac4100d8 --- /dev/null +++ b/cli/src/commands/gerrit/send.rs @@ -0,0 +1,431 @@ +// Copyright 2024 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::fmt::Debug; +use std::io::Write; +use std::sync::Arc; + +use hex::ToHex; +use indexmap::IndexMap; +use itertools::Itertools as _; +use jj_lib::commit::Commit; +use jj_lib::commit::CommitIteratorExt; +use jj_lib::content_hash::blake2b_hash; +use jj_lib::footer::get_footer_lines; +use jj_lib::footer::FooterEntry; +use jj_lib::git::GitRefUpdate; +use jj_lib::git::{self}; +use jj_lib::repo::Repo; +use jj_lib::store::Store; + +use crate::cli_util::short_commit_hash; +use crate::cli_util::CommandHelper; +use crate::cli_util::RevisionArg; +use crate::command_error::user_error; +use crate::command_error::CommandError; +use crate::git_util::get_git_repo; +use crate::git_util::with_remote_git_callbacks; +use crate::git_util::GitSidebandProgressMessageWriter; +use crate::ui::Ui; + +#[derive(clap::Args, Clone, Debug)] +pub struct SendArgs { + /// The revset, selecting which commits are sent in to Gerrit. This can be + /// any arbitrary set of commits; they will be modified to include a + /// `Change-Id` footer if one does not already exist, and then sent off to + /// Gerrit for review. + #[arg(long, short = 'r')] + revisions: Vec, + + /// The location where your changes are intended to land. This should be + /// an upstream branch. + #[arg(long = "for", short = 'f')] + for_: Option, + + /// The Gerrit remote to push to. Can be configured with the `gerrit.remote` + /// repository option as well. This is typically a full SSH URL for your + /// Gerrit instance. + #[arg(long)] + remote: Option, + + /// 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, +} + +/// calculate push remote. The logic is: +/// 1. If the user specifies `--remote`, use that +/// 2. If the user has 'gerrit.remote' configured, use that +/// 3. If the user has a single remote, use that +/// 4. If the user has a remote named 'gerrit', use that +/// 5. otherwise, bail out +fn calculate_push_remote( + store: &Arc, + config: &config::Config, + remote: Option, +) -> Result { + let git_repo = get_git_repo(store)?; // will fail if not a git repo + let remotes = git_repo.remotes()?; + + // case 1 + if let Some(remote) = remote { + if remotes.iter().any(|r| r == Some(&remote)) { + return Ok(remote); + } + return Err(user_error(format!( + "The remote '{}' (specified via `--remote`) does not exist", + remote + ))); + } + + // case 2 + if let Ok(remote) = config.get_string("gerrit.default_remote") { + if remotes.iter().any(|r| r == Some(&remote)) { + return Ok(remote); + } + return Err(user_error(format!( + "The remote '{}' (configured via `gerrit.default_remote`) does not exist", + remote + ))); + } + + // case 3 + if remotes.len() == 1 { + return Ok(remotes.get(0).unwrap().to_owned()); + } + + // case 4 + if remotes.iter().any(|r| r == Some("gerrit")) { + return Ok("gerrit".to_owned()); + } + + // case 5 + Err(user_error( + "No remote specified, and no 'gerrit' remote was found", + )) +} + +/// Determine what Gerrit ref and remote to use. The logic is: +/// +/// 1. If the user specifies `--for branch`, use that +/// 2. If the user has 'gerrit.default_for' configured, use that +/// 3. Otherwise, bail out +fn calculate_push_ref( + config: &config::Config, + for_: Option, +) -> Result { + // case 1 + if let Some(for_) = for_ { + return Ok(for_); + } + + // case 2 + if let Ok(default_for) = config.get_string("gerrit.default_for") { + return Ok(default_for); + } + + // case 3 + Err(user_error( + "No target branch specified via --for, and no 'gerrit.default_for' was found", + )) +} + +pub fn cmd_send(ui: &mut Ui, command: &CommandHelper, send: &SendArgs) -> Result<(), CommandError> { + let mut workspace_command = command.workspace_helper(ui)?; + + let to_send: Vec<_> = workspace_command + .parse_union_revsets(ui, &send.revisions)? + .evaluate_to_commits()? + .try_collect()?; + if to_send.is_empty() { + writeln!(ui.status(), "No revisions to send.")?; + return Ok(()); + } + + if to_send + .iter() + .any(|commit| commit.id() == workspace_command.repo().store().root_commit_id()) + { + return Err(user_error("Cannot send the virtual 'root()' commit")); + } + + workspace_command.check_rewritable(to_send.iter().ids())?; + + let mut tx = workspace_command.start_transaction(); + let base_repo = tx.base_repo().clone(); + let mut_repo = tx.repo_mut(); + let store = base_repo.store(); + let git_repo = get_git_repo(store)?; // do this early: will fail if not a git repo + + let for_remote = + calculate_push_remote(store, command.settings().config(), send.remote.clone())?; + let for_branch = calculate_push_ref(command.settings().config(), send.for_.clone())?; + + // immediately error and reject any discardable commits, i.e. the + // the empty wcc + for commit in to_send.iter() { + if commit.is_discardable(mut_repo)? { + return Err(user_error(format!( + "Refusing to send 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 = IndexMap::new(); + for commit_id in to_send.iter().map(|c| c.id()).rev() { + let original_commit = store.get_commit(commit_id).unwrap(); + let description = original_commit.description().to_owned(); + let footer = get_footer_lines(&description); + + if !footer.is_empty() { + // first, figure out if there are multiple Change-Id fields; if so, then we + // error and continue + if footer.iter().filter(|entry| entry.0 == "Change-Id").count() > 1 { + writeln!( + ui.warning_default(), + "warning: multiple Change-Id footers in commit {}", + short_commit_hash(original_commit.id()), + )?; + continue; + } + + // now, look up the existing change id footer + let change_id = footer.iter().find(|entry| entry.0 == "Change-Id"); + if let Some(FooterEntry(_, cid)) = change_id { + // map the old commit to itself + old_to_new.insert(original_commit.clone(), (original_commit.clone(), false)); + + // check the change-id format is correct in any case + if cid.len() != 41 || !cid.starts_with('I') { + writeln!( + ui.warning_default(), + "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 send.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: the character 'I', followed by + // 40 characters of the blake2 hash of a random binary blob. we use the hash + // so that any instance of `ContentHash` can be used to generate a unique + // id, if we ever need it. + let mut rand_id: [u8; 32] = [0; 32]; + rand::Rng::fill(&mut rand::thread_rng(), &mut rand_id); + + let hashed_id: String = blake2b_hash(&rand_id).encode_hex(); + let gerrit_change_id = format!("I{}", hashed_id.chars().take(40).collect::()); + + // XXX (aseipp): move this description junk for rewriting the description to + // footer.rs; improves reusability and makes things a little cleaner + let spacing = if footer.is_empty() { "\n\n" } else { "\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() + .map(|parent| { + let p = parent.unwrap(); + if let Some((rewritten_parent, _)) = old_to_new.get(&p) { + rewritten_parent + } else { + &p + } + .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 sending 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 mut workspace_command = command.workspace_helper(ui)?; + let mut 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::>(); + 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 send.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())?; + + // 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? + let mut writer = GitSidebandProgressMessageWriter::new(ui); + let mut sideband_progress_callback = |msg: &[u8]| { + _ = writer.write(ui, msg); + }; + with_remote_git_callbacks(ui, Some(&mut sideband_progress_callback), |cb| { + git::push_updates( + tx.repo_mut(), + &git_repo, + &for_remote, + &[GitRefUpdate { + qualified_name: remote_ref.clone(), + expected_current_target: None, + 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_default(), + "warning: ref update rejected by gerrit; no changes to push (did you \ + forget to update, amend, or add new changes?)" + )?; + + Ok(()) + } + git::GitPushError::InternalGitError(err) => { + writeln!( + ui.warning_default(), + "warning: internal git error while pushing to gerrit: {}", + err + )?; + Err(user_error(err.to_string())) + } + // XXX (aseipp): more cases to handle here? + _ => Err(user_error(err.to_string())), + }, + Ok, + )?; + } + + Ok(()) +} diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index 25bf0a32f1..f89b7d717d 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -29,6 +29,7 @@ mod edit; mod evolog; mod file; mod fix; +mod gerrit; mod git; mod help; mod init; @@ -111,6 +112,8 @@ enum Command { Files(file::list::FileListArgs), Fix(fix::FixArgs), #[command(subcommand)] + Gerrit(gerrit::GerritCommand), + #[command(subcommand)] Git(git::GitCommand), Help(help::HelpArgs), Init(init::InitArgs), @@ -219,6 +222,7 @@ pub fn run_command(ui: &mut Ui, command_helper: &CommandHelper) -> Result<(), Co cmd(ui, command_helper, args) } Command::Fix(args) => fix::cmd_fix(ui, command_helper, args), + Command::Gerrit(sub_args) => gerrit::cmd_gerrit(ui, command_helper, sub_args), Command::Git(args) => git::cmd_git(ui, command_helper, args), Command::Help(args) => help::cmd_help(ui, command_helper, args), Command::Init(args) => init::cmd_init(ui, command_helper, args), diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index bcb2631437..5283b9cc83 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -334,6 +334,20 @@ } } }, + "gerrit": { + "type": "object", + "description": "Settings for interacting with Gerrit", + "properties": { + "default_remote": { + "type": "string", + "description": "The Gerrit remote to interact with" + }, + "default_for": { + "type": "string", + "description": "The default branch to propose changes for" + } + } + }, "merge-tools": { "type": "object", "description": "Tables of custom options to pass to the given merge tool (selected in ui.merge-editor)", diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 87e2f2c904..300d6c6609 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -44,6 +44,8 @@ This document contains the help content for the `jj` command-line program. * [`jj file track`↴](#jj-file-track) * [`jj file untrack`↴](#jj-file-untrack) * [`jj fix`↴](#jj-fix) +* [`jj gerrit`↴](#jj-gerrit) +* [`jj gerrit send`↴](#jj-gerrit-send) * [`jj git`↴](#jj-git) * [`jj git clone`↴](#jj-git-clone) * [`jj git export`↴](#jj-git-export) @@ -127,6 +129,7 @@ To get started, see the tutorial at https://martinvonz.github.io/jj/latest/tutor * `evolog` — Show how a change has evolved over time * `file` — File operations * `fix` — Update files with formatting fixes or other changes +* `gerrit` — Interact with Gerrit Code Review * `git` — Commands for working with Git remotes and the underlying Git repo * `help` — Print this message or the help of the given subcommand(s) * `init` — Create a new repo in the given directory @@ -965,6 +968,44 @@ will be removed in a future version. +## `jj gerrit` + +Interact with Gerrit Code Review + +**Usage:** `jj gerrit ` + +###### **Subcommands:** + +* `send` — Send changes to Gerrit for code review, or update existing changes + + + +## `jj gerrit send` + +Send changes to Gerrit for code review, or update existing changes. + +Sending in a set of revisions to Gerrit creates a single "change" for each revision included in the revset. This change is then available for review on your Gerrit instance. + +This command modifies each commit in the revset to include a `Change-Id` footer in its commit message if one does not already exist. Note that this ID is NOT compatible with jj IDs, and is Gerrit-specific. + +If a change already exists for a given revision (i.e. it contains the same `Change-Id`), this command will update the contents of the existing change to match. + +Note: this command takes 1-or-more revsets arguments, each of which can resolve to multiple revisions; so you may post trees or ranges of commits to Gerrit for review all at once. + +**Usage:** `jj gerrit send [OPTIONS]` + +###### **Options:** + +* `-r`, `--revisions ` — The revset, selecting which commits are sent in to Gerrit. This can be any arbitrary set of commits; they will be modified to include a `Change-Id` footer if one does not already exist, and then sent off to Gerrit for review +* `-f`, `--for ` — The location where your changes are intended to land. This should be an upstream branch +* `--remote ` — The Gerrit remote to push to. Can be configured with the `gerrit.remote` repository option as well. This is typically a full SSH URL for your Gerrit instance +* `-n`, `--dry-run` — If true, do not actually add `Change-Id`s to commits, and do not push the changes to Gerrit + + Possible values: `true`, `false` + + + + ## `jj git` Commands for working with Git remotes and the underlying Git repo