diff --git a/git-branchless-lib/src/core/eventlog.rs b/git-branchless-lib/src/core/eventlog.rs index f534bf926..09f1cc60c 100644 --- a/git-branchless-lib/src/core/eventlog.rs +++ b/git-branchless-lib/src/core/eventlog.rs @@ -580,7 +580,6 @@ INSERT INTO event_log VALUES ( /// /// Returns: All the events in the database, ordered from oldest to newest. #[instrument] - pub fn get_events(&self) -> eyre::Result> { let mut stmt = self.conn.prepare( " diff --git a/git-branchless-submit/src/branch_forge.rs b/git-branchless-submit/src/branch_forge.rs index 5484de52f..2c870bf9b 100644 --- a/git-branchless-submit/src/branch_forge.rs +++ b/git-branchless-submit/src/branch_forge.rs @@ -15,7 +15,7 @@ use lib::try_exit_code; use lib::util::{ExitCode, EyreExitOr}; use tracing::{instrument, warn}; -use crate::{CommitStatus, CreateStatus, Forge, SubmitOptions, SubmitStatus}; +use crate::{CommitStatus, CreateStatus, Forge, SubmitOptions, SubmitStatus, UpdateStatus}; #[derive(Debug)] pub struct BranchForge<'a> { @@ -253,7 +253,7 @@ These remotes are available: {}", commit_status.local_commit_name.map(|local_commit_name| { ( commit_oid, - CreateStatus { + CreateStatus::Created { final_commit_oid: commit_oid, local_commit_name, }, @@ -269,20 +269,20 @@ These remotes are available: {}", &mut self, commits: HashMap, _options: &SubmitOptions, - ) -> EyreExitOr<()> { - let branches_by_remote: BTreeMap> = commits - .into_values() - .flat_map(|commit_status| match commit_status { + ) -> EyreExitOr> { + let branches_by_remote: BTreeMap> = commits + .into_iter() + .flat_map(|(commit_oid, commit_status)| match commit_status { CommitStatus { submit_status: _, remote_name: Some(remote_name), local_commit_name: Some(local_commit_name), remote_commit_name: _, - } => Some((remote_name, local_commit_name)), + } => Some((remote_name, (local_commit_name, commit_oid))), commit_status => { warn!( ?commit_status, - "Commit was requested to be updated, but it did not have the requisite information (remote name, local branch name)." + "Commit was requested to be updated, but it did not have an associated remote and a local commit name" ); None } @@ -299,24 +299,42 @@ These remotes are available: {}", .values() .map(|branch_names| branch_names.len()) .sum(); + let mut result = HashMap::new(); progress.notify_progress(0, total_num_branches); for (remote_name, branch_names) in branches_by_remote { let mut args = vec!["push", "--force-with-lease", &remote_name]; - args.extend(branch_names.iter().map(|s| s.as_str())); + args.extend( + branch_names + .iter() + .map(|(branch, _commit_oid)| branch.as_str()), + ); match self.git_run_info.run(&effects, Some(event_tx_id), &args)? { Ok(()) => {} Err(exit_code) => { writeln!( effects.get_output_stream(), "Failed to push branches: {}", - branch_names.into_iter().join(", ") + branch_names + .iter() + .map(|(branch, _commit_oid)| branch) + .join(", ") )?; return Ok(Err(exit_code)); } } progress.notify_progress_inc(branch_names.len()); + + // FIXME: report push errors + result.extend(branch_names.iter().map(|(branch, commit_oid)| { + ( + *commit_oid, + UpdateStatus::Updated { + local_commit_name: branch.to_owned(), + }, + ) + })); } - Ok(Ok(())) + Ok(Ok(result)) } } diff --git a/git-branchless-submit/src/github.rs b/git-branchless-submit/src/github.rs index 39eef9c06..9f3dea2e1 100644 --- a/git-branchless-submit/src/github.rs +++ b/git-branchless-submit/src/github.rs @@ -33,6 +33,7 @@ use tracing::warn; use crate::branch_forge::BranchForge; use crate::SubmitStatus; +use crate::UpdateStatus; use crate::{CommitStatus, CreateStatus, Forge, SubmitOptions}; /// Testing environment variable. When this is set, the executable will use the @@ -298,7 +299,7 @@ impl Forge for GithubForge<'_> { .copied() .map(|(commit_oid, commit_status)| { let commit_status = match created_branches.get(&commit_oid) { - Some(CreateStatus { + Some(CreateStatus::Created { final_commit_oid: _, local_commit_name, }) => CommitStatus { @@ -309,7 +310,9 @@ impl Forge for GithubForge<'_> { // Expecting this to be the same as the local branch name (for now): remote_commit_name: Some(local_commit_name.clone()), }, - None => commit_status.clone(), + Some(CreateStatus::Skipped { .. } | CreateStatus::Err { .. }) | None => { + commit_status.clone() + } }; (commit_oid, commit_status) }) @@ -368,7 +371,7 @@ impl Forge for GithubForge<'_> { &mut self, commit_statuses: HashMap, options: &SubmitOptions, - ) -> EyreExitOr<()> { + ) -> EyreExitOr> { let effects = self.effects; let SubmitOptions { create: _, @@ -392,6 +395,7 @@ impl Forge for GithubForge<'_> { let commit_set: CommitSet = commit_statuses.keys().copied().collect(); let commit_oids = self.dag.sort(&commit_set)?; + let mut result = HashMap::new(); { let (effects, progress) = effects.start_operation(OperationType::UpdateCommits); progress.notify_progress(0, commit_oids.len()); @@ -472,7 +476,7 @@ impl Forge for GithubForge<'_> { branch_forge.update(singleton(&commit_statuses, commit_oid, |x| x), options)? ); - // Update metdata: + // Update metadata: try_exit_code!(self.client.update_pull_request( &effects, pull_request_info.number, @@ -485,10 +489,21 @@ impl Forge for GithubForge<'_> { options )?); progress.notify_progress_inc(1); + + // FIXME: report push/update errors + result.insert( + commit_oid, + UpdateStatus::Updated { + local_commit_name: commit_status + .local_commit_name + .clone() + .unwrap_or_else(|| commit_oid.to_string()), + }, + ); } } - Ok(Ok(())) + Ok(Ok(result)) } } diff --git a/git-branchless-submit/src/lib.rs b/git-branchless-submit/src/lib.rs index 6762b3207..a77a63d26 100644 --- a/git-branchless-submit/src/lib.rs +++ b/git-branchless-submit/src/lib.rs @@ -13,12 +13,13 @@ mod branch_forge; pub mod github; pub mod phabricator; -use std::collections::{BTreeSet, HashMap}; +use std::collections::HashMap; use std::fmt::{Debug, Write}; use std::time::SystemTime; use branch_forge::BranchForge; use cursive_core::theme::{BaseColor, Effect, Style}; +use cursive_core::utils::markup::StyledString; use git_branchless_invoke::CommandContext; use git_branchless_test::{RawTestOptions, ResolvedTestOptions, Verbosity}; use github::GithubForge; @@ -27,7 +28,6 @@ use lazy_static::lazy_static; use lib::core::dag::{union_all, CommitSet, Dag}; use lib::core::effects::Effects; use lib::core::eventlog::{EventLogDb, EventReplayer}; -use lib::core::formatting::{Pluralize, StyledStringBuilder}; use lib::core::repo_ext::{RepoExt, RepoReferencesSnapshot}; use lib::git::{GitRunInfo, NonZeroOid, Repo}; use lib::try_exit_code; @@ -43,13 +43,20 @@ use tracing::{debug, info, instrument, warn}; use crate::github::github_push_remote; lazy_static! { - /// The style for branches which were successfully submitted. - pub static ref STYLE_PUSHED: Style = + /// The style for commits which were successfully submitted. + pub static ref STYLE_SUCCESS: Style = Style::merge(&[BaseColor::Green.light().into(), Effect::Bold.into()]); - /// The style for branches which were not submitted. + /// The style for commits which were not submitted. pub static ref STYLE_SKIPPED: Style = Style::merge(&[BaseColor::Yellow.light().into(), Effect::Bold.into()]); + + /// The style for commits which failed to be submitted. + pub static ref STYLE_FAILED: Style = + Style::merge(&[BaseColor::Red.light().into(), Effect::Bold.into()]); + + /// The style for informational text. + pub static ref STYLE_INFO: Style = Effect::Dim.into(); } /// The status of a commit, indicating whether it needs to be updated remotely. @@ -141,18 +148,47 @@ pub struct SubmitOptions { /// The result of creating a commit. #[derive(Clone, Debug)] -pub struct CreateStatus { - /// The commit OID after carrying out the creation process. Usually, this - /// will be the same as the original commit OID, unless the forge amends it - /// (e.g. to include a change ID). - pub final_commit_oid: NonZeroOid, - - /// An identifier corresponding to the commit, for display purposes only. - /// This may be a branch name, a change ID, the commit summary, etc. - /// - /// This does not necessarily correspond to the commit's name/identifier in - /// the forge (e.g. not a code review link). - pub local_commit_name: String, +pub enum CreateStatus { + /// The commit was successfully created. + Created { + /// The commit OID after carrying out the creation process. Usually, this + /// will be the same as the original commit OID, unless the forge amends it + /// (e.g. to include a change ID). + final_commit_oid: NonZeroOid, + + /// An identifier corresponding to the commit, for display purposes only. + /// This may be a branch name, a change ID, the commit summary, etc. + /// + /// This does not necessarily correspond to the commit's name/identifier in + /// the forge (e.g. not a code review link). + local_commit_name: String, + }, + + /// The commit was skipped because it didn't already exist on the remote and + /// `--create` was not passed. + Skipped, + + /// The commit could not be created due to an error. + Err { + /// The error message to display to the user. + message: String, + }, +} + +/// The result of updating a commit. +/// +/// TODO: have an `Err` case. +#[derive(Clone, Debug)] +pub enum UpdateStatus { + /// The commit was successfully updated. + Updated { + /// An identifier corresponding to the commit, for display purposes only. + /// This may be a branch name, a change ID, the commit summary, etc. + /// + /// This does not necessarily correspond to the commit's name/identifier in + /// the forge (e.g. not a code review link). + local_commit_name: String, + }, } /// "Forge" refers to a Git hosting provider, such as GitHub, GitLab, etc. @@ -176,7 +212,7 @@ pub trait Forge: Debug { &mut self, commits: HashMap, options: &SubmitOptions, - ) -> EyreExitOr<()>; + ) -> EyreExitOr>; } /// `submit` command. @@ -294,6 +330,7 @@ fn submit( }; let unioned_revset = Revset(revsets.iter().map(|Revset(inner)| inner).join(" + ")); + let commit_oids = dag.sort(&commit_set)?; let mut forge = select_forge( effects, git_run_info, @@ -304,217 +341,223 @@ fn submit( &unioned_revset, forge_kind, )?; - let statuses = try_exit_code!(forge.query_status(commit_set)?); - debug!(?statuses, "Commit statuses"); - - #[allow(clippy::type_complexity)] - let (_local_commits, unsubmitted_commits, commits_to_update, commits_to_skip): ( - HashMap, - HashMap, - HashMap, - HashMap, - ) = statuses.into_iter().fold(Default::default(), |acc, elem| { - let (mut local, mut unsubmitted, mut to_update, mut to_skip) = acc; - let (commit_oid, commit_status) = elem; - match commit_status { - CommitStatus { - submit_status: SubmitStatus::Local, - remote_name: _, - local_commit_name: _, - remote_commit_name: _, - } => { - local.insert(commit_oid, commit_status); - } - - CommitStatus { - submit_status: SubmitStatus::Unsubmitted, - remote_name: _, - local_commit_name: _, - remote_commit_name: _, - } => { - unsubmitted.insert(commit_oid, commit_status); - } + let current_statuses = try_exit_code!(forge.query_status(commit_set)?); + debug!(?current_statuses, "Commit statuses"); - CommitStatus { - submit_status: SubmitStatus::NeedsUpdate, - remote_name: _, - local_commit_name: _, - remote_commit_name: _, - } => { - to_update.insert(commit_oid, commit_status); - } + let create_statuses: HashMap = { + let unsubmitted_commits: HashMap = current_statuses + .iter() + .filter(|(_commit_oid, commit_status)| match commit_status { + CommitStatus { + submit_status: SubmitStatus::Unsubmitted, + .. + } => true, + CommitStatus { + submit_status: + SubmitStatus::Local + | SubmitStatus::NeedsUpdate + | SubmitStatus::Unknown + | SubmitStatus::UpToDate, + .. + } => false, + }) + .map(|(commit_oid, create_status)| (*commit_oid, create_status.clone())) + .collect(); + match (create, dry_run) { + (false, _) => unsubmitted_commits + .into_keys() + .map(|commit_oid| (commit_oid, CreateStatus::Skipped)) + .collect(), - CommitStatus { - submit_status: SubmitStatus::UpToDate, - remote_name: _, - local_commit_name: Some(_), - remote_commit_name: _, - } => { - to_skip.insert(commit_oid, commit_status); - } + (true, true) => unsubmitted_commits + .into_iter() + .map(|(commit_oid, commit_status)| { + ( + commit_oid, + CreateStatus::Created { + final_commit_oid: commit_oid, + local_commit_name: commit_status + .local_commit_name + .unwrap_or_else(|| "".to_string()), + }, + ) + }) + .collect(), - // Don't know what to do in these cases 🙃. - CommitStatus { - submit_status: SubmitStatus::Unknown, - remote_name: _, - local_commit_name: _, - remote_commit_name: _, - } - | CommitStatus { - submit_status: SubmitStatus::UpToDate, - remote_name: _, - local_commit_name: None, - remote_commit_name: _, - } => {} + (true, false) => try_exit_code!(forge.create(unsubmitted_commits, &submit_options)?), } - (local, unsubmitted, to_update, to_skip) - }); + }; - let (submitted_commit_names, unsubmitted_commit_names): (BTreeSet, BTreeSet) = { - let unsubmitted_commit_names: BTreeSet = unsubmitted_commits - .values() - .flat_map(|commit_status| commit_status.local_commit_name.clone()) + let update_statuses: HashMap = { + let commits_to_update: HashMap = current_statuses + .iter() + .filter(|(_commit_oid, commit_status)| match commit_status { + CommitStatus { + submit_status: SubmitStatus::NeedsUpdate, + .. + } => true, + CommitStatus { + submit_status: + SubmitStatus::Local + | SubmitStatus::Unsubmitted + | SubmitStatus::Unknown + | SubmitStatus::UpToDate, + .. + } => false, + }) + .map(|(commit_oid, commit_status)| (*commit_oid, commit_status.clone())) .collect(); - if create { - let created_commit_names = if dry_run { - unsubmitted_commit_names.clone() - } else { - let create_statuses = - try_exit_code!(forge.create(unsubmitted_commits, &submit_options)?); - create_statuses - .into_values() - .map( - |CreateStatus { - final_commit_oid: _, - local_commit_name, - }| local_commit_name, + if dry_run { + commits_to_update + .into_iter() + .map(|(commit_oid, commit_status)| { + ( + commit_oid, + UpdateStatus::Updated { + local_commit_name: commit_status + .local_commit_name + .unwrap_or_else(|| "".to_string()), + }, ) - .collect() - }; - (created_commit_names, Default::default()) + }) + .collect() } else { - (Default::default(), unsubmitted_commit_names) + try_exit_code!(forge.update(commits_to_update, &submit_options)?) } }; - let (updated_commit_names, skipped_commit_names): (BTreeSet, BTreeSet) = { - let updated_commit_names = commits_to_update - .iter() - .flat_map(|(_commit_oid, commit_status)| commit_status.local_commit_name.clone()) - .collect(); - let skipped_commit_names = commits_to_skip - .iter() - .flat_map(|(_commit_oid, commit_status)| commit_status.local_commit_name.clone()) - .collect(); - - if !dry_run { - try_exit_code!(forge.update(commits_to_update, &submit_options)?); - } - (updated_commit_names, skipped_commit_names) + let styled = |style: Style, text: &str| { + effects + .get_glyphs() + .render(StyledString::styled(text, style)) }; + let mut had_errors = false; + for commit_oid in commit_oids { + let current_status = match current_statuses.get(&commit_oid) { + Some(status) => status, + None => { + warn!(?commit_oid, "Could not find commit status"); + continue; + } + }; - if !submitted_commit_names.is_empty() { - writeln!( - effects.get_output_stream(), - "{} {}: {}", - if dry_run { "Would submit" } else { "Submitted" }, - Pluralize { - determiner: None, - amount: submitted_commit_names.len(), - unit: ("commit", "commits"), - }, - submitted_commit_names - .into_iter() - .map(|commit_name| effects - .get_glyphs() - .render( - StyledStringBuilder::new() - .append_styled(commit_name, *STYLE_PUSHED) - .build(), - ) - .expect("Rendering commit name")) - .join(", ") - )?; - } - if !updated_commit_names.is_empty() { - writeln!( - effects.get_output_stream(), - "{} {}: {}", - if dry_run { "Would update" } else { "Updated" }, - Pluralize { - determiner: None, - amount: updated_commit_names.len(), - unit: ("commit", "commits"), - }, - updated_commit_names - .into_iter() - .map(|branch_name| effects - .get_glyphs() - .render( - StyledStringBuilder::new() - .append_styled(branch_name, *STYLE_PUSHED) - .build(), - ) - .expect("Rendering commit name")) - .join(", ") - )?; - } - if !skipped_commit_names.is_empty() { - writeln!( - effects.get_output_stream(), - "{} {} (already up-to-date): {}", - if dry_run { "Would skip" } else { "Skipped" }, - Pluralize { - determiner: None, - amount: skipped_commit_names.len(), - unit: ("commit", "commits"), + let commit = repo.find_commit_or_fail(commit_oid)?; + let commit = effects + .get_glyphs() + .render(commit.friendly_describe(effects.get_glyphs())?)?; + + match current_status.submit_status { + SubmitStatus::Local => continue, + SubmitStatus::Unknown => { + warn!( + ?commit_oid, + "Commit status is unknown; reporting info not implemented." + ); + continue; + } + SubmitStatus::UpToDate => { + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_SKIPPED, + if dry_run { "Would skip" } else { "Skipped" } + )?, + info = styled(*STYLE_INFO, "(already up-to-date)")?, + )?; + } + + SubmitStatus::Unsubmitted => match create_statuses.get(&commit_oid) { + Some(CreateStatus::Created { + final_commit_oid: _, + local_commit_name, + }) => { + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_SUCCESS, + if dry_run { "Would submit" } else { "Submitted" }, + )?, + info = styled(*STYLE_INFO, &format!("(as {local_commit_name})"))?, + )?; + } + + Some(CreateStatus::Skipped) => { + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_SKIPPED, + if dry_run { "Would skip" } else { "Skipped" }, + )?, + info = styled(*STYLE_INFO, "(pass --create to submit)")?, + )?; + } + + Some(CreateStatus::Err { message }) => { + had_errors = true; + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_FAILED, + if dry_run { + "Would fail to create" + } else { + "Failed to create" + }, + )?, + info = styled(*STYLE_INFO, &format!("({message})"))?, + )?; + } + + None => { + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_SKIPPED, + if dry_run { "Would skip" } else { "Skipped" }, + )?, + info = styled(*STYLE_INFO, "(unknown reason, bug?)")?, + )?; + } }, - skipped_commit_names - .into_iter() - .map(|commit_name| effects - .get_glyphs() - .render( - StyledStringBuilder::new() - .append_styled(commit_name, *STYLE_SKIPPED) - .build(), - ) - .expect("Rendering commit name")) - .join(", ") - )?; - } - if !unsubmitted_commit_names.is_empty() { - writeln!( - effects.get_output_stream(), - "{} {} (not yet on remote): {}", - if dry_run { "Would skip" } else { "Skipped" }, - Pluralize { - determiner: None, - amount: unsubmitted_commit_names.len(), - unit: ("commit", "commits") + + SubmitStatus::NeedsUpdate => match update_statuses.get(&commit_oid) { + Some(UpdateStatus::Updated { local_commit_name }) => { + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_SUCCESS, + if dry_run { "Would update" } else { "Updated" }, + )?, + info = styled(*STYLE_INFO, &format!("(as {local_commit_name})"))?, + )?; + } + + None => { + writeln!( + effects.get_output_stream(), + "{action} {commit} {info}", + action = styled( + *STYLE_SKIPPED, + if dry_run { "Would skip" } else { "Skipped" }, + )?, + info = styled(*STYLE_INFO, "(unknown reason, bug?)")?, + )?; + } }, - unsubmitted_commit_names - .into_iter() - .map(|commit_name| effects - .get_glyphs() - .render( - StyledStringBuilder::new() - .append_styled(commit_name, *STYLE_SKIPPED) - .build(), - ) - .expect("Rendering commit name")) - .join(", ") - )?; - writeln!( - effects.get_output_stream(), - "\ -These commits {} skipped because they {} not already associated with a remote -repository. To submit them, retry this operation with the --create option.", - if dry_run { "would be" } else { "were" }, - if dry_run { "are" } else { "were" }, - )?; + }; } - Ok(Ok(())) + if had_errors { + Ok(Err(ExitCode(1))) + } else { + Ok(Ok(())) + } } #[instrument] diff --git a/git-branchless-submit/src/phabricator.rs b/git-branchless-submit/src/phabricator.rs index b21fb3c89..1ea69f745 100644 --- a/git-branchless-submit/src/phabricator.rs +++ b/git-branchless-submit/src/phabricator.rs @@ -10,7 +10,7 @@ use std::time::SystemTime; use cursive_core::theme::Effect; use cursive_core::utils::markup::StyledString; -use git_branchless_opts::Revset; +use git_branchless_opts::{Revset, TestSearchStrategy}; use git_branchless_test::{ run_tests, FixInfo, ResolvedTestOptions, TestOutput, TestResults, TestStatus, TestingAbortedError, Verbosity, @@ -35,7 +35,9 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use tracing::{instrument, warn}; -use crate::{CommitStatus, CreateStatus, Forge, SubmitOptions, SubmitStatus, STYLE_PUSHED}; +use crate::{ + CommitStatus, CreateStatus, Forge, SubmitOptions, SubmitStatus, UpdateStatus, STYLE_SUCCESS, +}; /// Wrapper around the Phabricator "ID" type. (This is *not* a PHID, just a /// regular ID). @@ -373,10 +375,18 @@ impl Forge for PhabricatorForge<'_> { TestCommand::Args(args.into_iter().map(ToString::to_string).collect()) } else { TestCommand::String( - r#"git commit --amend --message "$(git show --no-patch --format=%B HEAD) + r#" +echo "Creating $(git rev-parse HEAD)" +if [[ $(git show) == *BROKEN* ]]; then + echo 'Failed to create because the message contained the string BROKEN' + exit 1 +else + echo 'Amending commit message' + git commit --amend --message "$(git show --no-patch --format=%B HEAD) Differential Revision: https://phabricator.example.com/D000$(git rev-list --count HEAD) - " + " +fi "# .to_string(), ) @@ -394,7 +404,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun &ResolvedTestOptions { command, execution_strategy: *execution_strategy, - search_strategy: None, + search_strategy: Some(TestSearchStrategy::Linear), is_dry_run: false, use_cache: false, is_interactive: false, @@ -414,6 +424,9 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun testing_aborted_error, } = test_results; if let Some(testing_aborted_error) = testing_aborted_error { + // FIXME: we may still want to process any commits that succeeded; + // instead, we should probably update `test_outputs` in place and + // continue. let TestingAbortedError { commit_oid, exit_code, @@ -429,10 +442,23 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun return Ok(Err(ExitCode(1))); } - let rebase_plan = { + enum ErrorReason { + NotTested, + TestFailed, + } + let (maybe_rebase_plan, error_commits) = { let mut builder = RebasePlanBuilder::new(self.dag, permissions); - for (commit_oid, test_output) in test_outputs { - let head_commit_oid = match test_output.test_status { + let mut error_commits = HashMap::new(); + for commit_oid in commit_oids.iter().copied() { + let test_output = match test_outputs.get(&commit_oid) { + Some(test_output) => test_output, + None => { + // Testing was aborted due to a previous commit failing. + error_commits.insert(commit_oid, ErrorReason::NotTested); + continue; + } + }; + match test_output.test_status { TestStatus::CheckoutFailed | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal @@ -441,8 +467,8 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun | TestStatus::Indeterminate { .. } | TestStatus::Abort { .. } | TestStatus::Failed { .. } => { - self.render_failed_test(commit_oid, &test_output)?; - return Ok(Err(ExitCode(1))); + self.render_failed_test(commit_oid, test_output)?; + error_commits.insert(commit_oid, ErrorReason::TestFailed); } TestStatus::Passed { cached: _, @@ -452,88 +478,114 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun snapshot_tree_oid: _, }, interactive: _, - } => head_commit_oid, - }; - - let commit = self.repo.find_commit_or_fail(commit_oid)?; - builder.move_subtree(commit.get_oid(), commit.get_parent_oids())?; - builder.replace_commit(commit.get_oid(), head_commit_oid.unwrap_or(commit_oid))?; + } => { + let commit = self.repo.find_commit_or_fail(commit_oid)?; + builder.move_subtree(commit.get_oid(), commit.get_parent_oids())?; + builder.replace_commit( + commit.get_oid(), + head_commit_oid.unwrap_or(commit_oid), + )?; + } + } } let pool = ThreadPoolBuilder::new().build()?; let repo_pool = RepoResource::new_pool(self.repo)?; - match builder.build(self.effects, &pool, &repo_pool)? { - Ok(Some(rebase_plan)) => rebase_plan, - Ok(None) => return Ok(Ok(Default::default())), + let maybe_rebase_plan = match builder.build(self.effects, &pool, &repo_pool)? { + Ok(maybe_rebase_plan) => maybe_rebase_plan, Err(err) => { err.describe(self.effects, self.repo, self.dag)?; return Ok(Err(ExitCode(1))); } - } + }; + (maybe_rebase_plan, error_commits) }; - let rewritten_oids = match execute_rebase_plan( - self.effects, - self.git_run_info, - self.repo, - self.event_log_db, - &rebase_plan, - &execute_options, - )? { - ExecuteRebasePlanResult::Succeeded { - rewritten_oids: Some(rewritten_oids), - } => rewritten_oids, - ExecuteRebasePlanResult::Succeeded { - rewritten_oids: None, - } => { - warn!("No rewritten commit OIDs were produced by rebase plan execution"); - Default::default() - } - ExecuteRebasePlanResult::DeclinedToMerge { - failed_merge_info: _, - } => { - writeln!( - self.effects.get_error_stream(), - "BUG: Merge failed, but rewording shouldn't cause any merge failures." - )?; - return Ok(Err(ExitCode(1))); - } - ExecuteRebasePlanResult::Failed { exit_code } => { - return Ok(Err(exit_code)); - } + let rewritten_oids = match maybe_rebase_plan { + None => Default::default(), + Some(rebase_plan) => match execute_rebase_plan( + self.effects, + self.git_run_info, + self.repo, + self.event_log_db, + &rebase_plan, + &execute_options, + )? { + ExecuteRebasePlanResult::Succeeded { + rewritten_oids: Some(rewritten_oids), + } => rewritten_oids, + ExecuteRebasePlanResult::Succeeded { + rewritten_oids: None, + } => { + warn!("No rewritten commit OIDs were produced by rebase plan execution"); + Default::default() + } + ExecuteRebasePlanResult::DeclinedToMerge { + failed_merge_info: _, + } => { + writeln!( + self.effects.get_error_stream(), + "BUG: Merge failed, but rewording shouldn't cause any merge failures." + )?; + return Ok(Err(ExitCode(1))); + } + ExecuteRebasePlanResult::Failed { exit_code } => { + return Ok(Err(exit_code)); + } + }, }; let mut create_statuses = HashMap::new(); for commit_oid in commit_oids { + if let Some(error_reason) = error_commits.get(&commit_oid) { + create_statuses.insert( + commit_oid, + CreateStatus::Err { + message: match error_reason { + ErrorReason::TestFailed => "arc diff failed", + ErrorReason::NotTested => "dependency could not be submitted", + } + .to_string(), + }, + ); + continue; + } + let final_commit_oid = match rewritten_oids.get(&commit_oid) { Some(MaybeZeroOid::NonZero(commit_oid)) => *commit_oid, Some(MaybeZeroOid::Zero) => { warn!(?commit_oid, "Commit was rewritten to the zero OID",); commit_oid } - None => commit_oid, + None => { + create_statuses.insert( + commit_oid, + CreateStatus::Err { + message: "could not find rewritten commit".to_string(), + }, + ); + continue; + } }; let local_branch_name = { match self.get_revision_id(final_commit_oid)? { Some(Id(id)) => format!("D{id}"), None => { - writeln!( - self.effects.get_output_stream(), - "Failed to upload (link to newly-created revision not found in commit message): {}", - self.effects.get_glyphs().render( - self.repo.friendly_describe_commit_from_oid( - self.effects.get_glyphs(), - final_commit_oid - )? - )?, - )?; - return Ok(Err(ExitCode(1))); + create_statuses.insert( + commit_oid, + CreateStatus::Err { + message: + "could not find link to newly-created revision in updated commit message" + .to_string(), + }, + ); + continue; } } }; create_statuses.insert( commit_oid, - CreateStatus { + CreateStatus::Created { final_commit_oid, local_commit_name: local_branch_name, }, @@ -542,12 +594,12 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun let final_commit_oids: CommitSet = create_statuses .values() - .map(|create_status| { - let CreateStatus { + .filter_map(|create_status| match create_status { + CreateStatus::Created { final_commit_oid, local_commit_name: _, - } = create_status; - *final_commit_oid + } => Some(*final_commit_oid), + CreateStatus::Skipped { .. } | CreateStatus::Err { .. } => None, }) .collect(); self.dag.sync_from_oids( @@ -569,7 +621,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun &mut self, commits: HashMap, options: &SubmitOptions, - ) -> EyreExitOr<()> { + ) -> EyreExitOr> { let SubmitOptions { create: _, draft: _, @@ -702,12 +754,27 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun try_exit_code!(self.update_dependencies( &success_commits - .into_iter() + .iter() .map(|(commit_oid, _test_output)| commit_oid) + .copied() .collect(), &CommitSet::empty() )?); - Ok(Ok(())) + + let mut update_statuses = HashMap::new(); + for (commit_oid, _test_output) in success_commits.into_iter() { + let revision_id = self.get_revision_id(commit_oid)?; + let local_commit_name = match revision_id { + Some(Id(id)) => format!("D{id}"), + None => "".to_string(), + }; + update_statuses.insert(commit_oid, UpdateStatus::Updated { local_commit_name }); + } + for (commit_oid, _test_output) in failure_commits.into_iter() { + let local_commit_name = commit_oid.to_string(); + update_statuses.insert(commit_oid, UpdateStatus::Updated { local_commit_name }); + } + Ok(Ok(update_statuses)) } } @@ -941,7 +1008,7 @@ impl PhabricatorForge<'_> { fn render_id(id: &Id) -> StyledString { StyledStringBuilder::new() - .append_styled(id.to_string(), *STYLE_PUSHED) + .append_styled(id.to_string(), *STYLE_SUCCESS) .build() } diff --git a/git-branchless-submit/tests/test_github_forge.rs b/git-branchless-submit/tests/test_github_forge.rs index 2187c2b19..3c8af84a3 100644 --- a/git-branchless-submit/tests/test_github_forge.rs +++ b/git-branchless-submit/tests/test_github_forge.rs @@ -104,7 +104,8 @@ fn test_github_forge_reorder_commits() -> eyre::Result<()> { branchless: running command: push --force-with-lease origin mock-github-username/create-test1-txt Updating pull request (base branch, title, body) for commit 96d1c37 create test2.txt branchless: running command: push --force-with-lease origin mock-github-username/create-test2-txt - Submitted 2 commits: mock-github-username/create-test1-txt, mock-github-username/create-test2-txt + Submitted 62fc20d create test1.txt (as mock-github-username/create-test1-txt) + Submitted 96d1c37 create test2.txt (as mock-github-username/create-test2-txt) "###); } { @@ -175,7 +176,8 @@ fn test_github_forge_reorder_commits() -> eyre::Result<()> { branchless: running command: push --force-with-lease origin mock-github-username/create-test2-txt Updating pull request (commit, base branch, title, body) for commit 0770943 create test1.txt branchless: running command: push --force-with-lease origin mock-github-username/create-test1-txt - Updated 2 commits: mock-github-username/create-test1-txt, mock-github-username/create-test2-txt + Updated fe65c1f create test2.txt (as mock-github-username/create-test2-txt) + Updated 0770943 create test1.txt (as mock-github-username/create-test1-txt) "###); } { @@ -267,7 +269,8 @@ fn test_github_forge_mock_client_closes_pull_requests() -> eyre::Result<()> { branchless: running command: push --force-with-lease origin mock-github-username/create-test1-txt Updating pull request (base branch, title, body) for commit 96d1c37 create test2.txt branchless: running command: push --force-with-lease origin mock-github-username/create-test2-txt - Submitted 2 commits: mock-github-username/create-test1-txt, mock-github-username/create-test2-txt + Submitted 62fc20d create test1.txt (as mock-github-username/create-test1-txt) + Submitted 96d1c37 create test2.txt (as mock-github-username/create-test2-txt) "###); } { @@ -359,7 +362,7 @@ fn test_github_forge_mock_client_closes_pull_requests() -> eyre::Result<()> { insta::assert_snapshot!(stdout, @r###" Updating pull request (commit, base branch, title, body) for commit fa46633 create test2.txt branchless: running command: push --force-with-lease origin mock-github-username/create-test2-txt - Updated 1 commit: mock-github-username/create-test2-txt + Updated fa46633 create test2.txt (as mock-github-username/create-test2-txt) "###); } { @@ -458,7 +461,7 @@ fn test_github_forge_no_include_unsubmitted_commits_in_stack() -> eyre::Result<( branch 'mock-github-username/create-test1-txt' set up to track 'origin/mock-github-username/create-test1-txt'. Updating pull request (title, body) for commit 62fc20d create test1.txt branchless: running command: push --force-with-lease origin mock-github-username/create-test1-txt - Submitted 1 commit: mock-github-username/create-test1-txt + Submitted 62fc20d create test1.txt (as mock-github-username/create-test1-txt) "###); } { @@ -548,7 +551,7 @@ fn test_github_forge_multiple_commits_in_pull_request() -> eyre::Result<()> { branch 'mock-github-username/create-test3-txt' set up to track 'origin/mock-github-username/create-test3-txt'. Updating pull request (title, body) for commit 70deb1e create test3.txt branchless: running command: push --force-with-lease origin mock-github-username/create-test3-txt - Submitted 1 commit: mock-github-username/create-test3-txt + Submitted 70deb1e create test3.txt (as mock-github-username/create-test3-txt) "###); } { diff --git a/git-branchless-submit/tests/test_phabricator_forge.rs b/git-branchless-submit/tests/test_phabricator_forge.rs index 743f9f175..48da6e3b4 100644 --- a/git-branchless-submit/tests/test_phabricator_forge.rs +++ b/git-branchless-submit/tests/test_phabricator_forge.rs @@ -61,6 +61,7 @@ fn test_submit_phabricator_strategy_working_copy() -> eyre::Result<()> { Calling Git for on-disk rebase... branchless: running command: rebase --continue Using command execution strategy: working-copy + Using test search strategy: linear branchless: running command: rebase --abort Attempting rebase in-memory... [1/2] Committed as: 55af3db create test1.txt @@ -75,7 +76,8 @@ fn test_submit_phabricator_strategy_working_copy() -> eyre::Result<()> { branchless: running command: rebase --continue Using command execution strategy: working-copy branchless: running command: rebase --abort - Submitted 2 commits: D0002, D0003 + Submitted 62fc20d create test1.txt (as D0002) + Submitted 96d1c37 create test2.txt (as D0003) "###); } @@ -140,6 +142,7 @@ fn test_submit_phabricator_strategy_worktree() -> eyre::Result<()> { "###); insta::assert_snapshot!(stdout, @r###" Using command execution strategy: worktree + Using test search strategy: linear Attempting rebase in-memory... [1/2] Committed as: 55af3db create test1.txt [2/2] Committed as: ccb7fd5 create test2.txt @@ -149,7 +152,8 @@ fn test_submit_phabricator_strategy_worktree() -> eyre::Result<()> { Setting D0002 as stack root (no dependencies) Stacking D0003 on top of D0002 Using command execution strategy: worktree - Submitted 2 commits: D0002, D0003 + Submitted 62fc20d create test1.txt (as D0002) + Submitted 96d1c37 create test2.txt (as D0003) "###); } @@ -190,6 +194,7 @@ fn test_submit_phabricator_update() -> eyre::Result<()> { Calling Git for on-disk rebase... branchless: running command: rebase --continue Using command execution strategy: working-copy + Using test search strategy: linear branchless: running command: rebase --abort Attempting rebase in-memory... [1/2] Committed as: 55af3db create test1.txt @@ -204,7 +209,8 @@ fn test_submit_phabricator_update() -> eyre::Result<()> { branchless: running command: rebase --continue Using command execution strategy: working-copy branchless: running command: rebase --abort - Submitted 2 commits: D0002, D0003 + Submitted 62fc20d create test1.txt (as D0002) + Submitted 96d1c37 create test2.txt (as D0003) "###); } @@ -225,6 +231,86 @@ fn test_submit_phabricator_update() -> eyre::Result<()> { branchless: running command: rebase --abort Setting D0002 as stack root (no dependencies) Stacking D0003 on top of D0002 + Updated 55af3db create test1.txt (as D0002) + Updated ccb7fd5 create test2.txt (as D0003) + "###); + } + + Ok(()) +} + +#[test] +fn test_submit_phabricator_failure_commit() -> eyre::Result<()> { + let git = make_git()?; + git.init_repo()?; + + git.detach_head()?; + git.commit_file("test1", 1)?; + git.commit_file_with_contents_and_message("test2", 2, "test2 contents\n", "BROKEN:")?; + git.commit_file("test3", 3)?; + + { + let (stdout, stderr) = git.branchless_with_options( + "submit", + &["--create", "--forge", "phabricator"], + &GitRunOptions { + env: mock_env(&git), + expected_exit_code: 1, + ..Default::default() + }, + )?; + insta::assert_snapshot!(stderr, @r###" + Stopped at e9d3664 (create test3.txt) + branchless: processing 1 update: ref HEAD + branchless: creating working copy snapshot + Previous HEAD position was e9d3664 create test3.txt + branchless: processing 1 update: ref HEAD + HEAD is now at d5bb8b5 create test3.txt + branchless: processing checkout + Stopped at d5bb8b5 (create test3.txt) + branchless: processing 1 update: ref HEAD + "###); + insta::assert_snapshot!(stdout, @r###" + branchless: running command: diff --quiet + Calling Git for on-disk rebase... + branchless: running command: rebase --continue + Using command execution strategy: working-copy + Using test search strategy: linear + branchless: running command: rebase --abort + Failed (exit code 1): 5b9de4b BROKEN: test2.txt + Stdout: + Creating 5b9de4b86a0af34784df27834d677cc7000f092b + Failed to create because the message contained the string BROKEN + Stderr: + Attempting rebase in-memory... + [1/3] Committed as: 55af3db create test1.txt + [2/3] Committed as: 0741b57 BROKEN: test2.txt + [3/3] Committed as: d5bb8b5 create test3.txt + branchless: processing 3 rewritten commits + branchless: running command: checkout d5bb8b5754d76207bb9ed8551055f8f28beb1332 + In-memory rebase succeeded. + Setting D0002 as stack root (no dependencies) + branchless: running command: diff --quiet + Calling Git for on-disk rebase... + branchless: running command: rebase --continue + Using command execution strategy: working-copy + branchless: running command: rebase --abort + Submitted 62fc20d create test1.txt (as D0002) + Failed to create 5b9de4b BROKEN: test2.txt (arc diff failed) + Failed to create e9d3664 create test3.txt (dependency could not be submitted) + "###); + } + + { + let stdout = git.smartlog()?; + insta::assert_snapshot!(stdout, @r###" + O f777ecc (master) create initial.txt + | + o 55af3db D0002 create test1.txt + | + o 0741b57 BROKEN: test2.txt + | + @ d5bb8b5 create test3.txt "###); } diff --git a/git-branchless-submit/tests/test_submit.rs b/git-branchless-submit/tests/test_submit.rs index ebf3f0f82..3fdbc6542 100644 --- a/git-branchless-submit/tests/test_submit.rs +++ b/git-branchless-submit/tests/test_submit.rs @@ -58,9 +58,8 @@ fn test_submit() -> eyre::Result<()> { let (stdout, stderr) = cloned_repo.run(&["submit"])?; insta::assert_snapshot!(stderr, @""); insta::assert_snapshot!(stdout, @r###" - Skipped 2 commits (not yet on remote): bar, qux - These commits were skipped because they were not already associated with a remote - repository. To submit them, retry this operation with the --create option. + Skipped f57e36f create test4.txt (pass --create to submit) + Skipped 20230db create test5.txt (pass --create to submit) "###); } @@ -69,9 +68,8 @@ fn test_submit() -> eyre::Result<()> { let (stdout, stderr) = cloned_repo.run(&["submit", "bar+qux"])?; insta::assert_snapshot!(stderr, @""); insta::assert_snapshot!(stdout, @r###" - Skipped 2 commits (not yet on remote): bar, qux - These commits were skipped because they were not already associated with a remote - repository. To submit them, retry this operation with the --create option. + Skipped f57e36f create test4.txt (pass --create to submit) + Skipped 20230db create test5.txt (pass --create to submit) "###); } @@ -80,9 +78,8 @@ fn test_submit() -> eyre::Result<()> { let (stdout, stderr) = cloned_repo.run(&["submit", "bar", "qux"])?; insta::assert_snapshot!(stderr, @""); insta::assert_snapshot!(stdout, @r###" - Skipped 2 commits (not yet on remote): bar, qux - These commits were skipped because they were not already associated with a remote - repository. To submit them, retry this operation with the --create option. + Skipped f57e36f create test4.txt (pass --create to submit) + Skipped 20230db create test5.txt (pass --create to submit) "###); } @@ -90,9 +87,8 @@ fn test_submit() -> eyre::Result<()> { let (stdout, stderr) = cloned_repo.run(&["submit", "--dry-run"])?; insta::assert_snapshot!(stderr, @""); insta::assert_snapshot!(stdout, @r###" - Would skip 2 commits (not yet on remote): bar, qux - These commits would be skipped because they are not already associated with a remote - repository. To submit them, retry this operation with the --create option. + Would skip f57e36f create test4.txt (pass --create to submit) + Would skip 20230db create test5.txt (pass --create to submit) "###); } @@ -100,7 +96,8 @@ fn test_submit() -> eyre::Result<()> { let (stdout, stderr) = cloned_repo.run(&["submit", "--create", "--dry-run"])?; insta::assert_snapshot!(stderr, @""); insta::assert_snapshot!(stdout, @r###" - Would submit 2 commits: bar, qux + Would submit f57e36f create test4.txt (as bar) + Would submit 20230db create test5.txt (as qux) "###); } @@ -120,7 +117,8 @@ fn test_submit() -> eyre::Result<()> { branchless: running command: push --set-upstream origin bar qux branch 'bar' set up to track 'origin/bar'. branch 'qux' set up to track 'origin/qux'. - Submitted 2 commits: bar, qux + Submitted f57e36f create test4.txt (as bar) + Submitted 20230db create test5.txt (as qux) "###); } @@ -150,8 +148,8 @@ fn test_submit() -> eyre::Result<()> { insta::assert_snapshot!(stdout, @r###" branchless: running command: fetch origin refs/heads/bar refs/heads/qux branchless: running command: push --force-with-lease origin qux - Updated 1 commit: qux - Skipped 1 commit (already up-to-date): bar + Skipped f57e36f create test4.txt (already up-to-date) + Updated bae8307 updated message (as qux) "###); } @@ -166,7 +164,8 @@ fn test_submit() -> eyre::Result<()> { "###); insta::assert_snapshot!(stdout, @r###" branchless: running command: fetch origin refs/heads/bar refs/heads/qux - Skipped 2 commits (already up-to-date): bar, qux + Skipped f57e36f create test4.txt (already up-to-date) + Skipped bae8307 updated message (already up-to-date) "###); } @@ -323,7 +322,7 @@ fn test_submit_up_to_date_branch() -> eyre::Result<()> { insta::assert_snapshot!(stdout, @r###" branchless: running command: push --set-upstream origin feature branch 'feature' set up to track 'origin/feature'. - Submitted 1 commit: feature + Submitted 70deb1e create test3.txt (as feature) "###); } @@ -337,7 +336,7 @@ fn test_submit_up_to_date_branch() -> eyre::Result<()> { "###); insta::assert_snapshot!(stdout, @r###" branchless: running command: fetch origin refs/heads/feature - Skipped 1 commit (already up-to-date): feature + Skipped 70deb1e create test3.txt (already up-to-date) "###); } diff --git a/git-branchless-test/src/lib.rs b/git-branchless-test/src/lib.rs index a86d3d89d..48efa39ba 100644 --- a/git-branchless-test/src/lib.rs +++ b/git-branchless-test/src/lib.rs @@ -1189,13 +1189,19 @@ impl<'a> BasicSourceControlGraph for SearchGraph<'a> { #[derive(Debug)] pub struct TestResults { /// If a search strategy was provided, the calculated bounds on the input - /// commit set. + /// commit set. If no strategy was provided, then the bounds will be empty. pub search_bounds: search::Bounds, /// The test output for each commit. + /// + /// Note that not every input commit may have corresponding test output in + /// this map if testing exited early. For example, if + /// [`TestSearchStrategy::Linear`] was provided and testing exited early, + /// then commits after the first failing commit won't appear in this map. pub test_outputs: IndexMap, - /// If testing was aborted, the corresponding error. + /// If testing was aborted, the corresponding error. This happens when a + /// test returns the special abort exit code [`TEST_ABORT_EXIT_CODE`]. pub testing_aborted_error: Option, }