From 6667b3efecdf8418aa0f1cb8b583a7dc16258f5d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 27 May 2022 08:54:47 -0700 Subject: [PATCH] cli: prevent pushing commits without description, author, or committer This patch prevents perhaps pushing commits with an empty description or the placeholder "(no user/email configured)" values for author/committer. Closes #322. --- CHANGELOG.md | 4 ++ src/commands.rs | 22 ++++++++- tests/test_git_push.rs | 107 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 127 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 464b981b0..17fc008c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `push-`, but this can be overridden by the `push.branch-prefix` config setting. +* `jj git push` now aborts if you attempt to push a commit without a + description or with the placeholder "(no name/email configured)" values for + author/committer. + * Diff editor command arguments can now be specified by config file. Example: diff --git a/src/commands.rs b/src/commands.rs index e5d61e9c9..aea36100d 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -5001,12 +5001,30 @@ fn cmd_git_push( old_heads.extend(old_head.adds()); } } + if old_heads.is_empty() { + old_heads.push(repo.store().root_commit_id().clone()); + } for index_entry in repo.index().walk_revs(&new_heads, &old_heads) { let commit = repo.store().get_commit(&index_entry.commit_id())?; + let mut reasons = vec![]; + if commit.description().is_empty() { + reasons.push("it has no description"); + } + if commit.author().name == UserSettings::user_name_placeholder() + || commit.author().email == UserSettings::user_email_placeholder() + || commit.committer().name == UserSettings::user_name_placeholder() + || commit.committer().email == UserSettings::user_email_placeholder() + { + reasons.push("it has no author and/or committer set"); + } if commit.tree().has_conflict() { + reasons.push("it has conflicts"); + } + if !reasons.is_empty() { return Err(UserError(format!( - "Won't push commit {} since it has conflicts", - short_commit_hash(commit.id()) + "Won't push commit {} since {}", + short_commit_hash(commit.id()), + reasons.join(" and ") ))); } } diff --git a/tests/test_git_push.rs b/tests/test_git_push.rs index e7b9b6abe..a0a92b370 100644 --- a/tests/test_git_push.rs +++ b/tests/test_git_push.rs @@ -12,12 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::path::PathBuf; + use crate::common::TestEnvironment; pub mod common; -#[test] -fn test_git_push() { +fn set_up() -> (TestEnvironment, PathBuf) { let test_env = TestEnvironment::default(); let git_repo_path = test_env.env_root().join("git-repo"); git2::Repository::init(&git_repo_path).unwrap(); @@ -27,13 +28,22 @@ fn test_git_push() { &["git", "clone", "git-repo", "jj-repo"], ); let workspace_root = test_env.env_root().join("jj-repo"); + (test_env, workspace_root) +} +#[test] +fn test_git_push_nothing() { + let (test_env, workspace_root) = set_up(); // No branches to push yet let stdout = test_env.jj_cmd_success(&workspace_root, &["git", "push"]); insta::assert_snapshot!(stdout, @r###" Nothing changed. "###); +} +#[test] +fn test_git_push_open() { + let (test_env, workspace_root) = set_up(); // When pushing everything, won't push an open commit even if there's a branch // on it test_env.jj_cmd_success(&workspace_root, &["branch", "create", "my-branch"]); @@ -55,8 +65,11 @@ fn test_git_push() { insta::assert_snapshot!(stderr, @r###" Error: Won't push open commit "###); +} - // Try pushing a conflict +#[test] +fn test_git_push_conflict() { + let (test_env, workspace_root) = set_up(); std::fs::write(workspace_root.join("file"), "first").unwrap(); test_env.jj_cmd_success(&workspace_root, &["close", "-m", "first"]); std::fs::write(workspace_root.join("file"), "second").unwrap(); @@ -67,6 +80,92 @@ fn test_git_push() { test_env.jj_cmd_success(&workspace_root, &["close", "-m", "third"]); let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]); insta::assert_snapshot!(stderr, @r###" - Error: Won't push commit fcd0490f7df7 since it has conflicts + Error: Won't push commit 50ccff1aeab0 since it has conflicts + "###); +} + +#[test] +fn test_git_push_no_description() { + let (test_env, workspace_root) = set_up(); + test_env.jj_cmd_success(&workspace_root, &["branch", "create", "my-branch"]); + test_env.jj_cmd_success(&workspace_root, &["close", "-m", ""]); + let stderr = + test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch", "my-branch"]); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 4e5f01c842af since it has no description + "###); +} + +#[test] +fn test_git_push_missing_author() { + 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", &["checkout", "root"]); + run_without_var("JJ_USER", &["branch", "create", "missing-name"]); + run_without_var("JJ_USER", &["close", "-m", "initial"]); + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch", "missing-name"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 567e1ab3da0e since it has no author and/or committer set + "###); + run_without_var("JJ_EMAIL", &["checkout", "root"]); + run_without_var("JJ_EMAIL", &["branch", "create", "missing-email"]); + run_without_var("JJ_EMAIL", &["close", "-m", "initial"]); + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch", "missing-email"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit ce7b456bb11a since it has no author and/or committer set + "###); +} + +#[test] +fn test_git_push_missing_committer() { + 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(); + }; + test_env.jj_cmd_success(&workspace_root, &["branch", "create", "missing-name"]); + run_without_var("JJ_USER", &["close", "-m", "no committer name"]); + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch", "missing-name"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit df8d9f6cf625 since it has no author and/or committer set + "###); + test_env.jj_cmd_success(&workspace_root, &["checkout", "root"]); + test_env.jj_cmd_success(&workspace_root, &["branch", "create", "missing-email"]); + run_without_var("JJ_EMAIL", &["close", "-m", "no committer email"]); + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch", "missing-email"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 61b8a14387d7 since it has no author and/or committer set + "###); + + // Test message when there are multiple reasons (missing committer and + // description) + run_without_var("JJ_EMAIL", &["describe", "-m", "", "missing-email"]); + let stderr = test_env.jj_cmd_failure( + &workspace_root, + &["git", "push", "--branch", "missing-email"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Won't push commit 9e1aae45b6a3 since it has no description and it has no author and/or committer set "###); }