diff --git a/src/change.rs b/src/change.rs index 739b12c..b481d60 100644 --- a/src/change.rs +++ b/src/change.rs @@ -143,53 +143,6 @@ pub struct Change { } impl Change { - pub fn render_pr_ui( - &self, - changes: &[Self], - short_name: &str, - merged_prs: &[Pr], - ) -> Result<()> { - let commit = self.local_change.commit()?; - let mut index = None; - let title = String::from( - commit - .summary() - .context("failed to get commit summary")? - .context("commit has no summary")?, - ); - let mut body = String::from( - commit - .body() - .context("failed to get commit body")? - .context("commit has no body")?, - ); - body.push_str("\n\n---\n\n"); - body.push_str("**Stack**:\n"); - for (i, c) in changes.iter().enumerate() { - body.push_str(&format!("- #{}", c.pr.number)); - if self.pr.number == c.pr.number { - index = Some(i); - body.push('⬅'); - } - body.push('\n'); - } - let index = index.expect( - "render_pr_ui asked to render into a stack of changes which does not contain self?", - ); - for pr in merged_prs.iter().rev() { - body.push_str(&format!("- #{}\n", pr.number)); - } - body.push_str(&format!("- `{}`\n\n(Note: Closed and merged PRs may not be reflected here and PR numbering is not stable.)\n", env::get().base_branch())); - let count = changes.len() + merged_prs.len(); - let position = count - index; - let prefix = if short_name.is_empty() { - "".into() - } else { - format!("{}: ", short_name) - }; - self.pr - .set_title_and_body(&format!("[{prefix}{position}/{count}]: {title}"), &body) - } /// Adapted from https://joshcannon.me/2025/04/05/pr-interdiff.html pub fn interdiff(&self) -> Result { let change = self.local_change.id.as_str(); @@ -236,15 +189,6 @@ impl Change { .with_context(|| format!("failed to generate interdiff for change {change}"))?; Ok(out) } - pub fn merge(&self) -> Result<()> { - let commit = self.local_change.commit()?; - let subject_raw = commit.summary()?.context("couldn't get summary")?; - let subject = format!("{} (#{})", subject_raw, self.pr.number); - let body = commit.body()?.context("couldn't get summary")?; - let sha = format!("{}", self.local_change.oid); - self.pr.merge(&subject, body, &sha)?; - Ok(()) - } } fn tree<'repo>(commit: &Commit<'repo>) -> Result> { diff --git a/src/cli.rs b/src/cli.rs index 682e056..34c5c2e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -6,9 +6,7 @@ use clap::{ArgAction, Args, Parser, Subcommand}; /// /// * Never touches your local branches. The tool only reads from your local branch and attempts to /// mirror it to GitHub by: fetching remote tracking branches, force-pushing namespaced refs, -/// creating PRs, and maintaining PR bodies and comments to present a pseudo-UI for the stack. If -/// you use the `merge` subcommand, it also touches namespaced git-config entries under -/// `branch..praddle-*`, but this is only for optional features. +/// creating PRs and comments with the official GitHub UI. /// * Treats one branch as one patch-stack, where each commit maps 1:1 to a PR. /// * Uses the same "Change-Id" trailer used by Gerrit. You can install the commit-msg hook from /// a Gerrit instance or use the install-hook subcommand to install an embedded copy. @@ -34,11 +32,6 @@ use clap::{ArgAction, Args, Parser, Subcommand}; /// but an extremely short-lived review process. Ideas about how to potentially resolve this is /// documented at https://github.com/slinder1/praddle/blob/main/IDEAS.md and contributions are /// welcome! -/// * Can lose track of merged/closed PR if the user is not careful to use the `merge` subcommand. -/// This may be mildly confusing, but is more-or-less by design: the change commit which corresponds -/// to a merged PR will naturally disappear from the branch on rebase. The `merge` subcommand -/// notes the Change-Id of successful merges in a git-config entry tied to the branch, so -/// it doesn't forget to count them and link to them, but it is purely aesthetics. /// * Currently lacks a lot of polish and documentation. /// /// It reads configuration from the first of the following: @@ -54,12 +47,6 @@ use clap::{ArgAction, Args, Parser, Subcommand}; /// base_branch = "main" /// user_branch_prefix = "users/$USER/" /// -/// The title of PR number `N` in a series of `M` commits is prefixed with: -/// -/// * `[: N/M]: ` if the branch has a description, -/// editable via `git branch --edit-description`, or -/// * `[N/M]: ` otherwise. -/// #[derive(Parser)] #[command(version, verbatim_doc_comment, args_override_self = true)] pub struct Cli { @@ -103,33 +90,12 @@ pub enum Command { /// force-pushed to a corresponding branch named `${user_branch_prefix}${change_id}` on /// `${remote}`. Each commit will be matched to its existing PR or else a new PR will be /// created for it. The PRs will be "stacked" such that they reproduce the local branch - /// sequence, with additional trailers in the PR message body to help reviewers navigate the - /// stack. + /// sequence. /// /// Note: This command will never modify your commits or refs, even their messages. No local /// branches are created or destroyed. All mutation occurs exclusively on the `$remote`. #[command(visible_alias = "p")] Push(Push), - /// With no arguments, print the current stack's short-name. With an argument, set it. - /// - /// The short-name is tracked in the `branch..praddle-shortName` git config entry, - /// and is used to prefix the PR title, e.g. `[${short-name} 3/5] ...` - Name(Name), - /// Merge the next change. - /// - /// If successful, this will modify the local `branch..praddle-mergedChangeIds` git config - /// entry to record that the change was merged. This allows future `praddle push`es to include - /// merged changes in the reviewer stack "UI", keeping the relative numbering of changes stable. - /// - /// If a change's PR is merged in any other way, it will "disappear" from the stack, affecting - /// all downstream numbering and the total number of changes in the stack. If you want to - /// manually correct this, edit `branch..praddle-mergedChangeIds` (a ':' separated list) using - /// e.g. `git config --edit` and append the merged change's ID to the list (or create it, if it - /// does not already exist). - /// - /// If you don't care about the renumbering behavior, you can safely ignore this subcommand (it - /// is purely aesthetic). - Merge(Merge), /// Print the PR URL of the top-most (i.e. last) change which already has one. Url(Url), /// Install a commit-msg hook in the current git repo to create `Change-Id:` trailers. @@ -152,14 +118,6 @@ pub struct Push { pub draft: bool, } -#[derive(Args)] -pub struct Name { - pub new_name: Option, -} - -#[derive(Args)] -pub struct Merge; - #[derive(Args)] pub struct Url; diff --git a/src/gh.rs b/src/gh.rs index d6b4b03..7a8a244 100644 --- a/src/gh.rs +++ b/src/gh.rs @@ -118,15 +118,6 @@ impl Pr { self.state == state } - pub fn set_title_and_body(&self, title: &str, body: &str) -> Result<()> { - let mut body_arg = ArgInlineOrFile::new("body"); - let mut cmd = gh(); - let args = self.args_for("edit", [format!("--title={title}"), body_arg.arg(body)?])?; - cmd.args(args); - exec!(env::get(), dry_return = (), cmd); - Ok(()) - } - pub fn mark_ready(&self, ready: bool) -> Result<()> { let mut cmd = gh(); let opts = if ready { @@ -226,32 +217,6 @@ impl Pr { bail!("gh pr create did not produce a URL") } - pub fn merge(&self, subject: &str, body: &str, sha: &str) -> Result<()> { - let mut cmd = gh(); - let mut body_arg = ArgInlineOrFile::new("body"); - let body_arg_string = body_arg.arg(body)?; - let args = self.args_for( - "merge", - [ - "--squash", - "--match-head-commit", - sha, - "--subject", - subject, - &body_arg_string, - ], - )?; - cmd.args(args); - let output = exec!(env::get(), dry_return = (), cmd); - // gh cli doesn't consider this a failure, but we want to so we don't mistakenly add an - // already-merged change to the metadata. We could instead infer that the change should be - // added to the metadata, but we can't necessarily assume it is the *next* merged change(?) - if String::from_utf8_lossy(output.stderr.as_ref()).contains("was already merged") { - bail!("pr {} was already merged", self.number); - } - Ok(()) - } - pub fn get_url(&self) -> Result { Ok(format!("{}/pull/{}", build_repo_url()?, self.number)) } diff --git a/src/main.rs b/src/main.rs index 292038d..3a19f41 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,7 +5,6 @@ mod change; mod cli; mod env; mod gh; -mod metadata; mod praddle; mod util; diff --git a/src/metadata.rs b/src/metadata.rs deleted file mode 100644 index a338faa..0000000 --- a/src/metadata.rs +++ /dev/null @@ -1,53 +0,0 @@ -use crate::env; -use crate::util::RepoExt; -use anyhow::{Context, Result, bail}; -use std::collections::HashSet; - -#[derive(Default, Debug)] -pub struct StackMetadata { - pub short_name: String, - pub merged_change_ids: Vec, -} - -const SHORT_NAME_KEY: &str = "shortName"; -const MERGED_CHANGE_IDS_KEY: &str = "mergedChangeIds"; - -impl StackMetadata { - pub fn from_repo() -> Result { - let repo = env::get().repo()?; - let branch = repo.head_branch().context("HEAD must be a branch")?; - let branch_config = repo.branch_config(&branch)?; - let short_name = branch_config.get(SHORT_NAME_KEY)?; - let merged_change_ids = branch_config - .get(MERGED_CHANGE_IDS_KEY)? - .split(':') - .filter(|s| !s.is_empty()) - .map(String::from) - .collect(); - let res = StackMetadata { - short_name, - merged_change_ids, - }; - let mut changed_set = HashSet::new(); - for merged_change_id in res.merged_change_ids.iter() { - if !changed_set.insert(merged_change_id.as_str()) { - bail!( - "duplicate in merged_change_ids: {merged_change_id} (correct with `git branch --edit-description`)" - ); - } - } - Ok(res) - } - pub fn to_repo(&self) -> Result<()> { - if env::get().dry_run() { - eprintln!("would-set: {:?}", self); - return Ok(()); - } - let repo = env::get().repo()?; - let branch = repo.head_branch().context("HEAD must be a branch")?; - let mut branch_config = repo.branch_config(&branch)?; - branch_config.set(SHORT_NAME_KEY, &self.short_name)?; - branch_config.set(MERGED_CHANGE_IDS_KEY, &self.merged_change_ids.join(":"))?; - Ok(()) - } -} diff --git a/src/praddle.rs b/src/praddle.rs index f4e76e4..6362cd8 100644 --- a/src/praddle.rs +++ b/src/praddle.rs @@ -5,7 +5,6 @@ use crate::change::{self, AnyChange, Change, LocalChange}; use crate::cli; use crate::env; use crate::gh::{self, Pr, PrState}; -use crate::metadata::StackMetadata; use crate::util::Extract; use anyhow::{Context, Result, bail}; use git2::Repository; @@ -31,8 +30,6 @@ pub fn praddle(cli: cli::Cli) -> Result<()> { } match cli.command { cli::Command::Push(ref args) => push(args), - cli::Command::Name(ref args) => name(args), - cli::Command::Merge(ref args) => merge(args), cli::Command::Url(ref args) => url(args), cli::Command::InstallHook(_) => unreachable!(), } @@ -40,8 +37,6 @@ pub fn praddle(cli: cli::Cli) -> Result<()> { fn push(args: &cli::Push) -> Result<()> { let env = env::get(); - let stack_meta = - StackMetadata::from_repo().context("could not parse branch description metadata")?; let mut reviewers = vec![]; for group_key in args.reviewer_groups.iter() { let group = env @@ -55,20 +50,6 @@ fn push(args: &cli::Push) -> Result<()> { change::get_local_changes().context("could not enumerate current local branch")?; let mut prs_by_change_id = gh::prs_by_change_id(|pr| !pr.in_state(PrState::Closed)) .context("could not enumerate remote prs")?; - let mut merged_prs = vec![]; - for merged_change_id in stack_meta.merged_change_ids { - let pr = prs_by_change_id - .remove(&merged_change_id) - .with_context(|| format!("merged change {} has no pr", merged_change_id))?; - if pr.in_state(PrState::Open) { - bail!( - "pr {} for merged change {} is still open", - pr.number, - merged_change_id, - ); - } - merged_prs.push(pr); - } let mut any_changes = vec![]; for local_change in local_changes { any_changes.push(match prs_by_change_id.remove(&local_change.id) { @@ -159,8 +140,7 @@ fn push(args: &cli::Push) -> Result<()> { c.pr.number, base, ) })?; - c.render_pr_ui(&changes, &stack_meta.short_name, &merged_prs) - .context("could not render pseudo-ui in pr title/body") + Ok(()) }) .collect::>>() .context("could not set pr bases and bodies")?; @@ -200,42 +180,6 @@ fn detect_cycles(any_changes: &[AnyChange]) -> bool { false } -fn name(args: &cli::Name) -> Result<()> { - let mut stack_meta = StackMetadata::from_repo()?; - match args.new_name { - None => println!("{}", stack_meta.short_name), - Some(ref new_name) => { - stack_meta.short_name = new_name.to_string(); - stack_meta.to_repo()?; - } - } - Ok(()) -} - -fn merge(_args: &cli::Merge) -> Result<()> { - let mut stack_meta = - StackMetadata::from_repo().context("could not parse branch description metadata")?; - let local_change = change::get_local_changes() - .context("could not enumerate current local branch")? - .pop() - .context("no local changes")?; - if stack_meta.merged_change_ids.contains(&local_change.id) { - bail!("change {} was already merged", local_change.id); - } - let mut prs_by_change_id = gh::prs_by_change_id(|pr| !pr.in_state(PrState::Closed)) - .context("could not enumerate remote prs")?; - let pr = prs_by_change_id - .remove(&local_change.id) - .with_context(|| format!("change {} has no pr", local_change.id))?; - let change = Change { local_change, pr }; - change.merge()?; - stack_meta - .merged_change_ids - .push(change.local_change.id.clone()); - stack_meta.to_repo()?; - Ok(()) -} - fn url(_args: &cli::Url) -> Result<()> { let local_changes = change::get_local_changes().context("could not enumerate current local branch")?; diff --git a/src/util.rs b/src/util.rs index 4cf2dec..9b4286f 100644 --- a/src/util.rs +++ b/src/util.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: MIT use anyhow::{Context, Result, bail}; -use git2::{Branch, Config, Repository}; use std::fmt::Debug; use std::process::{Command, Output}; @@ -61,72 +60,3 @@ impl Extract for std::result::Result { } } } - -pub trait RepoExt { - fn head_branch(&self) -> Result>; - fn branch_config<'repo>(&self, branch: &'repo Branch) -> Result>; -} - -impl RepoExt for Repository { - fn head_branch<'repo>(&'repo self) -> Result> { - let branch = self.head().context("unknown HEAD")?; - if !branch.is_branch() { - bail!("HEAD is not a branch"); - } - Ok(Branch::wrap(branch)) - } - fn branch_config<'repo>(&self, branch: &'repo Branch) -> Result> { - BranchConfig::new(self, branch) - } -} - -pub struct BranchConfig<'repo> { - branch_name: &'repo str, - config: Config, -} - -impl<'repo> BranchConfig<'repo> { - pub fn new(repo: &Repository, branch: &'repo Branch<'_>) -> Result { - let branch_name = branch_name(branch)?; - let config = config(repo)?; - Ok(Self { - branch_name, - config, - }) - } - fn format_key(&self, key: &str) -> String { - format!("branch.{}.praddle-{key}", self.branch_name) - } - pub fn get(&self, key: &str) -> Result { - let config_key = self.format_key(key); - let value_result = self.config.get_entry(config_key.as_str()); - let value = match value_result { - Ok(ce) => ce - .value() - .context("error getting config entry value")? - .to_string(), - // all the config values can safely default to the empty string - Err(e) if e.code() == git2::ErrorCode::NotFound => "".to_string(), - Err(e) => return Err(anyhow::Error::new(e).context("error getting config entry")), - }; - Ok(value) - } - pub fn set(&mut self, key: &str, val: &str) -> Result<()> { - let config_key = self.format_key(key); - self.config - .set_str(config_key.as_str(), val) - .with_context(|| format!("could not update config key {config_key}"))?; - Ok(()) - } -} - -fn branch_name<'repo>(branch: &'repo Branch) -> Result<&'repo str> { - branch - .name() - .context("HEAD branch has no name")? - .context("HEAD branch name is not valid utf-8") -} - -fn config(repo: &Repository) -> Result { - repo.config().context("repo has no config") -}