From 3f6b0f6feee12aa3ad8ca9b540dfbb54c98ba41b Mon Sep 17 00:00:00 2001 From: Stephen Jennings 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 | 81 +++++++---- cli/tests/runner.rs | 1 + cli/tests/test_private_commits.rs | 227 ++++++++++++++++++++++++++++++ docs/config.md | 18 +++ lib/src/settings.rs | 3 + 6 files changed, 303 insertions(+), 29 deletions(-) create mode 100644 cli/tests/test_private_commits.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b3a549f90d..69b0edf4b6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * New `diff_contains()` revset function can be used to search diffs. +* 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 632a3f4e38a..9dfffa61b80 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::RevsetExpression; +use jj_lib::revset::{self, RevsetExpression}; use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::str_util::StringPattern; use jj_lib::view::View; @@ -279,36 +279,59 @@ pub fn cmd_git_push( &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 + let workspace_helper = tx.base_workspace_helper(); + + let git_settings = command.settings().git_settings(); + let is_private = if let Some(private_commits) = git_settings.private_commits { + let private_commits_revset = revset::parse( + private_commits.as_str(), + &tx.base_workspace_helper().revset_parse_context(), + )?; + Some( + workspace_helper + .attach_revset_evaluator(private_commits_revset)? + .evaluate()? + .containing_fn(), + ) + } else { + None + }; + + for commit in workspace_helper + .attach_revset_evaluator(commits_to_push)? + .evaluate_to_commits()? { - 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 ") - ))); + 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.as_ref().map_or(false, |f| f(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 ") + ))); + } } } diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs index 0a05d943f37..41c614dc0a0 100644 --- a/cli/tests/runner.rs +++ b/cli/tests/runner.rs @@ -51,6 +51,7 @@ mod test_next_prev_commands; mod test_obslog_command; mod test_operations; mod test_parallelize_command; +mod test_private_commits; mod test_rebase_command; mod test_repo_change_report; mod test_resolve_command; diff --git a/cli/tests/test_private_commits.rs b/cli/tests/test_private_commits.rs new file mode 100644 index 00000000000..38c6d23962a --- /dev/null +++ b/cli/tests/test_private_commits.rs @@ -0,0 +1,227 @@ +// Copyright 2023 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"]); + + // Will not push when a pushed commit is contained in git.private-commits + 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 bb89761f8aa..be732a042ba 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:*')" +``` + +Any commit descending from a private commit is also prevented from being pushed, +since doing so would require pushing the private commit as well. + +If a commit is in `git.private-commits` but is already on the remote, then it is +ignored and does not prevent pushing its descendants. Commits that are immutable +are also ignored. + ## Filesystem monitor In large repositories, it may be beneficial to use a "filesystem monitor" to diff --git a/lib/src/settings.rs b/lib/src/settings.rs index 6ee26b92af1..c43901fa538 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -42,6 +42,7 @@ pub struct RepoSettings { pub struct GitSettings { pub auto_local_branch: bool, pub abandon_unreachable_commits: bool, + pub private_commits: Option, } impl GitSettings { @@ -51,6 +52,7 @@ impl GitSettings { abandon_unreachable_commits: config .get_bool("git.abandon-unreachable-commits") .unwrap_or(true), + private_commits: config.get_string("git.private-commits").ok(), } } } @@ -60,6 +62,7 @@ impl Default for GitSettings { GitSettings { auto_local_branch: false, abandon_unreachable_commits: true, + private_commits: None, } } }