Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: In the commit description you could mention that "you" (i.e the chromium project) scripted around that which is imo a good motivation.

Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
to fetch during clone. If present, the first matching branch is used as the
working-copy parent.

* Added the first hook to jj - a pre-upload hook that runs `jj fix` before
uploading. Toggled via the config flag `hooks.pre-upload-fix`.
Comment on lines +73 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is missing that it only runs for gerrit upload which isn't such a common workflow for most people.


### Fixed bugs

* `jj metaedit --author-timestamp` twice with the same value no longer
Expand Down
7 changes: 7 additions & 0 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1652,6 +1652,13 @@ to the current parents may contain changes from multiple commits.
)
}

pub fn commits(&self, commits: Vec<CommitId>) -> Result<Vec<Commit>, CommandError> {
Ok(self
.attach_revset_evaluator(RevsetExpression::commits(commits))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to use a revset for this. Maybe a better way would be to use self.repo().store().get_commit(commit_id) directly? That's what evaluate_to_commits() uses internally anyway.

.evaluate_to_commits()?
.try_collect()?)
}

pub fn id_prefix_context(&self) -> &IdPrefixContext {
self.user_repo
.id_prefix_context
Expand Down
33 changes: 26 additions & 7 deletions cli/src/commands/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 root_commits: Vec<CommitId> = if args.source.is_empty() {
let revs = workspace_command.settings().get_string("revsets.fix")?;
workspace_command.parse_revset(ui, &RevisionArg::from(revs))?
Expand All @@ -147,6 +145,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<HashMap<CommitId, CommitId>, 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(
Expand All @@ -160,9 +178,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,
)
Expand All @@ -173,7 +191,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
Expand Down
15 changes: 13 additions & 2 deletions cli/src/commands/gerrit/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -167,7 +169,7 @@ pub fn cmd_gerrit_upload(
) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;

let revisions: Vec<_> = workspace_command
let mut revisions: Vec<_> = workspace_command
.parse_union_revsets(ui, &args.revisions)?
.evaluate_to_commit_ids()?
.try_collect()?;
Expand All @@ -183,7 +185,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<Commit> = workspace_command
let mut to_upload: Vec<_> = workspace_command
.attach_revset_evaluator(
workspace_command
.env()
Expand All @@ -193,6 +195,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
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
11 changes: 11 additions & 0 deletions cli/src/config-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,17 @@
}
}
},
"hooks": {
"type": "object",
"description": "Settings for jj hooks",
"properties": {
"pre-upload-fix": {
"type": "boolean",
"description": "Whether to run `jj fix` before uploading commits",
"default": false
}
}
},
"--when": {
"type": "object",
"description": "Conditions restriction the application of the configuration",
Expand Down
67 changes: 67 additions & 0 deletions cli/src/hooks.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// 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 jj_lib::backend::CommitId;
use jj_lib::fileset::FilesetExpression;

use crate::cli_util::CommandHelper;
use crate::command_error::CommandError;
use crate::commands::fix::fix_revisions;
use crate::ui::Ui;

/// 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`.
Comment on lines +28 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should advertise this as "general hook" support. Since it is the only support we'll ever have

///
/// 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,
root_commits: &[CommitId],
) -> Result<HashMap<CommitId, CommitId>, CommandError> {
Ok(if command.settings().get_bool("hooks.pre-upload-fix")? {
fix_revisions(
ui,
workspace_command,
root_commits,
&FilesetExpression::all().to_matcher(),
false,
)?
} else {
Default::default()
})
}

pub(crate) fn apply_rewrites(
rewrites: &HashMap<CommitId, CommitId>,
commits: Vec<CommitId>,
) -> Vec<CommitId> {
commits
.into_iter()
.map(|c| rewrites.get(&c).cloned().unwrap_or(c))
.collect()
}
1 change: 1 addition & 0 deletions cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
76 changes: 76 additions & 0 deletions cli/tests/test_gerrit_upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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 = 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]
"###);
}
10 changes: 10 additions & 0 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1746,6 +1746,16 @@ 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.

The special config flag `hooks.pre-upload-fix = true` flag can be set to run
`jj fix` before uploading your code.

Support for more generic hooks will be added in the future.
Comment on lines +1752 to +1757
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is also missing the major caveat of it being gerrit upload only


## Ways to specify `jj` config: details

### User config files
Expand Down
3 changes: 3 additions & 0 deletions lib/src/config/misc.toml
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,6 @@ name = ""

[working-copy]
eol-conversion = "none"

[hooks]
pre-upload-fix = false