Skip to content

Commit

Permalink
git: add git.private-commits setting for preventing commits from bein…
Browse files Browse the repository at this point in the history
…g 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
  • Loading branch information
jennings committed Jul 23, 2024
1 parent feceb62 commit 21e17fe
Show file tree
Hide file tree
Showing 5 changed files with 324 additions and 50 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
127 changes: 77 additions & 50 deletions cli/src/commands/git/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions cli/tests/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
226 changes: 226 additions & 0 deletions cli/tests/test_git_private_commits.rs
Original file line number Diff line number Diff line change
@@ -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
"###);
}
Loading

0 comments on commit 21e17fe

Please sign in to comment.