diff --git a/src/commands/submit.rs b/src/commands/submit.rs index 381aadc..1a8927f 100644 --- a/src/commands/submit.rs +++ b/src/commands/submit.rs @@ -15,7 +15,7 @@ use tracing::warn; use unicode_segmentation::UnicodeSegmentation as _; use crate::{ - bookmark::{BookmarkGraph, BookmarkOrPending}, + bookmark::{BookmarkGraph, BookmarkOrPending, JJName as _}, cli::CliConfig, commands::{GetBookmarksOptions, StrVisualWidth as _}, config::{Config, ForgeType}, @@ -221,6 +221,22 @@ pub async fn submit(config: &SubmitCommandConfig, cli_config: &CliConfig<'_>) -> ensure_whatever!(!bookmarks.is_empty(), "No bookmarks in revset {}", revset); + let changes = find_changes_to_submit( + &jj, + bookmarks.iter().map(BookmarkOrPending::change_id), + &pending_bookmarks, + )?; + + // Backstop (RIG-2267): pass 1 resolved a non-empty bookmark set from the + // revset, but pass 2 (find_changes_to_submit) resolved it to nothing to + // submit. Never announce bookmarks and then exit 0 with "No bookmarks + // pushed" — fail loudly with an actionable message. + ensure_whatever!( + !changes.is_empty(), + "Resolved bookmark(s) {} but found no changes to submit — the named bookmark(s) may already be merged into trunk (inspect with `jj log -r `). For a stacked/ancestry-walked submit, also confirm `jj config get user.email` matches the change authors.", + bookmarks.iter().map(|b| b.raw_name()).join(", ") + ); + let forge = ForgeImpl::new(&repo_config)?; output.log_message(&format!( @@ -235,12 +251,6 @@ pub async fn submit(config: &SubmitCommandConfig, cli_config: &CliConfig<'_>) -> bookmarks.iter().map(|b| b.magenta().to_string()).join(", ") )); - let changes = find_changes_to_submit( - &jj, - bookmarks.iter().map(BookmarkOrPending::change_id), - &pending_bookmarks, - )?; - let bookmark_graph = BookmarkGraph::from_changes(&jj, &changes, config.revset_options.tracked)?; let submission_plan = plan::plan(PlanContext { diff --git a/src/submit.rs b/src/submit.rs index 7018183..67e3c28 100644 --- a/src/submit.rs +++ b/src/submit.rs @@ -20,25 +20,39 @@ pub mod plan; pub mod stack_link; /// Find the changes that matter for a submission starting from `targets`: -/// bookmarked changes authored by the current user that are reachable from -/// the targets and are not already in the trunk ancestry. +/// bookmarked changes reachable from the targets that are not already in the +/// trunk ancestry. An explicitly-named target is included regardless of its +/// author; only the ancestry-walked companions are narrowed to `mine()` (so a +/// stacked submit does not sweep in other people's bookmarks). This split is +/// the RIG-2267 fix: filtering the explicit target by `mine()` too made +/// `submit ` silently no-op (exit 0, "No bookmarks pushed") whenever +/// the bookmark's commit author differed from the configured `user.email`. pub fn find_changes_to_submit( jj: &Jujutsu, targets: impl IntoIterator, change_ids_pending_bookmarks: &HashSet, ) -> Result> { + let target_atoms: Vec = targets.into_iter().map(|t| t.name_for_jj()).collect(); + + let explicit = if target_atoms.is_empty() { + "none()".to_owned() + } else { + target_atoms.iter().join(" | ") + }; + let ancestry = if target_atoms.is_empty() { + "none()".to_owned() + } else { + target_atoms.iter().map(|t| format!("::{t}")).join(" | ") + }; + let pending = if change_ids_pending_bookmarks.is_empty() { + "none()".to_owned() + } else { + change_ids_pending_bookmarks.iter().join(" | ") + }; + jj.log_with_pending_bookmarks( format!( - "((({}) & mine() & bookmarks()) | ({})) ~ (::trunk())", - targets - .into_iter() - .map(|t| format!("::{}", t.name_for_jj())) - .join(" | "), - if change_ids_pending_bookmarks.is_empty() { - "none()".to_owned() - } else { - change_ids_pending_bookmarks.iter().join(" | ") - } + "(({explicit}) | (({ancestry}) & mine() & bookmarks()) | ({pending})) ~ (::trunk())" ), change_ids_pending_bookmarks, ) diff --git a/src/tests/edge_cases.rs b/src/tests/edge_cases.rs index 3ae1da4..28a97c4 100644 --- a/src/tests/edge_cases.rs +++ b/src/tests/edge_cases.rs @@ -167,6 +167,90 @@ fn find_changes_to_submit_with_advanced_main() -> Result<()> { Ok(()) } +#[test] +fn find_changes_to_submit_includes_foreign_authored_named_target() -> Result<()> { + let repo = TestRepo::with_local_remote(); + + repo.jj.exec(["new", "main"])?; + repo.create_change("f1.txt", "f1", "Feature 1") + .create_bookmark("feature"); + + // Author the bookmarked commit under a *different* identity than the one + // configured now — the RIG-2267 trigger (the fleet commit-author flip left + // pre-flip commits authored under the old identity). `mine()` would drop it. + repo.set_config("user.email", "seal@sealedsecurity.com"); + repo.set_config("user.name", "seal"); + repo.jj.exec(["metaedit", "--update-author"])?; + repo.set_config("user.email", "mintaka@rigel.build"); + repo.set_config("user.name", "mintaka"); + + // An explicitly-named target must be submitted regardless of its author. + let changes = find_changes_to_submit(&repo.jj, ["feature"], &HashSet::new())?; + let names: Vec<_> = Bookmark::from_changes(&changes) + .into_iter() + .map(|b| b.name().to_owned()) + .collect(); + assert_eq!(names, vec!["feature".to_owned()]); + + Ok(()) +} + +#[test] +fn find_changes_to_submit_excludes_foreign_authored_ancestry_companion() -> Result<()> { + let repo = TestRepo::with_local_remote(); + + // Build a stack off trunk: a (mine) -> c (foreign) -> b (mine), where the + // middle bookmark `c` is authored under a *different* identity (the + // RIG-2267 commit-author flip). Only the explicitly-named target is taken + // raw; ancestry-walked companions stay narrowed to `mine()`, so a foreign + // companion sitting in the ancestry of the target must be EXCLUDED — the + // other half of the fix (a stacked submit must not sweep in other people's + // bookmarks). Without `& mine()` on the ancestry branch, `c` would leak in. + repo.set_config("user.email", "mintaka@rigel.build"); + repo.set_config("user.name", "mintaka"); + + repo.jj.exec(["new", "main"])?; + repo.create_change("a.txt", "a", "Change A") + .create_bookmark("a"); + + repo.jj.exec(["new"])?; + repo.create_change("c.txt", "c", "Change C") + .create_bookmark("c"); + // Re-author `c` (=@) under the old identity, then restore `user.email` so + // `mine()` resolves to `mintaka` at query time. + repo.set_config("user.email", "seal@sealedsecurity.com"); + repo.set_config("user.name", "seal"); + repo.jj.exec(["metaedit", "--update-author"])?; + repo.set_config("user.email", "mintaka@rigel.build"); + repo.set_config("user.name", "mintaka"); + + repo.jj.exec(["new"])?; + repo.create_change("b.txt", "b", "Change B") + .create_bookmark("b"); + + // Submitting `b` walks its ancestry: `a` (mine) is included, `c` (foreign) + // is dropped by `& mine()`. + let changes = find_changes_to_submit(&repo.jj, ["b"], &HashSet::new())?; + let mut names: Vec<_> = Bookmark::from_changes(&changes) + .into_iter() + .map(|b| b.name().to_owned()) + .collect(); + names.sort(); + assert_eq!(names, vec!["a".to_owned(), "b".to_owned()]); + + // Two explicit targets exercise the multi-target `join(" | ")` on both the + // explicit and ancestry branches; `c` stays excluded from the ancestry. + let changes = find_changes_to_submit(&repo.jj, ["a", "b"], &HashSet::new())?; + let mut names: Vec<_> = Bookmark::from_changes(&changes) + .into_iter() + .map(|b| b.name().to_owned()) + .collect(); + names.sort(); + assert_eq!(names, vec!["a".to_owned(), "b".to_owned()]); + + Ok(()) +} + #[cfg(not(feature = "no-e2e-tests"))] mod e2e { use assertables::assert_contains; diff --git a/src/tests/submit/validation.rs b/src/tests/submit/validation.rs index 9fcda55..71ac11d 100644 --- a/src/tests/submit/validation.rs +++ b/src/tests/submit/validation.rs @@ -1,3 +1,7 @@ +use assertables::assert_contains; + +use crate::{error::Result, tests::TestRepo}; + #[cfg(not(feature = "no-e2e-tests"))] mod e2e { use assertables::assert_contains; @@ -44,3 +48,34 @@ mod e2e { Ok(()) } } + +#[tokio::test] +async fn named_bookmark_in_trunk_errors_instead_of_silent_noop() -> Result<()> { + let repo = TestRepo::with_local_remote(); + + // A bookmark pointing at a commit already in trunk resolves in pass 1 + // (verbatim, no mine() filter) but has nothing to submit in pass 2. It must + // error, not exit 0 with "No bookmarks pushed" (RIG-2267 backstop). The + // guard runs before any forge network call, so a config that merely passes + // validation plus --dry-run keeps this off the network and lets it gate CI + // (which runs with --features no-e2e-tests). Seed the full minimum GitHub + // config (forge + project + token) at the repo layer — `with_local_remote` + // sets no jj-vine config, and relying on an ambient user-level `forge`/token + // would make this test pass only on a developer box (the exact config-leak + // non-hermeticity documented in config.rs), while CI's clean HOME fails the + // parse with "missing field `forge`" before ever reaching the guard. + repo.set_config("jj-vine.forge", "github"); + repo.set_config("jj-vine.github.project", "owner/repo"); + repo.set_config("jj-vine.github.token", "gh-test-token"); + repo.jj + .exec(["bookmark", "create", "on-trunk", "-r", "main"])?; + + let result = repo.try_run(["submit", "on-trunk", "--dry-run"]).await; + + assert_contains!( + result.unwrap_err().to_string(), + "found no changes to submit" + ); + + Ok(()) +}