cli: Ignore immutable() in jj git push conflict/desc/user checks

Fixes #3029.

CHANGELOG.md

@@ -74,6 +74,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

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![];

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<Rc<RevsetExpression>, 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<Rc<RevsetExpression>, RevsetParseError> {
     // Negated ancestors expression `~::(<heads> | root())` is slightly easier
     // to optimize than negated union `~(::<heads> | 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>(

cli/tests/test_git_push.rs

@@ -844,6 +844,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();
@@ -872,6 +902,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();
@@ -908,6 +976,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();