From f87ec7a62d06a44e2c3163d5ee731b4fa85f4521 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Mon, 24 Aug 2026 23:13:27 -0400 Subject: [PATCH] feat(stack)!: name marks for you, and ask before a pull per commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `atgc stack mark HEAD~3` names the cut after that commit's subject. An unmarked branch of more than three commits now says what it is about to open and asks to be told again — by marking, by `--per-commit`, or once and for all with `git config stack.perCommit true`. Marking with the branch you are on is refused with the reason. --- src/cmd/stack/marks.rs | 10 +++++ src/cmd/stack/mod.rs | 9 ++-- src/cmd/stack/read.rs | 99 +++++++++++++++++++++++++++++++++++++----- src/cmd/stack/write.rs | 47 +++++++++++++++++++- tests/stack_flows.rs | 91 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 242 insertions(+), 14 deletions(-) diff --git a/src/cmd/stack/marks.rs b/src/cmd/stack/marks.rs index 4db3570..7c516fa 100644 --- a/src/cmd/stack/marks.rs +++ b/src/cmd/stack/marks.rs @@ -133,6 +133,16 @@ pub(crate) fn positions(branch: &str, commits: &[String]) -> Vec<(usize, String) /// looks like — the equivalent of `gh stack modify`'s fold and reorder — so /// it is allowed, and reported, rather than refused. pub(crate) fn place(branch: &str, mark: &str, rev: &str) -> Result { + // The branch being stacked already ends the top member, and git refuses + // to force-update a checked-out branch anyway — but it refuses with a + // sentence about worktrees that says nothing about stacks, and the + // recovery is not obvious from it. + if mark == branch { + bail!( + "{branch} is the branch you are on, and it already ends the top pull request\n\ + mark a cut below it with another name: `atgc stack mark part1 HEAD~1`" + ); + } let target = git::git_in( Path::new("."), &["rev-parse", "--verify", &format!("{rev}^{{commit}}")], diff --git a/src/cmd/stack/mod.rs b/src/cmd/stack/mod.rs index 55e5441..847f278 100644 --- a/src/cmd/stack/mod.rs +++ b/src/cmd/stack/mod.rs @@ -404,9 +404,11 @@ pub(crate) enum Command { /// /// A stack is cut at *marks*: ordinary local branches pointing at commits /// inside `/..HEAD`, each ending one pull request, with - /// the branch you are on ending the top one. `atgc stack mark part1 - /// HEAD~3` points a branch there and records it as a cut of this branch; - /// with no arguments it lists what is recorded and where each one sits. + /// the branch you are on ending the top one. `atgc stack mark HEAD~3` + /// points a branch there and records it as a cut of this branch, named + /// after the commit it lands on; `atgc stack mark part1 HEAD~3` names it + /// yourself. With no arguments it lists what is recorded and where each + /// one sits. /// /// Recorded, not guessed: a branch that merely happens to point into the /// range — `backup` before a rebase, an old worktree's branch — is not a @@ -419,6 +421,7 @@ pub(crate) enum Command { /// /// Examples: /// atgc stack mark + /// atgc stack mark HEAD~3 /// atgc stack mark part1 HEAD~3 /// atgc stack mark --forget part1 #[command(verbatim_doc_comment)] diff --git a/src/cmd/stack/read.rs b/src/cmd/stack/read.rs index 75bb8da..766894e 100644 --- a/src/cmd/stack/read.rs +++ b/src/cmd/stack/read.rs @@ -395,12 +395,13 @@ mod tests { #[derive(clap::Args, Debug)] pub(crate) struct MarkArgs { - /// Branch name to mark with, e.g. `part1` - pub name: Option, - /// Where it goes: any revision inside the range, e.g. HEAD~3 + /// Where the mark goes: any revision inside the range, e.g. HEAD~3. + /// A name may be given first: `atgc stack mark part1 HEAD~3` + pub first: Option, + /// Where it goes, when a name was given pub rev: Option, /// Forget this mark, leaving its branch alone - #[arg(long, value_name = "NAME", conflicts_with_all = ["name", "rev"])] + #[arg(long, value_name = "NAME", conflicts_with_all = ["first", "rev"])] pub forget: Option, /// Git remote pointing at the repo #[arg(long, default_value = "origin")] @@ -435,6 +436,66 @@ pub(in crate::cmd) struct MarksJson { pub marks: Vec, } +/// A branch name for a mark on `rev`, from the subject of the commit it +/// lands on. +/// +/// `feat(stack): cut the range at marks` becomes `cut-the-range-at-marks`: +/// the conventional-commit prefix is dropped because every commit in a repo +/// that uses them would otherwise start the same way, and what distinguishes +/// one cut from another is the rest. Trimmed to something a listing can show +/// whole, and suffixed if that name is taken, because a mark that silently +/// moved an existing branch would be a re-cut nobody asked for. +fn name_for(rev: &str) -> Result { + let subject = git::git_in( + std::path::Path::new("."), + &["log", "-1", "--format=%s", rev], + )?; + let subject = subject + .split_once(": ") + .map(|(_, rest)| rest) + .unwrap_or(&subject); + let slug: String = subject + .chars() + .map(|c| match c.is_ascii_alphanumeric() { + true => c.to_ascii_lowercase(), + false => '-', + }) + .collect::() + .split('-') + .filter(|part| !part.is_empty()) + .take(5) + .collect::>() + .join("-"); + let slug = match slug.is_empty() { + // A subject of nothing but punctuation, which git allows. + true => "mark".to_string(), + false => slug.chars().take(40).collect(), + }; + let taken = |name: &str| { + git::git_in( + std::path::Path::new("."), + &[ + "rev-parse", + "--verify", + "--quiet", + &format!("refs/heads/{name}"), + ], + ) + .map(|out| !out.trim().is_empty()) + .unwrap_or(false) + }; + if !taken(&slug) { + return Ok(slug); + } + for n in 2..100 { + let candidate = format!("{slug}-{n}"); + if !taken(&candidate) { + return Ok(candidate); + } + } + bail!("could not find a free branch name from {slug}; name the mark yourself") +} + /// `atgc stack mark`: place one, forget one, or list them. /// /// Deliberately offline. Marks are a fact about this checkout, not about any @@ -456,16 +517,34 @@ pub(crate) fn mark(args: MarkArgs) -> Result<()> { None => git::remote_default_branch(&args.remote).unwrap_or_else(|| "main".to_string()), }; let base = format!("{}/{}", args.remote, target); - if !git::ref_exists(&base) { - git::fetch(&args.remote, &target)?; + if !git::ref_exists(&base) + && let Err(e) = git::fetch(&args.remote, &target) + { + // Marks are a fact about this checkout, and a listing of them should + // not depend on a host being up. What it does need is the range they + // sit in, so this only fails when the target is nowhere to be found + // locally either — and says that, rather than handing over git's + // sentence about a repository that does not appear to be one. + crate::logging::debug::dump_err("stack mark: fetch failed", &e); + bail!( + "{base} is not in this checkout and {} could not be fetched, so there is no \ + range to place marks in\n\ + fetch it when the remote is reachable, or name another target with --target", + args.remote, + ); } let commits = crate::clients::git::patch::commits_since(&base)?; - if let Some(name) = &args.name { - let Some(rev) = &args.rev else { - bail!("say where the mark goes: `atgc stack mark {name} HEAD~3`"); + if let Some(first) = &args.first { + // One argument is the revision, and the name comes off the commit + // it lands on: naming a cut is a chore, and the subject of the + // commit that ends it is the name somebody would have typed anyway. + // Two arguments are the name and the revision, in that order. + let (name, rev) = match &args.rev { + Some(rev) => (first.clone(), rev.clone()), + None => (name_for(first)?, first.clone()), }; - let placed = super::marks::place(&branch, name, rev)?; + let placed = super::marks::place(&branch, &name, &rev)?; if !args.json { let said = match placed { super::marks::Placed::Created => format!("marked {rev} as the end of {name}"), diff --git a/src/cmd/stack/write.rs b/src/cmd/stack/write.rs index ca1c483..7147a32 100644 --- a/src/cmd/stack/write.rs +++ b/src/cmd/stack/write.rs @@ -463,10 +463,37 @@ async fn create_inner(args: CreateArgs, rewritten: &mut Option Cut::per_commit(commits.len()), - false => cut_at_marks(commits.len(), &super::marks::positions(&branch, &commits)), + false => cut_at_marks(commits.len(), &marks), }; + // An unmarked branch of any length opens a pull request per commit, and + // that is a fine default for two or three — it is the jj-shaped + // workflow. Past that it is almost never what somebody meant: the usual + // cause is not knowing marks exist, and the result is eight pull + // requests nobody can review, each one commit of a change. Eight records + // are also eight records to close by hand, since a stack is not + // something `create` can undo. + // + // So the wide case says what it is about to do and asks to be told + // again, either by marking the cuts or by saying `--per-commit` out + // loud. `stack.perCommit` in git config is the way to stop being asked. + if !args.per_commit + && marks.is_empty() + && commits.len() > UNMARKED_LIMIT + && !per_commit_by_config() + { + bail!( + "{} commits over {base}, and no marks: that is {} pull requests of one commit \ + each\n\ + cut it where the changes are — `atgc stack mark `, once per pull request \ + — or say `--per-commit` if a pull per commit is what you want\n\ + `git config stack.perCommit true` makes --per-commit this checkout's default", + commits.len(), + commits.len(), + ); + } // A commit whose id the pending rewrite has not minted yet is planned // under the id that rewrite will give it, which only a dry run can see. let planned = plan_groups(&selection.did, &commits, &cut.groups, true)?; @@ -1130,6 +1157,24 @@ fn one_group_per_commit(total: usize) -> Groups { (0..total).map(|i| vec![i]).collect() } +/// How many commits an unmarked branch may stack one-per-commit before +/// `create` stops to ask. +/// +/// Three is the largest number that still reads as a deliberate series +/// rather than an accident: the jj workflow these commands grew out of opens +/// a pull per change, and two or three of those is an ordinary stack. Above +/// it, the odds that somebody meant "one pull request per commit" fall off a +/// cliff, and the cost of guessing wrong is a pile of records to close by +/// hand. +const UNMARKED_LIMIT: usize = 3; + +/// Whether this checkout has said, once, that a pull per commit is what it +/// wants — so the question above is asked at most one time per repository. +fn per_commit_by_config() -> bool { + crate::clients::git::config::local_config(Path::new("."), "stack.perCommit") + .is_some_and(|v| matches!(v.trim(), "true" | "1" | "yes" | "on")) +} + /// A cut branch: the groups, and the branch name that ended each one. /// /// The names are not written to any record — a member's source branch is the diff --git a/tests/stack_flows.rs b/tests/stack_flows.rs index 78d0063..6febfe7 100644 --- a/tests/stack_flows.rs +++ b/tests/stack_flows.rs @@ -55,6 +55,18 @@ fn knot_mailbox(ids: &[&str]) -> String { .collect() } +/// Four commits with change-ids: one more than an unmarked stack may open +/// without being asked about. +fn four_commit_branch(world: &Scenario) { + three_commit_branch(world); + world.checkout.commit( + "four.txt", + "four\n", + "feat: fourth", + Some("Ifourth0000000000000000000000000000000a"), + ); +} + /// A stacked branch that has already been published. fn published_stack(label: &str) -> Scenario { let world = Scenario::new(label); @@ -1575,6 +1587,38 @@ fn merging_a_multi_commit_member_lands_every_commit_in_it() { ); } +/// One argument is a revision, and the mark is named after the commit it +/// lands on. Naming a cut is a chore, and the name somebody would have typed +/// is sitting in the commit subject. +#[test] +fn a_mark_names_itself_after_the_commit_it_ends() { + let world = Scenario::new("mark-autoname"); + three_commit_branch(&world); + + let placed = world.run(&["stack", "mark", "HEAD~1"]).success(); + assert!(placed.stdout.contains("middle"), "{}", placed.stdout); + + // It is a real branch, at the commit named, and recorded as a cut. + assert_eq!( + world.checkout.git(&["rev-parse", "middle"]).trim(), + world.checkout.git(&["rev-parse", "HEAD~1"]).trim(), + ); + world.run(&["stack", "create"]).success(); + assert_eq!(titles(&world, ALICE), ["feat: bottom", "feat: top"]); +} + +/// Marking with the branch you are on is refused with the reason, not with +/// git's sentence about worktrees. +#[test] +fn marking_with_the_branch_you_are_on_is_refused() { + let world = Scenario::new("mark-self"); + three_commit_branch(&world); + + world + .run(&["stack", "mark", "feature", "HEAD~1"]) + .refused("already ends the top pull request"); +} + /// A mark can be forgotten, and one left outside the range is listed as such. /// /// Forgetting is how a cut is undone without losing the sha, and the @@ -1653,3 +1697,50 @@ fn per_commit_recuts_a_marked_stack_without_dropping_records() { "a re-cut destroyed a record instead of splitting around it: {before:?} -> {after:?}" ); } + +/// An unmarked branch wide enough to be an accident is asked about rather +/// than opened. +/// +/// The failure this prevents is the one that reads as success: eight commits +/// become eight pull requests of one commit each, nobody can review them, +/// and closing them is eight more acts. Two remedies, both named, and a +/// config for anybody who really does want a pull per commit every time. +#[test] +fn a_wide_unmarked_branch_is_asked_about_before_it_opens_a_pull_per_commit() { + let world = Scenario::new("create-wide-unmarked"); + four_commit_branch(&world); + + world + .run(&["stack", "create"]) + .refused("4 pull requests of one commit each"); + world.with(|w| assert!(w.collection(ALICE, PULL_NSID).is_empty())); + + // Saying it out loud goes through. + world.run(&["stack", "create", "--per-commit"]).success(); + assert_eq!(world.pulls(ALICE).len(), 4); +} + +/// Marking the branch answers the question, without --per-commit. +#[test] +fn marking_a_wide_branch_is_the_other_way_past_the_question() { + let world = Scenario::new("create-wide-marked"); + four_commit_branch(&world); + world.run(&["stack", "mark", "HEAD~2"]).success(); + + world.run(&["stack", "create"]).success(); + assert_eq!(world.pulls(ALICE).len(), 2, "the marks decide the count"); +} + +/// `stack.perCommit` in git config answers it once and for all: a checkout +/// that always wants a pull per commit should not be asked every time. +#[test] +fn a_checkout_can_say_once_that_it_wants_a_pull_per_commit() { + let world = Scenario::new("create-wide-configured"); + four_commit_branch(&world); + world + .checkout + .git(&["config", "--local", "stack.perCommit", "true"]); + + world.run(&["stack", "create"]).success(); + assert_eq!(world.pulls(ALICE).len(), 4); +} -- 2.51.2