diff --git a/CHANGELOG.md b/CHANGELOG.md index 26ce0775aa..98b6c904b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed bugs +* `jj git push` now ignores immutable commits when checking whether a + to-be-pushed commit has conflicts, or has no description / committer / author + set. [#3029](https://github.com/martinvonz/jj/issues/3029) + ## [0.18.0] - 2024-06-05 ### Breaking changes diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index 40596b65aa..58122f3122 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -25,7 +25,7 @@ use jj_lib::refs::{ classify_branch_push_action, BranchPushAction, BranchPushUpdate, LocalAndRemoteRef, }; use jj_lib::repo::Repo; -use jj_lib::revset::{self, RevsetExpression, RevsetIteratorExt as _}; +use jj_lib::revset::RevsetExpression; use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::str_util::StringPattern; use jj_lib::view::View; @@ -37,6 +37,7 @@ use crate::cli_util::{ use crate::command_error::{user_error, user_error_with_hint, CommandError}; use crate::commands::git::{get_single_remote, map_git_error}; use crate::git_util::{get_git_repo, with_remote_git_callbacks, GitSidebandProgressMessageWriter}; +use crate::revset_util; use crate::ui::Ui; /// Push to a Git remote @@ -266,18 +267,22 @@ pub fn cmd_git_push( .iter() .filter_map(|(_, update)| update.new_target.clone()) .collect_vec(); - let mut old_heads = repo + let old_heads = repo .view() .remote_branches(&remote) .flat_map(|(_, old_head)| old_head.target.added_ids()) .cloned() .collect_vec(); - if old_heads.is_empty() { - old_heads.push(repo.store().root_commit_id().clone()); - } - for commit in revset::walk_revs(repo.as_ref(), &new_heads, &old_heads)? - .iter() - .commits(repo.store()) + // (old_heads | immutable_heads() | root())..new_heads + let commits_to_push = RevsetExpression::commits(old_heads) + .union(&revset_util::parse_immutable_heads_expression( + &tx.base_workspace_helper().revset_parse_context(), + )?) + .range(&RevsetExpression::commits(new_heads)); + for commit in tx + .base_workspace_helper() + .attach_revset_evaluator(commits_to_push)? + .evaluate_to_commits()? { let commit = commit?; let mut reasons = vec![]; diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 986c0719d1..96d3adeb80 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -160,18 +160,26 @@ pub fn default_symbol_resolver<'a>( DefaultSymbolResolver::new(repo, extensions).with_id_prefix_context(id_prefix_context) } -/// Parses user-configured expression defining the immutable set. -pub fn parse_immutable_expression( +/// Parses user-configured expression defining the heads of the immutable set. +/// Includes the root commit. +pub fn parse_immutable_heads_expression( context: &RevsetParseContext, ) -> Result, RevsetParseError> { let (_, _, immutable_heads_str) = context .aliases_map() .get_function(BUILTIN_IMMUTABLE_HEADS, 0) .unwrap(); + let heads = revset::parse(immutable_heads_str, context)?; + Ok(heads.union(&RevsetExpression::root())) +} + +/// Parses user-configured expression defining the immutable set. +pub fn parse_immutable_expression( + context: &RevsetParseContext, +) -> Result, RevsetParseError> { // Negated ancestors expression `~::( | root())` is slightly easier // to optimize than negated union `~(:: | root())`. - let heads = revset::parse(immutable_heads_str, context)?; - Ok(heads.union(&RevsetExpression::root()).ancestors()) + Ok(parse_immutable_heads_expression(context)?.ancestors()) } pub(super) fn evaluate_revset_to_single_commit<'a>( diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 9e3b01ce25..502e44ca6e 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -835,6 +835,36 @@ fn test_git_push_no_description() { ); } +#[test] +fn test_git_push_no_description_in_immutable() { + let (test_env, workspace_root) = set_up(); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "imm"]); + test_env.jj_cmd_ok(&workspace_root, &["describe", "-m="]); + test_env.jj_cmd_ok(&workspace_root, &["new", "-m", "foo"]); + std::fs::write(workspace_root.join("file"), "contents").unwrap(); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "my-branch"]); + + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch=my-branch", "--dry-run"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 5b36783cd11c since it has no description + "###); + + test_env.add_config(r#"revset-aliases."immutable_heads()" = "imm""#); + let (stdout, stderr) = test_env.jj_cmd_ok( + &workspace_root, + &["git", "push", "--branch=my-branch", "--dry-run"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Branch changes to push to origin: + Add branch my-branch to ea7373507ad9 + Dry-run requested, not pushing. + "###); +} + #[test] fn test_git_push_missing_author() { let (test_env, workspace_root) = set_up(); @@ -863,6 +893,44 @@ fn test_git_push_missing_author() { "###); } +#[test] +fn test_git_push_missing_author_in_immutable() { + let (test_env, workspace_root) = set_up(); + let run_without_var = |var: &str, args: &[&str]| { + test_env + .jj_cmd(&workspace_root, args) + .env_remove(var) + .assert() + .success(); + }; + run_without_var("JJ_USER", &["new", "root()", "-m=no author name"]); + run_without_var("JJ_EMAIL", &["new", "-m=no author email"]); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "imm"]); + test_env.jj_cmd_ok(&workspace_root, &["new", "-m", "foo"]); + std::fs::write(workspace_root.join("file"), "contents").unwrap(); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "my-branch"]); + + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch=my-branch", "--dry-run"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 011f740bf8b5 since it has no author and/or committer set + "###); + + test_env.add_config(r#"revset-aliases."immutable_heads()" = "imm""#); + let (stdout, stderr) = test_env.jj_cmd_ok( + &workspace_root, + &["git", "push", "--branch=my-branch", "--dry-run"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Branch changes to push to origin: + Add branch my-branch to 68fdae89de4f + Dry-run requested, not pushing. + "###); +} + #[test] fn test_git_push_missing_committer() { let (test_env, workspace_root) = set_up(); @@ -899,6 +967,45 @@ fn test_git_push_missing_committer() { "###); } +#[test] +fn test_git_push_missing_committer_in_immutable() { + let (test_env, workspace_root) = set_up(); + let run_without_var = |var: &str, args: &[&str]| { + test_env + .jj_cmd(&workspace_root, args) + .env_remove(var) + .assert() + .success(); + }; + run_without_var("JJ_USER", &["describe", "-m=no committer name"]); + test_env.jj_cmd_ok(&workspace_root, &["new"]); + run_without_var("JJ_EMAIL", &["describe", "-m=no committer email"]); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "imm"]); + test_env.jj_cmd_ok(&workspace_root, &["new", "-m", "foo"]); + std::fs::write(workspace_root.join("file"), "contents").unwrap(); + test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "my-branch"]); + + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch=my-branch", "--dry-run"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 7e61dc727a8f since it has no author and/or committer set + "###); + + test_env.add_config(r#"revset-aliases."immutable_heads()" = "imm""#); + let (stdout, stderr) = test_env.jj_cmd_ok( + &workspace_root, + &["git", "push", "--branch=my-branch", "--dry-run"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Branch changes to push to origin: + Add branch my-branch to c79f85e90b4a + Dry-run requested, not pushing. + "###); +} + #[test] fn test_git_push_deleted() { let (test_env, workspace_root) = set_up();