From ffdb8029347a548c3d360ecfa8e2cc256abac069 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Sat, 29 Aug 2026 21:54:00 -0400 Subject: [PATCH] fix(stack): name an interrupted rebase, and a mark that cuts nothing Every verb called a conflicted rebase a detached HEAD, advice that would discard the resolution. A stranded mark silently changed the cut; the commands that decide the cut now say so. Change-Id: I23d8a6c12afd14514da525a3a520da5d8413f4de --- plan/stacks.md | 22 +++++++++++++ src/clients/git/run.rs | 25 +++++++++++++-- src/cmd/stack/marks.rs | 39 +++++++++++++++++++++++ src/cmd/stack/write.rs | 30 ++++++++++++++++-- tests/stack_flows.rs | 70 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 182 insertions(+), 4 deletions(-) diff --git a/plan/stacks.md b/plan/stacks.md index fd84d7b..02903a8 100644 --- a/plan/stacks.md +++ b/plan/stacks.md @@ -43,6 +43,28 @@ under Done, and they are the reason the refusals are as loud as they are. resubmit agrees is unchecked ## Done +- [x] Two more from the scenario sweep, both about a command doing something + other than what it said. + **A rebase git stopped in the middle of was reported as a detached + HEAD.** A conflicted rebase leaves HEAD on no branch, so every stack + verb answered "detached HEAD … `git checkout ` first" — true, + useless, and advice that would throw away the resolution sitting in the + working tree. Somebody who has just hit a conflict is the likeliest + reader of any message here, and it was the one message that did not + mention the rebase. `NoCheckout::MidRebase` names it and points at + `--continue`, `--abort` and `stack sync`. + **A mark outside the range is silently not a cut.** A stack recorded as + grouped members reconciles as one pull request per commit when the mark + that grouped them sits on a commit the branch no longer has — which a + plain `git rebase` produces, exactly when nobody is thinking about + marks. `stack mark` reported it when asked; `create` and `resubmit`, + which are where the shape is actually decided, said nothing. + `positions_and_stranded` hands back both halves and they warn. + Also: I wrote a second `rebase_in_progress` before noticing the one + that has been there all along, used by `stack rebase`. The compiler + caught it. That is the fourth time this session a rule already existed + and I wrote it again, which is the thing `docs/testing.md` closes on + - [x] A sweep of every stacking scenario, asking of each whether it works or says clearly why not. Six answered badly and are fixed. **Two exit statuses disagreed with themselves.** `stack view` answered diff --git a/src/clients/git/run.rs b/src/clients/git/run.rs index d8de1a1..645ece7 100644 --- a/src/clients/git/run.rs +++ b/src/clients/git/run.rs @@ -338,6 +338,15 @@ pub(crate) enum NoCheckout { NoRemote { remote: String, known: Vec }, /// A git repository whose `HEAD` names no branch. Detached, + /// The same, during a rebase git has stopped in the middle of. + /// + /// **A rebase in progress detaches `HEAD`**, so every command that reads + /// the current branch reported "detached HEAD … `git checkout ` + /// first" — which is true, useless, and would throw away the conflict + /// resolution in the working tree. Somebody who has just hit a conflict + /// is the likeliest reader of any of these messages, and it was the one + /// message that did not mention the rebase. + MidRebase, /// A git repository whose `HEAD` names a branch no commit has created /// yet. Distinct from [`Detached`](NoCheckout::Detached), and reported as /// it until now: `rev-parse --abbrev-ref HEAD` fails outright on an @@ -373,6 +382,12 @@ pub(crate) fn no_checkout_message(trouble: &NoCheckout, dir: &str) -> String { "detached HEAD in {dir}: atgc reads the branch it works from out of the \ checkout, so `git checkout ` first" ), + NoCheckout::MidRebase => format!( + "a rebase is in progress in {dir}, which leaves HEAD on no branch\n\ + finish it with `git rebase --continue` once the conflicts are resolved, or \ + `git rebase --abort` to put the branch back; `atgc stack sync` then rebases \ + and reconciles together" + ), NoCheckout::Unborn { branch } => format!( "no commits yet in {dir}: HEAD names {branch}, which nothing has created; \ commit something first" @@ -441,8 +456,14 @@ pub fn current_branch() -> Result { match git(&["rev-parse", "--abbrev-ref", "HEAD"]) { Ok(branch) if branch != "HEAD" => Ok(branch), // `HEAD` is what `--abbrev-ref` answers for a detached HEAD, and git - // forbids a branch of that name, so this is not a guess. - Ok(_) => Err(no_checkout(NoCheckout::Detached)), + // forbids a branch of that name, so this is not a guess. Which + // *kind* of detached is the useful half: a rebase git stopped in the + // middle of leaves HEAD here too, and telling somebody to check out + // a branch would throw away the resolution they are standing in. + Ok(_) => Err(no_checkout(match rebase_in_progress() { + true => NoCheckout::MidRebase, + false => NoCheckout::Detached, + })), // The command itself failed, which it does for two known reasons and // an unknown number of others. `symbolic-ref` separates them: it // answers a branch name on an unborn HEAD, where `rev-parse` will diff --git a/src/cmd/stack/marks.rs b/src/cmd/stack/marks.rs index bb1ea85..e498ed6 100644 --- a/src/cmd/stack/marks.rs +++ b/src/cmd/stack/marks.rs @@ -102,6 +102,45 @@ fn regex_quote(value: &str) -> String { /// bottom of a stack merges, and a reconcile that failed there would fail /// exactly when it is most needed. pub(crate) fn positions(branch: &str, commits: &[String]) -> Vec<(usize, String)> { + positions_and_stranded(branch, commits).0 +} + +/// The same, and the marks that cut nothing. +/// +/// **A mark outside the range is silently not a cut**, and silence there +/// changes what a command does without saying so: a stack recorded as two +/// grouped members reconciles as one pull request per commit, because the +/// mark that grouped them is sitting on a commit the branch no longer has. +/// `stack mark` reports it when asked; the commands that *act* on the cut +/// said nothing, so the shape somebody asked for quietly became a different +/// one. A plain `git rebase` without `--update-refs` is the usual cause, +/// which is exactly when nobody is thinking about marks. +pub(crate) fn positions_and_stranded( + branch: &str, + commits: &[String], +) -> (Vec<(usize, String)>, Vec) { + let mut stranded = Vec::new(); + for mark in recorded(branch) { + let sha = git::git_in( + Path::new("."), + &[ + "rev-parse", + "--verify", + "--quiet", + &format!("refs/heads/{mark}^{{commit}}"), + ], + ) + .ok() + .map(|sha| sha.trim().to_string()) + .unwrap_or_default(); + if sha.is_empty() || !commits.contains(&sha) { + stranded.push(mark); + } + } + (positions_inner(branch, commits), stranded) +} + +fn positions_inner(branch: &str, commits: &[String]) -> Vec<(usize, String)> { let mut found: Vec<(usize, String)> = recorded(branch) .into_iter() .filter_map(|mark| { diff --git a/src/cmd/stack/write.rs b/src/cmd/stack/write.rs index 9c3f14e..c7a145d 100644 --- a/src/cmd/stack/write.rs +++ b/src/cmd/stack/write.rs @@ -506,7 +506,8 @@ async fn create_inner(args: CreateArgs, rewritten: &mut Option Result<(Cut, Vec)> { - let marks = super::marks::positions(&branch, commits); + let (marks, stranded) = super::marks::positions_and_stranded(&branch, commits); + warn_stranded_marks(&stranded); let cut = match args.per_commit { true => Cut::per_commit(commits.len()), false => cut_at_marks(commits.len(), &marks), @@ -2624,7 +2625,11 @@ async fn resubmit_inner( // all even in a checkout that has lost the branches. let marks = match args.per_commit { true => Vec::new(), - false => super::marks::positions(&branch, &commits), + false => { + let (marks, stranded) = super::marks::positions_and_stranded(&branch, &commits); + warn_stranded_marks(&stranded); + marks + } }; let cut = match (args.per_commit, marks.is_empty()) { (true, _) => Cut::per_commit(commits.len()), @@ -4096,6 +4101,27 @@ fn bottom_author(chain: &super::Chain<'_>) -> Option { .map(str::to_string) } +/// Say when a recorded mark is cutting nothing. +/// +/// The cut it was recorded to make is not the cut being made, and nothing +/// else says so at the moment it matters: `stack mark` reports a stranded +/// mark only when somebody asks it to, and the commands that act on the cut +/// are the ones changing shape because of it. +fn warn_stranded_marks(stranded: &[String]) { + if stranded.is_empty() { + return; + } + crate::term::say::warning!( + Git, + "{} recorded mark(s) sit outside this range and are cutting nothing: {}\n\ + the members below are cut without them. `atgc stack rebase` carries marks and a \ + plain `git rebase` does not; `atgc stack mark` shows where each one is, and \ + `--forget ` drops one", + stranded.len(), + stranded.join(", "), + ); +} + /// Say so when a failed merge call may have moved the branch anyway. /// /// **"Retry later" is the wrong advice for a call whose outcome is diff --git a/tests/stack_flows.rs b/tests/stack_flows.rs index 18e92fc..9f101ca 100644 --- a/tests/stack_flows.rs +++ b/tests/stack_flows.rs @@ -4480,3 +4480,73 @@ fn an_at_uri_that_resolves_to_nothing_gets_no_bare_key_advice() { run.stderr ); } + +/// **A rebase git stopped in the middle of is not a detached HEAD somebody +/// chose**, and every stack verb said it was. +/// +/// A conflicted rebase leaves HEAD on no branch, so `current_branch` failed +/// with "detached HEAD … `git checkout ` first" — true, useless, and +/// advice that would throw away the resolution in the working tree. Somebody +/// who has just hit a conflict is the likeliest reader of any message here, +/// and it was the one message that did not mention the rebase. +#[test] +fn a_conflicted_rebase_is_reported_as_one_and_not_as_a_detached_head() { + let world = published_stack("mid-rebase-report"); + world.checkout.git(&["checkout", "-q", "main"]); + world + .checkout + .commit("one.txt", "theirs\n", "chore: conflicting change", None); + world.checkout.sync_remote("main"); + world.checkout.git(&["checkout", "-q", "feature"]); + world + .run(&["stack", "rebase"]) + .refused("the rebase stopped"); + + for verb in ["view", "resubmit"] { + let run = world.run(&["stack", verb]); + assert!( + run.stderr.contains("a rebase is in progress"), + "`atgc stack {verb}` did not mention the rebase\n--- stderr ---\n{}", + run.stderr + ); + assert!( + run.stderr.contains("--abort"), + "the way back was not named\n--- stderr ---\n{}", + run.stderr + ); + } +} + +/// **A mark outside the range is silently not a cut**, and the commands that +/// act on the cut said nothing about it. +/// +/// A stack recorded as grouped members reconciles as one pull request per +/// commit when the mark that grouped them is left on a commit the branch no +/// longer has — which a plain `git rebase` does, exactly when nobody is +/// thinking about marks. `stack mark` reports it when asked; `create` and +/// `resubmit` now say it where the shape is being decided. +#[test] +fn a_mark_left_outside_the_range_is_named_where_the_cut_is_made() { + let world = published_stack("stranded-mark-warns"); + world.run(&["stack", "mark", "part1", "HEAD~1"]).success(); + world.checkout.git(&["checkout", "-q", "main"]); + world + .checkout + .commit("z.txt", "z\n", "chore: move main", None); + world.checkout.sync_remote("main"); + world.checkout.git(&["checkout", "-q", "feature"]); + // A plain rebase, which does not carry marks. + world.checkout.git(&["rebase", "-q", "origin/main"]); + + let run = world.run(&["stack", "resubmit", "--dry-run"]).success(); + assert!( + run.stderr.contains("cutting nothing: part1"), + "the stranded mark was not named where the cut was decided\n--- stderr ---\n{}", + run.stderr + ); + assert!( + run.stderr.contains("stack rebase"), + "the warning did not say what carries marks\n--- stderr ---\n{}", + run.stderr + ); +} -- 2.51.2