From 03b6d380f5b0dc1fbaf752f0a99d15b418af8ef8 Mon Sep 17 00:00:00 2001
From: Stephen Jennings <stephen.g.jennings@gmail.com>
Date: Fri, 5 Jul 2024 14:47:46 -0700
Subject: [PATCH] git: add git.private-commits setting for preventing commits
 from being pushed

The user can define the setting `git.private-commits` as they desire. For
example:

    git.private-commits = 'description(glob:"wip:*")'

If any commits are in this revset, then the push is aborted.

If a commit would be private but already exists on the remote, then it does
not block pushes, nor do its descendents block pushes unless they are also
contained in `git.private-commits`.

Closes #3376
---
 CHANGELOG.md                          |   2 +
 cli/src/commands/git/push.rs          | 127 +++++++++------
 cli/tests/runner.rs                   |   1 +
 cli/tests/test_git_private_commits.rs | 226 ++++++++++++++++++++++++++
 docs/config.md                        |  18 ++
 5 files changed, 324 insertions(+), 50 deletions(-)
 create mode 100644 cli/tests/test_git_private_commits.rs

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d486b746ef..e44457e358 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -73,6 +73,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * New command `jj operation show` that can show the changes made in a single
   operation.
 
+* New config setting `git.private-commits` to prevent commits from being pushed.
+
 ### Fixed bugs
 
 * `jj diff --git` no longer shows the contents of binary files.
diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs
index 632a3f4e38..ba3e11dd5e 100644
--- a/cli/src/commands/git/push.rs
+++ b/cli/src/commands/git/push.rs
@@ -18,6 +18,7 @@ use std::{fmt, io};
 
 use clap::ArgGroup;
 use itertools::Itertools;
+use jj_lib::backend::CommitId;
 use jj_lib::git::{self, GitBranchPushTargets, GitPushError};
 use jj_lib::object_id::ObjectId;
 use jj_lib::op_store::RefTarget;
@@ -261,56 +262,7 @@ pub fn cmd_git_push(
         );
     }
 
-    // Check if there are conflicts in any commits we're about to push that haven't
-    // already been pushed.
-    let new_heads = branch_updates
-        .iter()
-        .filter_map(|(_, update)| update.new_target.clone())
-        .collect_vec();
-    let old_heads = repo
-        .view()
-        .remote_branches(&remote)
-        .flat_map(|(_, old_head)| old_head.target.added_ids())
-        .cloned()
-        .collect_vec();
-    // (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![];
-        if commit.description().is_empty() && !args.allow_empty_description {
-            reasons.push("it has no description");
-        }
-        if commit.author().name.is_empty()
-            || commit.author().name == UserSettings::USER_NAME_PLACEHOLDER
-            || commit.author().email.is_empty()
-            || commit.author().email == UserSettings::USER_EMAIL_PLACEHOLDER
-            || commit.committer().name.is_empty()
-            || commit.committer().name == UserSettings::USER_NAME_PLACEHOLDER
-            || commit.committer().email.is_empty()
-            || commit.committer().email == UserSettings::USER_EMAIL_PLACEHOLDER
-        {
-            reasons.push("it has no author and/or committer set");
-        }
-        if commit.has_conflict()? {
-            reasons.push("it has conflicts");
-        }
-        if !reasons.is_empty() {
-            return Err(user_error(format!(
-                "Won't push commit {} since {}",
-                short_commit_hash(commit.id()),
-                reasons.join(" and ")
-            )));
-        }
-    }
+    validate_commits_ready_to_push(&branch_updates, &remote, &tx, command, args)?;
 
     writeln!(ui.status(), "Branch changes to push to {}:", &remote)?;
     for (branch_name, update) in &branch_updates {
@@ -387,6 +339,81 @@ pub fn cmd_git_push(
     Ok(())
 }
 
+/// Validates that the commits that will be pushed are ready (have authorship
+/// information, are not conflicted, etc.)
+fn validate_commits_ready_to_push(
+    branch_updates: &[(String, BranchPushUpdate)],
+    remote: &str,
+    tx: &WorkspaceCommandTransaction,
+    command: &CommandHelper,
+    args: &GitPushArgs,
+) -> Result<(), CommandError> {
+    let workspace_helper = tx.base_workspace_helper();
+    let repo = workspace_helper.repo();
+
+    let new_heads = branch_updates
+        .iter()
+        .filter_map(|(_, update)| update.new_target.clone())
+        .collect_vec();
+    let old_heads = repo
+        .view()
+        .remote_branches(remote)
+        .flat_map(|(_, old_head)| old_head.target.added_ids())
+        .cloned()
+        .collect_vec();
+    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));
+
+    let config = command.settings().config();
+    let is_private = if let Ok(revset) = config.get_string("git.private-commits") {
+        workspace_helper
+            .parse_revset(&RevisionArg::from(revset))?
+            .evaluate()?
+            .containing_fn()
+    } else {
+        Box::new(|_: &CommitId| false)
+    };
+
+    for commit in workspace_helper
+        .attach_revset_evaluator(commits_to_push)?
+        .evaluate_to_commits()?
+    {
+        let commit = commit?;
+        let mut reasons = vec![];
+        if commit.description().is_empty() && !args.allow_empty_description {
+            reasons.push("it has no description");
+        }
+        if commit.author().name.is_empty()
+            || commit.author().name == UserSettings::USER_NAME_PLACEHOLDER
+            || commit.author().email.is_empty()
+            || commit.author().email == UserSettings::USER_EMAIL_PLACEHOLDER
+            || commit.committer().name.is_empty()
+            || commit.committer().name == UserSettings::USER_NAME_PLACEHOLDER
+            || commit.committer().email.is_empty()
+            || commit.committer().email == UserSettings::USER_EMAIL_PLACEHOLDER
+        {
+            reasons.push("it has no author and/or committer set");
+        }
+        if commit.has_conflict()? {
+            reasons.push("it has conflicts");
+        }
+        if is_private(commit.id()) {
+            reasons.push("it is private");
+        }
+        if !reasons.is_empty() {
+            return Err(user_error(format!(
+                "Won't push commit {} since {}",
+                short_commit_hash(commit.id()),
+                reasons.join(" and ")
+            )));
+        }
+    }
+    Ok(())
+}
+
 fn get_default_push_remote(
     ui: &Ui,
     settings: &UserSettings,
diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs
index 0a05d943f3..116667adae 100644
--- a/cli/tests/runner.rs
+++ b/cli/tests/runner.rs
@@ -36,6 +36,7 @@ mod test_git_colocated;
 mod test_git_fetch;
 mod test_git_import_export;
 mod test_git_init;
+mod test_git_private_commits;
 mod test_git_push;
 mod test_git_remotes;
 mod test_git_submodule;
diff --git a/cli/tests/test_git_private_commits.rs b/cli/tests/test_git_private_commits.rs
new file mode 100644
index 0000000000..a1473ffb9f
--- /dev/null
+++ b/cli/tests/test_git_private_commits.rs
@@ -0,0 +1,226 @@
+// 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::path::{Path, PathBuf};
+
+use crate::common::TestEnvironment;
+
+fn set_up() -> (TestEnvironment, PathBuf) {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "origin"]);
+    let origin_path = test_env.env_root().join("origin");
+    let origin_git_repo_path = origin_path
+        .join(".jj")
+        .join("repo")
+        .join("store")
+        .join("git");
+
+    test_env.jj_cmd_ok(&origin_path, &["describe", "-m=public 1"]);
+    test_env.jj_cmd_ok(&origin_path, &["new", "-m=public 2"]);
+    test_env.jj_cmd_ok(&origin_path, &["branch", "create", "main"]);
+    test_env.jj_cmd_ok(&origin_path, &["git", "export"]);
+
+    test_env.jj_cmd_ok(
+        test_env.env_root(),
+        &[
+            "git",
+            "clone",
+            "--config-toml=git.auto-local-branch=true",
+            origin_git_repo_path.to_str().unwrap(),
+            "local",
+        ],
+    );
+    let workspace_root = test_env.env_root().join("local");
+
+    (test_env, workspace_root)
+}
+
+fn set_up_remote_at_main(test_env: &TestEnvironment, workspace_root: &Path, remote_name: &str) {
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", remote_name]);
+    let other_path = test_env.env_root().join(remote_name);
+    let other_git_repo_path = other_path
+        .join(".jj")
+        .join("repo")
+        .join("store")
+        .join("git");
+    test_env.jj_cmd_ok(
+        workspace_root,
+        &[
+            "git",
+            "remote",
+            "add",
+            remote_name,
+            other_git_repo_path.to_str().unwrap(),
+        ],
+    );
+    test_env.jj_cmd_ok(
+        workspace_root,
+        &["git", "push", "--remote", remote_name, "-b=main"],
+    );
+}
+
+#[test]
+fn test_git_private_commits_block_pushing() {
+    let (test_env, workspace_root) = set_up();
+
+    test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=private 1"]);
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "main"]);
+
+    // Will not push when a pushed commit is contained in git.private-commits
+    test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--all"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Won't push commit aa3058ff8663 since it is private
+    "###);
+
+    // May push when the commit is removed from git.private-commits
+    test_env.add_config(r#"git.private-commits = "none()""#);
+    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--all"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Move forward branch main from 7eb97bf230ad to aa3058ff8663
+    Warning: The working-copy commit in workspace 'default' became immutable, so a new commit has been created on top of it.
+    Working copy now at: znkkpsqq 2e1adf47 (empty) (no description set)
+    Parent commit      : yqosqzyt aa3058ff main | (empty) private 1
+    "###);
+}
+
+#[test]
+fn test_git_private_commits_are_not_checked_if_immutable() {
+    let (test_env, workspace_root) = set_up();
+
+    test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=private 1"]);
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "main"]);
+
+    test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
+    test_env.add_config(r#"revset-aliases."immutable_heads()" = "all()""#);
+    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--all"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Move forward branch main from 7eb97bf230ad to aa3058ff8663
+    Warning: The working-copy commit in workspace 'default' became immutable, so a new commit has been created on top of it.
+    Working copy now at: yostqsxw dce4a15c (empty) (no description set)
+    Parent commit      : yqosqzyt aa3058ff main | (empty) private 1
+    "###);
+}
+
+#[test]
+fn test_git_private_commits_not_directly_in_line_block_pushing() {
+    let (test_env, workspace_root) = set_up();
+
+    // New private commit descended from root()
+    test_env.jj_cmd_ok(&workspace_root, &["new", "root()", "-m=private 1"]);
+
+    test_env.jj_cmd_ok(&workspace_root, &["new", "main", "@", "-m=public 3"]);
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "branch1"]);
+
+    test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "-b=branch1"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Won't push commit f1253a9b1ea9 since it is private
+    "###);
+}
+
+#[test]
+fn test_git_private_commits_descending_from_commits_pushed_do_not_block_pushing() {
+    let (test_env, workspace_root) = set_up();
+
+    test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=public 3"]);
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "move", "main"]);
+    test_env.jj_cmd_ok(&workspace_root, &["new", "-m=private 1"]);
+
+    test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
+    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=main"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Move forward branch main from 7eb97bf230ad to 05ef53bc99ec
+    "###);
+}
+
+#[test]
+fn test_git_private_commits_already_on_the_remote_do_not_block_push() {
+    let (test_env, workspace_root) = set_up();
+
+    // Start a branch before a "private" commit lands in main
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "branch1", "-r=main"]);
+
+    // Push a commit that would become a private_root if it weren't already on
+    // the remote
+    test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=private 1"]);
+    test_env.jj_cmd_ok(&workspace_root, &["new", "-m=public 3"]);
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "main"]);
+    let (_, stderr) =
+        test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=main", "-b=branch1"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Move forward branch main from 7eb97bf230ad to fbb352762352
+      Add branch branch1 to 7eb97bf230ad
+    Warning: The working-copy commit in workspace 'default' became immutable, so a new commit has been created on top of it.
+    Working copy now at: kpqxywon a7b08364 (empty) (no description set)
+    Parent commit      : yostqsxw fbb35276 main | (empty) public 3
+    "###);
+
+    test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
+
+    // Since "private 1" is already on the remote, pushing it should be allowed
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "branch1", "-r=main"]);
+    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--all"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Move forward branch branch1 from 7eb97bf230ad to fbb352762352
+    "###);
+
+    // Ensure that the already-pushed commit doesn't block a new branch from
+    // being pushed
+    test_env.jj_cmd_ok(
+        &workspace_root,
+        &["new", "description('private 1')", "-m=public 4"],
+    );
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "branch2"]);
+    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=branch2"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Add branch branch2 to ee5b808b0b95
+    "###);
+}
+
+#[test]
+fn test_git_private_commits_are_evaluated_separately_for_each_remote() {
+    let (test_env, workspace_root) = set_up();
+    set_up_remote_at_main(&test_env, &workspace_root, "other");
+    test_env.add_config(r#"revset-aliases."immutable_heads()" = "none()""#);
+
+    // Push a commit that would become a private_root if it weren't already on
+    // the remote
+    test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=private 1"]);
+    test_env.jj_cmd_ok(&workspace_root, &["new", "-m=public 3"]);
+    test_env.jj_cmd_ok(&workspace_root, &["branch", "set", "main"]);
+    let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=main"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Branch changes to push to origin:
+      Move forward branch main from 7eb97bf230ad to d8632ce893ab
+    "###);
+
+    test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
+
+    // But pushing to a repo that doesn't have the private commit yet is still
+    // blocked
+    let stderr = test_env.jj_cmd_failure(
+        &workspace_root,
+        &["git", "push", "--remote=other", "-b=main"],
+    );
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Won't push commit 36b7ecd11ad9 since it is private
+    "###);
+}
diff --git a/docs/config.md b/docs/config.md
index bb89761f8a..8bf105da75 100644
--- a/docs/config.md
+++ b/docs/config.md
@@ -809,6 +809,24 @@ example:
 
     git.push-branch-prefix = "martinvonz/push-"
 
+### Set of private commits
+
+You can configure the set of private commits by setting `git.private-commits` to
+a revset. The value is a revset of commits that Jujutsu will refuse to push. If
+unset, all commits are eligible to be pushed.
+
+```toml
+# Prevent pushing work in progress or anything explicitly labeled "private"
+git.private-commits = "description(glob:'wip:*') | description(glob:'private:*')"
+```
+
+If a commit is in `git.private-commits` but is already on the remote, then it is
+not considered a private commit. Commits that are immutable are also excluded
+from the private set.
+
+Private commits prevent their descendants from being pushed, since doing so
+would require pushing the private commit as well.
+
 ## Filesystem monitor
 
 In large repositories, it may be beneficial to use a "filesystem monitor" to