diff --git a/src/commands.rs b/src/commands.rs index 7d7a219..a956124 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -174,6 +174,21 @@ pub(crate) fn run_new(preflight: &env::PreflightContext, new_branch: &str) -> Ex eprintln!("error: {message}"); return ExitCode::from(1); } + } else { + let needs_push = match gitops::branch_needs_push(current_branch) { + Ok(needs_push) => needs_push, + Err(message) => { + eprintln!("error: {message}"); + return ExitCode::from(1); + } + }; + if needs_push { + println!("$ git push origin {}", current_branch); + if let Err(message) = gitops::push_branch(current_branch) { + eprintln!("error: {message}"); + return ExitCode::from(1); + } + } } let current_has_pr = match github::pr_exists_for_head(current_branch) { @@ -309,6 +324,21 @@ pub(crate) fn run_submit( eprintln!("error: {message}"); return ExitCode::from(1); } + } else { + let needs_push = match gitops::branch_needs_push(current_branch) { + Ok(needs_push) => needs_push, + Err(message) => { + eprintln!("error: {message}"); + return ExitCode::from(1); + } + }; + if needs_push { + println!("$ git push origin {}", current_branch); + if let Err(message) = gitops::push_branch(current_branch) { + eprintln!("error: {message}"); + return ExitCode::from(1); + } + } } let current_has_pr = match github::pr_exists_for_head(current_branch) { @@ -532,6 +562,11 @@ pub(crate) fn run_sync( } } + // Track branches rebased in this sync run so subsequent steps that depend + // on them use the just-updated local ref instead of the stale remote ref. + let mut rebased_in_this_sync: std::collections::HashSet = + std::collections::HashSet::new(); + for index in state.completed_steps..state.steps.len() { let step = &state.steps[index]; let branch_ref = format!("refs/heads/{}", step.branch); @@ -550,21 +585,44 @@ pub(crate) fn run_sync( return ExitCode::from(1); } }; + let onto_ref = if rebased_in_this_sync.contains(&step.new_base_ref) { + // Parent was rebased in a prior step of this sync; the local ref + // is up-to-date but the remote ref is stale (not yet pushed). + format!("refs/heads/{}", step.new_base_ref) + } else { + match gitops::resolve_onto_ref(&step.new_base_ref) { + Ok(r) => r, + Err(message) => { + eprintln!("error: {message}"); + return ExitCode::from(1); + } + } + }; let total_steps = state.steps.len(); - println!( - "Step {}/{}: rebasing {} onto {} (from {})", - index + 1, - total_steps, - step.branch, - step.new_base_ref, - step.old_base_ref - ); + if step.old_base_ref == step.new_base_ref { + println!( + "Step {}/{}: rebasing {} onto {} (dropping already-upstream commits)", + index + 1, + total_steps, + step.branch, + step.new_base_ref + ); + } else { + println!( + "Step {}/{}: rebasing {} onto {} (from {})", + index + 1, + total_steps, + step.branch, + step.new_base_ref, + step.old_base_ref + ); + } println!( "$ git rebase --onto {} {} {}", - step.new_base_ref, old_base_sha, step.branch + onto_ref, old_base_sha, step.branch ); - if let Err(message) = gitops::rebase_onto(&step.new_base_ref, &old_base_sha, &step.branch) { + if let Err(message) = gitops::rebase_onto(&onto_ref, &old_base_sha, &step.branch) { state.failed_step = Some(index); state.failed_step_branch_head = Some(branch_head); if let Err(save_error) = sync_state::save_sync(&state) { @@ -582,6 +640,7 @@ pub(crate) fn run_sync( return ExitCode::from(1); } + rebased_in_this_sync.insert(step.branch.clone()); state.completed_steps = index + 1; state.failed_step = None; state.failed_step_branch_head = None; diff --git a/src/gitops.rs b/src/gitops.rs index f5ca683..c4af796 100644 --- a/src/gitops.rs +++ b/src/gitops.rs @@ -46,6 +46,12 @@ pub fn resolve_ref(reference: &str) -> Result { rev_parse(reference) } +/// Resolve the `--onto` target ref for a rebase, preferring `origin/` +/// over the local ref so the rebase target reflects the fetched remote state. +pub fn resolve_onto_ref(base_branch: &str) -> Result { + resolve_base_ref(base_branch) +} + /// Resolve the fork point to use as the old base for `git rebase --onto`. /// /// This prefers the merge-base between `branch` and `base_branch` so sync can @@ -66,16 +72,20 @@ pub fn resolve_old_base_for_rebase(base_branch: &str, branch: &str) -> Result Result { - let local_ref = format!("refs/heads/{base_branch}"); - if ref_exists(&local_ref)? { - return Ok(local_ref); - } - + // Prefer the remote ref because `stck sync` fetches before planning. + // Using the local ref for shared branches like the default branch can + // produce a stale merge-base when the remote has advanced (e.g. after a + // PR merge on GitHub while local main has not been pulled). let remote_ref = format!("refs/remotes/origin/{base_branch}"); if ref_exists(&remote_ref)? { return Ok(remote_ref); } + let local_ref = format!("refs/heads/{base_branch}"); + if ref_exists(&local_ref)? { + return Ok(local_ref); + } + Err(format!( "could not resolve old base branch `{base_branch}` locally or on origin; fetch and/or restore the branch, then rerun `stck sync`" )) @@ -192,6 +202,27 @@ pub fn rebase_onto(new_base: &str, old_base: &str, branch: &str) -> Result<(), S } } +/// Push `branch` to `origin` as a regular (fast-forward) push. +/// +/// Unlike [`push_force_with_lease`] this does **not** rewrite remote history. +/// A non-fast-forward push will fail, which is the desired safety behaviour +/// when the caller simply wants to publish new local commits. +pub fn push_branch(branch: &str) -> Result<(), String> { + let status = Command::new("git") + .args(["push", "origin", branch]) + .stderr(Stdio::inherit()) + .status() + .map_err(|_| "failed to run `git push`; ensure this is a git repository".to_string())?; + + if status.success() { + Ok(()) + } else { + Err(format!( + "push failed for branch {branch}; fix the push error and retry" + )) + } +} + /// Push `branch` to `origin` with `--force-with-lease`. pub fn push_force_with_lease(branch: &str) -> Result<(), String> { let status = Command::new("git") diff --git a/tests/harness/mod.rs b/tests/harness/mod.rs index d7d5467..e5f90da 100644 --- a/tests/harness/mod.rs +++ b/tests/harness/mod.rs @@ -87,6 +87,13 @@ if [[ "${1:-}" == "rev-parse" && "${2:-}" == "--verify" ]]; then fi ;; feature-child) echo "3333333333333333333333333333333333333333" ;; + main) + if [[ -n "${STCK_TEST_LOCAL_MAIN_SHA:-}" ]]; then + echo "${STCK_TEST_LOCAL_MAIN_SHA}" + else + echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + fi + ;; *) echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ;; esac exit 0 @@ -115,6 +122,13 @@ if [[ "${1:-}" == "rev-parse" && "${2:-}" == "--verify" ]]; then ;; feature-branch) echo "2222222222222222222222222222222222222222" ;; feature-child) echo "3333333333333333333333333333333333333333" ;; + main) + if [[ -n "${STCK_TEST_REMOTE_MAIN_SHA:-}" ]]; then + echo "${STCK_TEST_REMOTE_MAIN_SHA}" + else + echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + fi + ;; *) echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ;; esac exit 0 @@ -209,8 +223,19 @@ if [[ "${1:-}" == "merge-base" && "${2:-}" != "--is-ancestor" ]]; then refs/remotes/origin/feature-branch) echo "2222222222222222222222222222222222222222" ;; - refs/heads/main|refs/remotes/origin/main) - echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + refs/heads/main) + if [[ -n "${STCK_TEST_LOCAL_MAIN_SHA:-}" ]]; then + echo "${STCK_TEST_LOCAL_MAIN_SHA}" + else + echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + fi + ;; + refs/remotes/origin/main) + if [[ -n "${STCK_TEST_REMOTE_MAIN_SHA:-}" ]]; then + echo "${STCK_TEST_REMOTE_MAIN_SHA}" + else + echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + fi ;; *) echo "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ;; esac @@ -289,6 +314,17 @@ if [[ "${1:-}" == "push" && "${2:-}" == "--force-with-lease" && "${3:-}" == "ori exit 0 fi +if [[ "${1:-}" == "push" && "${2:-}" == "origin" ]]; then + branch="${3:-}" + if [[ -n "${STCK_TEST_LOG:-}" ]]; then + echo "$*" >> "${STCK_TEST_LOG}" + fi + if [[ "${STCK_TEST_PUSH_FAIL_BRANCH:-}" == "${branch}" ]]; then + exit 1 + fi + exit 0 +fi + if [[ "${1:-}" == "push" && "${2:-}" == "-u" && "${3:-}" == "origin" ]]; then branch="${4:-}" if [[ -n "${STCK_TEST_LOG:-}" ]]; then diff --git a/tests/new.rs b/tests/new.rs index 752022a..4f961fb 100644 --- a/tests/new.rs +++ b/tests/new.rs @@ -200,6 +200,54 @@ fn new_rejects_invalid_branch_name() { )); } +#[test] +fn new_auto_pushes_when_upstream_exists_but_branch_needs_push() { + let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); + let log_path = log_path(&temp, "stck-new-auto-push.log"); + cmd.env("STCK_TEST_LOG", log_path.as_os_str()); + cmd.env("STCK_TEST_HAS_UPSTREAM", "1"); + cmd.env("STCK_TEST_NEEDS_PUSH_BRANCH", "feature-branch"); + cmd.env("STCK_TEST_NEW_BRANCH_HAS_COMMITS", "1"); + cmd.args(["new", "feature-next"]); + + cmd.assert() + .success() + .stdout(predicate::str::contains("$ git push origin feature-branch")); + + let log = fs::read_to_string(&log_path).expect("new log should exist"); + assert!( + log.contains("push origin feature-branch"), + "new should auto-push current branch when it has upstream but needs push" + ); + assert!( + !log.contains("push -u origin feature-branch"), + "new should use regular push, not push -u, when upstream already exists" + ); +} + +#[test] +fn new_skips_push_when_upstream_exists_and_branch_is_up_to_date() { + let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); + let log_path = log_path(&temp, "stck-new-no-push.log"); + cmd.env("STCK_TEST_LOG", log_path.as_os_str()); + cmd.env("STCK_TEST_HAS_UPSTREAM", "1"); + cmd.env("STCK_TEST_HAS_CURRENT_PR", "1"); + cmd.env("STCK_TEST_NEW_BRANCH_HAS_COMMITS", "1"); + cmd.args(["new", "feature-next"]); + + cmd.assert().success(); + + let log = fs::read_to_string(&log_path).expect("new log should exist"); + assert!( + !log.contains("push origin feature-branch"), + "new should not push when branch is already up to date with remote" + ); + assert!( + !log.contains("push -u origin feature-branch"), + "new should not push -u when upstream already exists" + ); +} + #[test] fn new_fails_when_pr_presence_check_errors() { let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); diff --git a/tests/submit.rs b/tests/submit.rs index 1d0dc2d..186d3cb 100644 --- a/tests/submit.rs +++ b/tests/submit.rs @@ -122,6 +122,30 @@ fn submit_noops_when_pr_exists() { )); } +#[test] +fn submit_auto_pushes_when_upstream_exists_but_branch_needs_push() { + let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); + let log_path = log_path(&temp, "stck-submit-auto-push.log"); + cmd.env("STCK_TEST_LOG", log_path.as_os_str()); + cmd.env("STCK_TEST_HAS_UPSTREAM", "1"); + cmd.env("STCK_TEST_NEEDS_PUSH_BRANCH", "feature-branch"); + cmd.args(["submit", "--base", "main"]); + + cmd.assert() + .success() + .stdout(predicate::str::contains("$ git push origin feature-branch")); + + let log = fs::read_to_string(&log_path).expect("submit log should exist"); + assert!( + log.contains("push origin feature-branch"), + "submit should auto-push current branch when it has upstream but needs push" + ); + assert!( + !log.contains("push -u origin feature-branch"), + "submit should use regular push, not push -u, when upstream already exists" + ); +} + #[test] fn submit_rejects_default_branch() { let (_temp, mut cmd) = stck_cmd_with_stubbed_tools(); diff --git a/tests/sync.rs b/tests/sync.rs index f0bc9d9..c210fc3 100644 --- a/tests/sync.rs +++ b/tests/sync.rs @@ -25,10 +25,10 @@ fn sync_executes_rebase_plan_and_prints_success_message() { cmd.assert() .success() .stdout(predicate::str::contains( - "$ git rebase --onto main 1111111111111111111111111111111111111111 feature-branch", + "$ git rebase --onto refs/remotes/origin/main 1111111111111111111111111111111111111111 feature-branch", )) .stdout(predicate::str::contains( - "$ git rebase --onto feature-branch 2222222222222222222222222222222222222222 feature-child", + "$ git rebase --onto refs/heads/feature-branch 2222222222222222222222222222222222222222 feature-child", )) .stdout(predicate::str::contains("$ git checkout feature-branch")) .stdout(predicate::str::contains( @@ -37,10 +37,10 @@ fn sync_executes_rebase_plan_and_prints_success_message() { let log = fs::read_to_string(&log_path).expect("rebase log should exist"); assert!( - log.contains("rebase --onto main 1111111111111111111111111111111111111111 feature-branch") + log.contains("rebase --onto refs/remotes/origin/main 1111111111111111111111111111111111111111 feature-branch") ); assert!(log.contains( - "rebase --onto feature-branch 2222222222222222222222222222222222222222 feature-child" + "rebase --onto refs/heads/feature-branch 2222222222222222222222222222222222222222 feature-child" )); assert!(log.contains("checkout feature-branch")); } @@ -56,7 +56,7 @@ fn sync_uses_remote_old_base_when_local_old_base_is_missing() { cmd.arg("sync"); cmd.assert().success().stdout(predicate::str::contains( - "$ git rebase --onto main 9999999999999999999999999999999999999999 feature-branch", + "$ git rebase --onto refs/remotes/origin/main 9999999999999999999999999999999999999999 feature-branch", )); } @@ -142,13 +142,13 @@ fn sync_rebases_when_default_branch_has_advanced() { cmd.assert() .success() .stdout(predicate::str::contains( - "$ git rebase --onto main aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa feature-base", + "$ git rebase --onto refs/remotes/origin/main aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa feature-base", )) .stdout(predicate::str::contains( - "$ git rebase --onto feature-base 1111111111111111111111111111111111111111 feature-branch", + "$ git rebase --onto refs/heads/feature-base 1111111111111111111111111111111111111111 feature-branch", )) .stdout(predicate::str::contains( - "$ git rebase --onto feature-branch 2222222222222222222222222222222222222222 feature-child", + "$ git rebase --onto refs/heads/feature-branch 2222222222222222222222222222222222222222 feature-child", )); } @@ -222,16 +222,16 @@ fn sync_continue_resumes_after_previous_failure() { .assert() .success() .stdout(predicate::str::contains( - "$ git rebase --onto feature-branch bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb feature-child", + "$ git rebase --onto refs/remotes/origin/feature-branch 2222222222222222222222222222222222222222 feature-child", )) .stdout(predicate::str::contains( "Sync succeeded locally. Run `stck push` to update remotes + PR bases.", )); let log = fs::read_to_string(&log_path).expect("rebase log should exist"); - let first_step = "rebase --onto main 1111111111111111111111111111111111111111 feature-branch"; + let first_step = "rebase --onto refs/remotes/origin/main 1111111111111111111111111111111111111111 feature-branch"; let second_step = - "rebase --onto feature-branch bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb feature-child"; + "rebase --onto refs/remotes/origin/feature-branch 2222222222222222222222222222222222222222 feature-child"; assert_eq!(log.matches(first_step).count(), 1); assert_eq!(log.matches(second_step).count(), 1); assert!( @@ -267,16 +267,16 @@ fn sync_reset_recomputes_from_scratch_after_failure() { "Cleared previous sync state. Recomputing from scratch.", )) .stdout(predicate::str::contains( - "$ git rebase --onto main 1111111111111111111111111111111111111111 feature-branch", + "$ git rebase --onto refs/remotes/origin/main 1111111111111111111111111111111111111111 feature-branch", )) .stdout(predicate::str::contains( - "$ git rebase --onto feature-branch 2222222222222222222222222222222222222222 feature-child", + "$ git rebase --onto refs/heads/feature-branch 2222222222222222222222222222222222222222 feature-child", )); let log = fs::read_to_string(&log_path).expect("rebase log should exist"); - let first_step = "rebase --onto main 1111111111111111111111111111111111111111 feature-branch"; + let first_step = "rebase --onto refs/remotes/origin/main 1111111111111111111111111111111111111111 feature-branch"; let second_step = - "rebase --onto feature-branch 2222222222222222222222222222222222222222 feature-child"; + "rebase --onto refs/heads/feature-branch 2222222222222222222222222222222222222222 feature-child"; assert_eq!( log.matches(first_step).count(), 2, @@ -357,7 +357,9 @@ fn sync_after_squash_merge_uses_merge_base_for_old_base() { cmd.assert() .success() - .stdout(predicate::str::contains("$ git rebase --onto main")) + .stdout(predicate::str::contains( + "$ git rebase --onto refs/remotes/origin/main", + )) .stdout(predicate::str::contains("feature-branch")) .stdout(predicate::str::contains( "Sync succeeded locally. Run `stck push` to update remotes + PR bases.", @@ -376,7 +378,7 @@ fn sync_falls_back_to_remote_ref_when_merge_base_fails() { cmd.arg("sync"); cmd.assert().success().stdout(predicate::str::contains( - "$ git rebase --onto main 9999999999999999999999999999999999999999 feature-branch", + "$ git rebase --onto refs/remotes/origin/main 9999999999999999999999999999999999999999 feature-branch", )); } @@ -411,7 +413,7 @@ fn sync_continue_uses_merge_base_for_remaining_steps() { )); let log = fs::read_to_string(&log_path).expect("rebase log should exist"); - assert!(log.contains("rebase --onto feature-branch")); + assert!(log.contains("rebase --onto refs/remotes/origin/feature-branch")); assert!(log.contains("feature-child")); } @@ -443,6 +445,98 @@ fn sync_reset_with_merge_base_recomputes_correctly() { .stdout(predicate::str::contains("feature-child")); } +#[test] +fn sync_uses_remote_main_when_local_main_is_stale() { + // After a parent PR merges on GitHub, origin/main advances but local main + // stays stale (user hasn't pulled). Sync must use the fetched remote ref + // for both the merge-base calculation and the --onto target. + // + // Scenario: + // - feature-base PR merged into main on GitHub + // - GitHub auto-retargets feature-branch base to main + // - origin/main at bbbb (advanced, includes merged PR) + // - local main at aaaa (stale, hasn't been pulled) + // + // Expected: rebase uses origin/main (bbbb) for both --onto and old-base + let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); + let log_path = log_path(&temp, "stck-sync-stale-main.log"); + cmd.env("STCK_TEST_LOG", log_path.as_os_str()); + // feature-branch PR base is already retargeted to main (by GitHub auto-retarget) + cmd.env("STCK_TEST_FEATURE_BRANCH_BASE", "main"); + // Local main is stale (hasn't been pulled after the merge) + cmd.env( + "STCK_TEST_LOCAL_MAIN_SHA", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + ); + // origin/main has advanced (includes the merged PR) + cmd.env( + "STCK_TEST_REMOTE_MAIN_SHA", + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + ); + // origin/main is NOT an ancestor of feature-branch (main advanced past fork point) + cmd.env("STCK_TEST_NOT_ANCESTOR_PAIRS", "main:feature-branch"); + cmd.arg("sync"); + + cmd.assert().success(); + + let log = fs::read_to_string(&log_path).expect("sync log should exist"); + + // After the fix, the rebase should use the remote merge-base (bbbb) and + // the remote onto ref (refs/remotes/origin/main), not the stale local + // main (aaaa / refs/heads/main). + assert!( + log.contains("rebase --onto refs/remotes/origin/main bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb feature-branch"), + "sync should use remote merge-base and remote onto ref. Log: {log}" + ); + assert!( + !log.contains( + "rebase --onto refs/remotes/origin/main aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ), + "sync should NOT use stale local main as old-base. Log: {log}" + ); +} + +#[test] +fn sync_chained_rebase_uses_local_ref_for_previously_rebased_branch() { + // After a bottom PR merges, sync cascades through the chain: + // Step 1: rebase feature-branch onto main (from feature-base) + // Step 2: rebase feature-child onto feature-branch + // + // Step 1's --onto target should be refs/remotes/origin/main (remote is + // up-to-date after fetch; local main may be stale). + // + // Step 2's --onto target should be refs/heads/feature-branch (local ref + // was just updated by the rebase in step 1; remote hasn't been pushed yet + // and is stale). + // + // Without the fix, step 2 uses refs/remotes/origin/feature-branch which + // still points to the pre-rebase commit, making the rebase a no-op. + let (temp, mut cmd) = stck_cmd_with_stubbed_tools(); + let log_path = log_path(&temp, "stck-sync-chained.log"); + cmd.env("STCK_TEST_LOG", log_path.as_os_str()); + cmd.arg("sync"); + + cmd.assert().success(); + + let log = fs::read_to_string(&log_path).expect("sync log should exist"); + + // Step 1: onto target must be the remote ref (handles stale local main) + assert!( + log.contains("rebase --onto refs/remotes/origin/main"), + "step 1 should use remote ref for default branch. Log:\n{log}" + ); + + // Step 2: onto target must be the LOCAL ref (parent was just rebased) + assert!( + log.contains("rebase --onto refs/heads/feature-branch"), + "step 2 should use local ref for branch rebased in prior step. Log:\n{log}" + ); + assert!( + !log.contains("rebase --onto refs/remotes/origin/feature-branch"), + "step 2 should NOT use stale remote ref for branch rebased in prior step. Log:\n{log}" + ); +} + #[test] fn sync_blocked_while_push_state_exists() { let (temp, mut push) = stck_cmd_with_stubbed_tools();