diff --git a/CHANGELOG.md b/CHANGELOG.md index a40b565df1..0c2557ceff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). colocation state (`status`) and to convert a non-colocated git repo into a colocated repo (`enable`) and vice-versa `disable`. +* Added pre-upload hooks to jj. For now, this is limited in scope to a hook + named fix, and only works for `jj gerrit upload`. + Toggled via the config flag `hooks.pre-upload.fix.enabled`. + ### Fixed bugs * `jj metaedit --author-timestamp` twice with the same value no longer diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 16160f2bd1..55b98abca5 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1652,6 +1652,14 @@ to the current parents may contain changes from multiple commits. ) } + pub fn commits(&self, commits: Vec) -> BackendResult> { + let store = self.repo().store(); + commits + .into_iter() + .map(|id| store.get_commit(&id)) + .try_collect() + } + pub fn id_prefix_context(&self) -> &IdPrefixContext { self.user_repo .id_prefix_context diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 367f5107dc..75f4e85e54 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -38,6 +38,7 @@ use tracing::instrument; use crate::cli_util::CommandHelper; use crate::cli_util::RevisionArg; +use crate::cli_util::WorkspaceCommandHelper; use crate::command_error::CommandError; use crate::command_error::config_error; use crate::command_error::print_parse_diagnostics; @@ -131,9 +132,6 @@ pub(crate) fn cmd_fix( args: &FixArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let workspace_root = workspace_command.workspace_root().to_owned(); - let path_converter = workspace_command.path_converter().to_owned(); - let tools_config = get_tools_config(ui, workspace_command.settings())?; let target_expr = if args.source.is_empty() { let revs = workspace_command.settings().get_string("revsets.fix")?; workspace_command.parse_revset(ui, &RevisionArg::from(revs))? @@ -150,6 +148,26 @@ pub(crate) fn cmd_fix( .parse_file_patterns(ui, &args.paths)? .to_matcher(); + fix_revisions( + ui, + &mut workspace_command, + &root_commits, + &matcher, + args.include_unchanged_files, + )?; + Ok(()) +} + +pub(crate) fn fix_revisions( + ui: &mut Ui, + workspace_command: &mut WorkspaceCommandHelper, + root_commits: &[CommitId], + matcher: &dyn Matcher, + include_unchanged_files: bool, +) -> Result, CommandError> { + let workspace_root = workspace_command.workspace_root().to_owned(); + let path_converter = workspace_command.path_converter().to_owned(); + let tools_config = get_tools_config(ui, workspace_command.settings())?; let mut tx = workspace_command.start_transaction(); let mut parallel_fixer = ParallelFileFixer::new(|store, file_to_fix| { fix_one_file( @@ -163,9 +181,9 @@ pub(crate) fn cmd_fix( .block_on() }); let summary = fix_files( - root_commits, - &matcher, - args.include_unchanged_files, + root_commits.to_vec(), + matcher, + include_unchanged_files, tx.repo_mut(), &mut parallel_fixer, ) @@ -176,7 +194,8 @@ pub(crate) fn cmd_fix( summary.num_fixed_commits, summary.num_checked_commits )?; - tx.finish(ui, format!("fixed {} commits", summary.num_fixed_commits)) + tx.finish(ui, format!("fixed {} commits", summary.num_fixed_commits))?; + Ok(summary.rewrites) } /// Invokes all matching tools (if any) to file_to_fix. If the content is diff --git a/cli/src/commands/gerrit/upload.rs b/cli/src/commands/gerrit/upload.rs index b79e759276..ddbc5483d8 100644 --- a/cli/src/commands/gerrit/upload.rs +++ b/cli/src/commands/gerrit/upload.rs @@ -41,6 +41,8 @@ use crate::command_error::user_error; use crate::command_error::user_error_with_hint; use crate::command_error::user_error_with_message; use crate::git_util::with_remote_git_callbacks; +use crate::hooks::apply_rewrites; +use crate::hooks::run_pre_upload_hooks; use crate::ui::Ui; /// Upload changes to Gerrit for code review, or update existing changes. @@ -171,7 +173,7 @@ pub fn cmd_gerrit_upload( .parse_union_revsets(ui, &args.revisions)? .resolve()?; workspace_command.check_rewritable_expr(&target_expr)?; - let revisions: Vec<_> = target_expr + let mut revisions: Vec<_> = target_expr .evaluate(workspace_command.repo().as_ref())? .iter() .try_collect()?; @@ -185,7 +187,7 @@ pub fn cmd_gerrit_upload( // has a Change-ID. // We make an assumption here that all immutable commits already have a // Change-ID. - let to_upload: Vec = workspace_command + let mut to_upload: Vec<_> = workspace_command .attach_revset_evaluator( workspace_command .env() @@ -195,6 +197,15 @@ pub fn cmd_gerrit_upload( .evaluate_to_commits()? .try_collect()?; + { + let ids: Vec<_> = to_upload.iter().map(|c| c.id().clone()).collect(); + let rewrites = run_pre_upload_hooks(ui, command, &mut workspace_command, &ids)?; + if !rewrites.is_empty() { + revisions = apply_rewrites(&rewrites, revisions); + to_upload = workspace_command.commits(apply_rewrites(&rewrites, ids))?; + } + } + // Note: This transaction is intentionally never finished. This way, the // Change-Id is never part of the commit description in jj. // This avoids scenarios where you have many commits with the same diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index 1b03b6dbb5..0a638839ba 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -28,7 +28,7 @@ mod duplicate; mod edit; mod evolog; mod file; -mod fix; +pub(crate) mod fix; #[cfg(feature = "git")] mod gerrit; #[cfg(feature = "git")] diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index cf473dcd2f..e150e14a9e 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -939,6 +939,32 @@ } } }, + "hooks": { + "type": "object", + "description": "Settings for jj hooks", + "properties": { + "pre-upload": { + "type": "object", + "additionalProperties": { + "type": "object", + "description": "Each pre-upload tool will run once, and be passed all commits to be uploaded in reverse jj log order (topological)", + "properties": { + "enabled": { + "type": "boolean", + "description": "Disables this tool if set to false", + "default": true + }, + "order": { + "type": "integer", + "description": "Hooks are run in ascending order on (order, name)", + "default": 0 + } + } + }, + "description": "Settings for pre-upload hooks" + } + } + }, "--when": { "type": "object", "description": "Conditions restriction the application of the configuration", diff --git a/cli/src/hooks.rs b/cli/src/hooks.rs new file mode 100644 index 0000000000..ddc95ad5b5 --- /dev/null +++ b/cli/src/hooks.rs @@ -0,0 +1,151 @@ +// Copyright 2025 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::collections::HashMap; + +use itertools::Itertools as _; +use jj_lib::backend::CommitId; +use jj_lib::fileset::FilesetExpression; +use jj_lib::settings::UserSettings; + +use crate::cli_util::CommandHelper; +use crate::command_error::CommandError; +use crate::command_error::user_error; +use crate::commands::fix::fix_revisions; +use crate::ui::Ui; + +/// A hook in the `hooks.pre-upload` config table. +enum PreUploadToolConfig { + FixTool, +} + +impl PreUploadToolConfig { + fn name(&self) -> &str { + match self { + Self::FixTool => "fix", + } + } +} + +/// Parses the `hooks.pre-upload` config table. +fn pre_upload_tools(settings: &UserSettings) -> Result, CommandError> { + fn default_true() -> bool { + true + } + + // Simplifies deserialization of the config values while building a ToolConfig. + #[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)] + #[serde(rename_all = "kebab-case")] + struct RawPreUploadToolConfig { + #[serde(default = "default_true")] + enabled: bool, + + order: i32, + } + + let mut tools: Vec<_> = vec![]; + for name in settings + .table_keys("hooks.pre-upload") + // Sort keys so errors are deterministic. + .sorted() + { + let tool: RawPreUploadToolConfig = settings.get(["hooks", "pre-upload", name])?; + if tool.enabled { + tools.push(( + (tool.order, name), + if name == "fix" { + PreUploadToolConfig::FixTool + } else { + return Err(user_error( + "Generic pre-upload hooks are currently unsupported. Only fix is \ + supported for now", + )); + }, + )); + } + } + Ok(tools + .into_iter() + .sorted_by(|lhs, rhs| Ord::cmp(&lhs.0, &rhs.0)) + .map(|(_, value)| value) + .collect()) +} + +/// Triggered every time a user runs something that semantically approximates +/// an "upload". +/// +/// Currently, this triggers on `jj gerrit upload`. Other forges which +/// implement custom upload scripts should also call this. +/// +/// This should ideally work for `jj git push` too, but doing so has +/// consequences. `git push` can be used to upload to code review, but it can +/// do many other things as well. We need to ensure the UX works well before +/// adding it to `git push`. +/// +/// This function may create transactions that rewrite commits, so is not +/// allowed to be called while a transaction is ongoing. +/// It returns a mapping of rewrites, and users are expected to update any +/// references to point at the new revision. +pub(crate) fn run_pre_upload_hooks( + ui: &mut Ui, + command: &CommandHelper, + workspace_command: &mut crate::cli_util::WorkspaceCommandHelper, + commit_ids: &[CommitId], +) -> Result, CommandError> { + let mut rewrites: HashMap = Default::default(); + let mut rewrites_reversed: HashMap = Default::default(); + let mut current_commits: Vec = commit_ids.to_vec(); + for tool in pre_upload_tools(command.settings())? { + // Tools must perform 1-1 rewrites. + // They cannot split commits (1-2), squash commits (2-1), or abandon commits + // (1-0). We deal with any instance of the above. + let next_rewrites = match tool { + PreUploadToolConfig::FixTool => fix_revisions( + ui, + workspace_command, + commit_ids, + &FilesetExpression::all().to_matcher(), + false, + )?, + }; + current_commits = current_commits + .into_iter() + .map(|c| next_rewrites.get(&c).cloned().unwrap_or(c)) + .collect(); + for (from, to) in next_rewrites { + let from = rewrites_reversed.get(&from).cloned().unwrap_or(from); + rewrites.insert(from, to); + } + rewrites_reversed = Default::default(); + for (from, to) in &rewrites { + if rewrites_reversed.insert(from.clone(), to.clone()).is_some() { + return Err(user_error(format!( + "The pre-upload tool {} combined two commits into one", + tool.name() + ))); + }; + } + } + Ok(rewrites) +} + +pub(crate) fn apply_rewrites( + rewrites: &HashMap, + commits: Vec, +) -> Vec { + commits + .into_iter() + .map(|c| rewrites.get(&c).cloned().unwrap_or(c)) + .collect() +} diff --git a/cli/src/lib.rs b/cli/src/lib.rs index ef3be98157..c3195241b4 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -39,6 +39,7 @@ pub mod git_util { } } pub mod graphlog; +pub mod hooks; pub mod merge_tools; pub mod movement_util; pub mod operation_templater; diff --git a/cli/tests/test_gerrit_upload.rs b/cli/tests/test_gerrit_upload.rs index aa1451495f..0e926d8149 100644 --- a/cli/tests/test_gerrit_upload.rs +++ b/cli/tests/test_gerrit_upload.rs @@ -14,6 +14,7 @@ use crate::common::TestEnvironment; use crate::common::create_commit; +use crate::common::to_toml_value; #[test] fn test_gerrit_upload_dryrun() { @@ -170,3 +171,78 @@ fn test_gerrit_upload_local() { [EOF] "###); } + +#[test] +fn test_pre_upload_fix_hook() { + let test_env = TestEnvironment::default(); + test_env + .run_jj_in(".", ["git", "init", "--colocate", "remote"]) + .success(); + let remote_dir = test_env.work_dir("remote"); + create_commit(&remote_dir, "a", &[]); + + test_env + .run_jj_in(".", ["git", "clone", "remote", "local"]) + .success(); + let local_dir = test_env.work_dir("local"); + local_dir.write_file("file.txt", "content\n"); + local_dir.run_jj(["commit", "-m", "a"]).success(); + local_dir.write_file("file.txt", "new content\n"); + local_dir.run_jj(["describe", "-m", "b"]).success(); + + let formatter_path = assert_cmd::cargo::cargo_bin("fake-formatter"); + assert!(formatter_path.is_file()); + let formatter = to_toml_value(formatter_path.to_str().unwrap()); + + test_env.add_config(format!( + r###" + hooks.pre-upload.fix.enabled = true + + [fix.tools.my-tool] + command = [{formatter}, "--uppercase"] + patterns = ["file.txt"] + "### + )); + + let output = local_dir.run_jj(["gerrit", "upload", "--remote-branch=main", "-r", "@"]); + insta::assert_snapshot!(output, @r###" + ------- stderr ------- + Fixed 2 commits of 2 checked. + Working copy (@) now at: mzvwutvl ed78d33d b + Parent commit (@-) : zsuskuln 531e34f0 a + Added 0 files, modified 1 files, removed 0 files + + Found 1 heads to push to Gerrit (remote 'origin'), target branch 'main' + + Pushing mzvwutvl ed78d33d b + [EOF] + "###); + + assert_eq!(local_dir.read_file("file.txt"), "NEW CONTENT\n"); + let output = local_dir.run_jj(["file", "show", "file.txt", "-r", "@"]); + insta::assert_snapshot!(output, @r###" + NEW CONTENT + [EOF] + "###); + let output = local_dir.run_jj(["file", "show", "file.txt", "-r", "@-"]); + insta::assert_snapshot!(output, @r###" + CONTENT + [EOF] + "###); + + // The revision doesn't show up on the remote in jj. + // Not sure why, but it doesn't really matter. + let output = remote_dir.run_jj([ + "util", + "exec", + "--", + "git", + "cat-file", + "-p", + "refs/for/main:file.txt", + ]); + insta::assert_snapshot!(output, @r###" + NEW CONTENT + [EOF] + "###); +} diff --git a/docs/config.md b/docs/config.md index b0bb0abcda..54acbb53a4 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1747,6 +1747,27 @@ eol-conversion = "input-output" [`gitoxide`][gitoxide-is-binary] or [`git`][git-is-binary]. Jujutsu doesn't plan to align the binary detection logic with git. +## Hook settings + +### Pre-upload hooks +Pre-upload hooks are triggered when you upload your code for code review. This +hook is only run during `jj gerrit upload`. + +Hooks are configured via the `hooks.pre-upload.` table, similar to fix +tools. Relevant options are: +* `enabled`: Defaults to `true`. If disabled, the hook will not run. +* `order`: Defaults to `0`. Hooks will run in ascending order. Hooks with equal + order will run in order of name. + +#### Predefined pre-upload hooks +These hooks support the `enabled` and `order` flags. They are disabled by +default. + +* `hooks.pre-upload.fix`: Runs `jj fix` before uploading. + +#### Generic pre-upload hooks +These are not yet supported. + ## Ways to specify `jj` config: details ### User config files diff --git a/lib/src/config/misc.toml b/lib/src/config/misc.toml index 5e7a1b0330..b3bcd5eb2e 100644 --- a/lib/src/config/misc.toml +++ b/lib/src/config/misc.toml @@ -52,3 +52,8 @@ name = "" [working-copy] eol-conversion = "none" + +[hooks.pre-upload] +# Run before other hooks by default. +fix.order = -100 +fix.enabled = false