From bd9ddcf745792a87a4711a06b956009fe5a5595b Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Thu, 18 Apr 2024 15:10:06 -0500 Subject: [PATCH 1/5] lib: add `[T; N]` instance for `ContentHash` Natural extension of the existing `[T]` instance. Signed-off-by: Austin Seipp Change-Id: Ib7f6fd829096b2cac8e3d7b9471a92ddb76a621b --- lib/src/content_hash.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/src/content_hash.rs b/lib/src/content_hash.rs index e3918589af..6546149828 100644 --- a/lib/src/content_hash.rs +++ b/lib/src/content_hash.rs @@ -79,6 +79,15 @@ impl ContentHash for [T] { } } +impl ContentHash for [T; N] { + fn hash(&self, state: &mut impl DigestUpdate) { + state.update(&(N as u64).to_le_bytes()); + for x in self { + x.hash(state); + } + } +} + impl ContentHash for Vec { fn hash(&self, state: &mut impl DigestUpdate) { self.as_slice().hash(state) From 20b4df2b8b2a79c6c852f70cf54a9279f13e43d0 Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Wed, 17 Jan 2024 19:57:43 -0600 Subject: [PATCH 2/5] lib: add `footer` module for commit footers To be used for parsing `Change-Id`s from commits, in service of Gerrit support. Signed-off-by: Austin Seipp Change-Id: I434d76b1229b36b815622ad7409ced3a405cbe22 --- lib/src/footer.rs | 132 ++++++++++++++++++++++++++++++++++++++++++++++ lib/src/lib.rs | 1 + 2 files changed, 133 insertions(+) create mode 100644 lib/src/footer.rs diff --git a/lib/src/footer.rs b/lib/src/footer.rs new file mode 100644 index 0000000000..b7e2c5dd05 --- /dev/null +++ b/lib/src/footer.rs @@ -0,0 +1,132 @@ +// 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. + +//! Parsing footer lines from commit messages. + +/// A key-value pair representing a footer line in a commit message, of the +/// form `Key: Value`. +#[derive(Debug, PartialEq, Clone)] +pub struct FooterEntry(pub String, pub String); + +/// Parse the footer lines from a commit message; these are simple key-value +/// pairs, separated by a colon, describing extra information in a commit +/// message; an example is the following: +/// +/// ```text +/// chore: fix bug 1234 +/// +/// Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod +/// tempor incididunt ut labore et dolore magna aliqua. +/// +/// Co-authored-by: Alice +/// Co-authored-by: Bob +/// Reviewed-by: Charlie +/// Change-Id: I1234567890abcdef1234567890abcdef12345678 +/// ``` +/// +/// In this case, there are four footer lines: two `Co-authored-by` lines, one +/// `Reviewed-by` line, and one `Change-Id` line. +pub fn get_footer_lines(body: &str) -> Vec { + // a footer always comes at the end of a message; we can split the message + // by newline, but we need to immediately reverse the order of the lines + // to ensure we parse the footer in an unambiguous manner; this avoids cases + // where a colon in the body of the message is mistaken for a footer line + + let lines = body.trim().lines().rev().collect::>(); + + // short-circuit if there is only 1 line; this avoids a case where a commit + // with a single-line description like 'cli: fix bug' does not have a + // footer, but would otherwise be mistaken for a footer line + if lines.len() <= 1 { + return vec![]; + } + + let mut footer: Vec = Vec::new(); + for line in lines { + if line.is_empty() { + break; + } + if let Some((key, value)) = line.split_once(": ") { + let key = key.trim(); + let value = value.trim(); + footer.push(FooterEntry(key.to_string(), value.to_string())); + } else { + break; + } + } + + // reverse the insert order, since we parsed the footer in reverse + footer.reverse(); + + if footer.is_empty() { + vec![] + } else { + footer + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_simple_footer_lines() { + let body = r#"chore: fix bug 1234 + +Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed +do eiusmod tempor incididunt ut labore et dolore magna aliqua. + +Acked-by: Austin Seipp +Reviewed-by: Yuya Nishihara +Reviewed-by: Martin von Zweigbergk +Change-Id: I1234567890abcdef1234567890abcdef12345678"#; + + let footer = get_footer_lines(body); + assert_eq!(footer.len(), 4); + + assert_eq!(footer.first().unwrap().1, "Austin Seipp "); + assert_eq!(footer.get(1).unwrap().1, "Yuya Nishihara "); + assert_eq!( + footer.get(2).unwrap().1, + "Martin von Zweigbergk " + ); + assert_eq!( + footer.get(3).unwrap().1, + "I1234567890abcdef1234567890abcdef12345678" + ); + } + + #[test] + fn test_footer_lines_with_colon_in_body() { + let body = r#"chore: fix bug 1234 + +Summary: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod +tempor incididunt ut labore et dolore magna aliqua. + +Change-Id: I1234567890abcdef1234567890abcdef12345678"#; + + let footer = get_footer_lines(body); + + // should only have Change-Id + assert_eq!(footer.len(), 1); + assert_eq!(footer.first().unwrap().0, "Change-Id"); + } + + #[test] + fn test_footer_lines_with_single_line_description() { + let body = r#"chore: fix bug 1234"#; + let footer = get_footer_lines(body); + assert_eq!(footer.len(), 0); + } +} diff --git a/lib/src/lib.rs b/lib/src/lib.rs index 60172d7622..05b480788f 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -44,6 +44,7 @@ pub mod files; pub mod fileset; mod fileset_parser; pub mod fmt_util; +pub mod footer; pub mod fsmonitor; #[cfg(feature = "git")] pub mod git; From 11ed301f853e18c708992baf0c91f7651e0dc147 Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Thu, 18 Jan 2024 00:35:09 -0600 Subject: [PATCH 3/5] 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 | 1 + cli/Cargo.toml | 3 +- 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, 550 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 7328185026..f5993d72f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,6 +1850,7 @@ dependencies = [ "pest", "pest_derive", "pollster", + "rand", "rayon", "regex", "rpassword", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index b6b7f59688..04a2eebb50 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]] @@ -75,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..000e69f445 --- /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(&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 52c03c1743..e071a37138 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 init; mod interdiff; @@ -108,6 +109,8 @@ enum Command { Files(file::list::FileListArgs), Fix(fix::FixArgs), #[command(subcommand)] + Gerrit(gerrit::GerritCommand), + #[command(subcommand)] Git(git::GitCommand), Init(init::InitArgs), Interdiff(interdiff::InterdiffArgs), @@ -212,6 +215,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::Init(args) => init::cmd_init(ui, command_helper, args), Command::Interdiff(args) => interdiff::cmd_interdiff(ui, command_helper, args), diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 997d62c829..64f8dd5635 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 8d255d7af6..f37a80988d 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -43,6 +43,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) @@ -124,6 +126,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 * `init` — Create a new repo in the given directory * `interdiff` — Compare the changes of two commits @@ -922,6 +925,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 From fcfd793103b1e43b69ec56bc7da2e7deea67bbb6 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Sat, 2 Mar 2024 00:43:48 +0800 Subject: [PATCH 4/5] git: vendor custom prerelease version of `libgit2` --- Cargo.lock | 6 ++---- Cargo.toml | 4 ++-- flake.nix | 3 +++ 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 04d05d1a45..d00bac4606 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -925,8 +925,7 @@ checksum = "40ecd4077b5ae9fd2e9e169b102c6c330d0605168eb0e8bf79952b256dbefffd" [[package]] name = "git2" version = "0.19.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b903b73e45dc0c6c596f2d37eccece7c1c8bb6e4407b001096387c63d0d93724" +source = "git+https://github.com/bnjmnt4n/git2-rs.git?rev=60e29ff0d#60e29ff0d84cdffd9f366455d32606e582a4c378" dependencies = [ "bitflags 2.6.0", "libc", @@ -1987,8 +1986,7 @@ checksum = "561d97a539a36e26a9a5fad1ea11a3039a67714694aaa379433e580854bc3dc5" [[package]] name = "libgit2-sys" version = "0.17.0+1.8.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "10472326a8a6477c3c20a64547b0059e4b0d086869eee31e6d7da728a8eb7224" +source = "git+https://github.com/bnjmnt4n/git2-rs.git?rev=60e29ff0d#60e29ff0d84cdffd9f366455d32606e582a4c378" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index cc384a8fd3..2c0ca6cd07 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,7 +49,7 @@ dunce = "1.0.5" either = "1.13.0" esl01-renderdag = "0.3.0" futures = "0.3.31" -git2 = { version = "0.19.0", features = [ +git2 = { git = "https://github.com/bnjmnt4n/git2-rs.git", rev = "60e29ff0d", features = [ # Do *not* disable this feature even if you'd like dynamic linking. Instead, # set the environment variable `LIBGIT2_NO_VENDOR=1` if dynamic linking must # be used (this will override the Cargo feature), and allow static linking @@ -140,7 +140,7 @@ implicit_clone = "warn" needless_for_each = "warn" semicolon_if_nothing_returned = "warn" uninlined_format_args = "warn" - + # Insta suggests compiling these packages in opt mode for faster testing. # See https://docs.rs/insta/latest/insta/#optional-faster-runs. [profile.dev.package] diff --git a/flake.nix b/flake.nix index 7089f58129..ff05c33d16 100644 --- a/flake.nix +++ b/flake.nix @@ -85,6 +85,9 @@ ]; cargoLock.lockFile = ./Cargo.lock; + cargoLock.outputHashes = { + "git2-0.19.0" = "sha256-fV8dFChGeDhb20bMyqefpAD5/+raQQ2sMdkEtlA1jaE="; + }; nativeBuildInputs = with pkgs; [ gzip installShellFiles From 1d7aaa6f8d8db233e25fa4a3627506f280a8c2ea Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Sat, 2 Mar 2024 00:43:48 +0800 Subject: [PATCH 5/5] git: enable `libgit2`'s OpenSSH feature --- Cargo.lock | 15 --------------- Cargo.toml | 4 +++- cli/src/commands/git/mod.rs | 4 ++-- flake.nix | 6 ++---- 4 files changed, 7 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d00bac4606..4c06e4bd66 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1990,7 +1990,6 @@ source = "git+https://github.com/bnjmnt4n/git2-rs.git?rev=60e29ff0d#60e29ff0d84c dependencies = [ "cc", "libc", - "libssh2-sys", "libz-sys", "openssl-sys", "pkg-config", @@ -2007,20 +2006,6 @@ dependencies = [ "redox_syscall", ] -[[package]] -name = "libssh2-sys" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2dc8a030b787e2119a731f1951d6a773e2280c660f8ec4b0f5e1505a386e71ee" -dependencies = [ - "cc", - "libc", - "libz-sys", - "openssl-sys", - "pkg-config", - "vcpkg", -] - [[package]] name = "libz-sys" version = "1.1.20" diff --git a/Cargo.toml b/Cargo.toml index 2c0ca6cd07..1d580c23d2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,7 +49,9 @@ dunce = "1.0.5" either = "1.13.0" esl01-renderdag = "0.3.0" futures = "0.3.31" -git2 = { git = "https://github.com/bnjmnt4n/git2-rs.git", rev = "60e29ff0d", features = [ +git2 = { git = "https://github.com/bnjmnt4n/git2-rs.git", rev = "60e29ff0d", default-features = false, features = [ + "https", + "ssh-openssh", # Do *not* disable this feature even if you'd like dynamic linking. Instead, # set the environment variable `LIBGIT2_NO_VENDOR=1` if dynamic linking must # be used (this will override the Cargo feature), and allow static linking diff --git a/cli/src/commands/git/mod.rs b/cli/src/commands/git/mod.rs index 2a418751b6..7d6df34b14 100644 --- a/cli/src/commands/git/mod.rs +++ b/cli/src/commands/git/mod.rs @@ -90,8 +90,8 @@ fn map_git_error(err: git2::Error) -> CommandError { successfully load certificates. Try setting it to the path of a directory that \ contains a `.ssh` directory." } else { - "Jujutsu uses libssh2, which doesn't respect ~/.ssh/config. Does `ssh -F \ - /dev/null` to the host work?" + "There was an error creating an SSH connection. Does `ssh -F /dev/null` to the \ + host work?" }; user_error_with_hint(err, hint) diff --git a/flake.nix b/flake.nix index ff05c33d16..fd4f98e635 100644 --- a/flake.nix +++ b/flake.nix @@ -99,11 +99,10 @@ openssh ] ++ linuxNativeDeps; buildInputs = with pkgs; [ - openssl zstd libgit2 libssh2 + openssl zstd libgit2 openssh ] ++ darwinDeps; ZSTD_SYS_USE_PKG_CONFIG = "1"; - LIBSSH2_SYS_USE_PKG_CONFIG = "1"; RUSTFLAGS = pkgs.lib.optionalString pkgs.stdenv.isLinux "-C link-arg=-fuse-ld=mold"; NIX_JJ_GIT_HASH = self.rev or ""; CARGO_INCREMENTAL = "0"; @@ -153,7 +152,7 @@ ourRustVersion # Foreign dependencies - openssl zstd libgit2 libssh2 + openssl zstd libgit2 pkg-config # Additional tools recommended by contributing.md @@ -179,7 +178,6 @@ shellHook = '' export RUST_BACKTRACE=1 export ZSTD_SYS_USE_PKG_CONFIG=1 - export LIBSSH2_SYS_USE_PKG_CONFIG=1 export RUSTFLAGS="-Zthreads=0 ${rustLinkFlagsString}" '';