From 70c8b31d184fb9ec98bb3120123f635e20e4e356 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Sun, 16 Jun 2024 11:23:46 -0700 Subject: [PATCH 1/4] build: add `rust-toolchain.toml` It seems easier for contributors to get started if the compiler version is already configured; plus, this will presumably make it so that I don't have to set the Rust version with `rustup override` whenever I create a new worktree. --- rust-toolchain.toml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 rust-toolchain.toml diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 000000000..94ed87190 --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,4 @@ +[toolchain] +# Current minimum-supported Rust version +channel = "1.74" +profile = "default" From 847c8532f2450397f5c64a2f1c37f3e6368ac269 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Sun, 30 Jun 2024 13:40:49 -0700 Subject: [PATCH 2/4] refactor(core): re-export formatting symbols from `cursive_core` This should have been done from the beginning, since we operate on these symbols in several places for formatting. This would also help a migration away from `cursive`, since now only two modules have direct dependencies on `cursive`/`cursive_core`: - git-branchless-lib for general output formatting. - git-branchless-undo for the interactive undo UI. --- Cargo.lock | 10 +--------- git-branchless-invoke/Cargo.toml | 1 - git-branchless-invoke/src/lib.rs | 4 +--- git-branchless-lib/Cargo.toml | 2 +- git-branchless-lib/src/core/check_out.rs | 3 +-- git-branchless-lib/src/core/config.rs | 4 +--- git-branchless-lib/src/core/formatting.rs | 7 +++---- git-branchless-lib/src/core/node_descriptors.rs | 12 +++++------- git-branchless-lib/src/git/object.rs | 4 +--- git-branchless-lib/src/git/repo.rs | 4 +--- git-branchless-navigation/Cargo.toml | 1 - git-branchless-navigation/src/lib.rs | 5 +---- git-branchless-record/Cargo.toml | 2 -- git-branchless-smartlog/Cargo.toml | 1 - git-branchless-smartlog/src/lib.rs | 7 +++---- git-branchless-submit/Cargo.toml | 1 - git-branchless-submit/src/github.rs | 3 +-- git-branchless-submit/src/lib.rs | 3 +-- git-branchless-submit/src/phabricator.rs | 7 ++++--- git-branchless-test/Cargo.toml | 1 - git-branchless-test/src/lib.rs | 7 ++++--- git-branchless-undo/src/lib.rs | 17 +++++++++-------- git-branchless/Cargo.toml | 1 - git-branchless/src/commands/snapshot.rs | 3 +-- git-branchless/src/commands/sync.rs | 3 +-- git-branchless/tests/test_undo.rs | 5 +---- 26 files changed, 41 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c43ad561b..b8659a171 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1401,7 +1401,6 @@ dependencies = [ "bugreport", "color-eyre", "console", - "cursive_core", "esl01-dag", "eyre", "fslock", @@ -1475,7 +1474,6 @@ version = "0.9.0" dependencies = [ "clap 4.5.8", "color-eyre", - "cursive_core", "eyre", "git-branchless-lib", "git-branchless-opts", @@ -1501,7 +1499,7 @@ dependencies = [ "concolor", "console", "criterion", - "cursive", + "cursive_core", "esl01-dag", "eyre", "futures", @@ -1546,7 +1544,6 @@ dependencies = [ name = "git-branchless-navigation" version = "0.9.0" dependencies = [ - "cursive", "esl01-dag", "eyre", "git-branchless-lib", @@ -1588,8 +1585,6 @@ dependencies = [ name = "git-branchless-record" version = "0.9.0" dependencies = [ - "cursive", - "cursive_buffered_backend", "esl01-dag", "eyre", "git-branchless-invoke", @@ -1651,7 +1646,6 @@ dependencies = [ name = "git-branchless-smartlog" version = "0.9.0" dependencies = [ - "cursive_core", "esl01-dag", "eyre", "git-branchless-invoke", @@ -1667,7 +1661,6 @@ name = "git-branchless-submit" version = "0.9.0" dependencies = [ "clap 4.5.8", - "cursive_core", "esl01-dag", "eyre", "git-branchless-invoke", @@ -1696,7 +1689,6 @@ dependencies = [ "bstr", "clap 4.5.8", "crossbeam", - "cursive", "esl01-dag", "eyre", "fslock", diff --git a/git-branchless-invoke/Cargo.toml b/git-branchless-invoke/Cargo.toml index d73cf9959..49a9ece53 100644 --- a/git-branchless-invoke/Cargo.toml +++ b/git-branchless-invoke/Cargo.toml @@ -11,7 +11,6 @@ version = "0.9.0" [dependencies] clap = { workspace = true, features = ["derive"] } color-eyre = { workspace = true } -cursive_core = { workspace = true } eyre = { workspace = true } git-branchless-opts = { workspace = true } git2 = { workspace = true } diff --git a/git-branchless-invoke/src/lib.rs b/git-branchless-invoke/src/lib.rs index eee43ffaa..532087fb4 100644 --- a/git-branchless-invoke/src/lib.rs +++ b/git-branchless-invoke/src/lib.rs @@ -21,13 +21,11 @@ use std::path::PathBuf; use std::time::SystemTime; use clap::{CommandFactory, FromArgMatches, Parser}; -use cursive_core::theme::BaseColor; -use cursive_core::utils::markup::StyledString; use eyre::Context; use git_branchless_opts::{ColorSetting, GlobalArgs}; use lib::core::config::env_vars::{get_git_exec_path, get_path_to_git}; use lib::core::effects::Effects; -use lib::core::formatting::Glyphs; +use lib::core::formatting::{BaseColor, Glyphs, StyledString}; use lib::git::GitRunInfo; use lib::git::{Repo, RepoError}; use lib::util::{ExitCode, EyreExitOr}; diff --git a/git-branchless-lib/Cargo.toml b/git-branchless-lib/Cargo.toml index bcbd15f82..2e7e2af5b 100644 --- a/git-branchless-lib/Cargo.toml +++ b/git-branchless-lib/Cargo.toml @@ -51,7 +51,7 @@ chrono = { workspace = true } color-eyre = { workspace = true } concolor = { workspace = true } console = { workspace = true } -cursive = { workspace = true } +cursive_core = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } futures = { workspace = true } diff --git a/git-branchless-lib/src/core/check_out.rs b/git-branchless-lib/src/core/check_out.rs index 546a09828..0d4dca138 100644 --- a/git-branchless-lib/src/core/check_out.rs +++ b/git-branchless-lib/src/core/check_out.rs @@ -4,13 +4,12 @@ use std::ffi::{OsStr, OsString}; use std::fmt::Write; use std::time::{SystemTime, UNIX_EPOCH}; -use cursive::theme::BaseColor; -use cursive::utils::markup::StyledString; use eyre::Context; use itertools::Itertools; use tracing::instrument; use crate::core::config::get_auto_switch_branches; +use crate::core::formatting::{BaseColor, StyledString}; use crate::git::{ update_index, CategorizedReferenceName, GitRunInfo, MaybeZeroOid, NonZeroOid, ReferenceName, Repo, Stage, UpdateIndexCommand, WorkingCopySnapshot, diff --git a/git-branchless-lib/src/core/config.rs b/git-branchless-lib/src/core/config.rs index 248525a36..e6a10d172 100644 --- a/git-branchless-lib/src/core/config.rs +++ b/git-branchless-lib/src/core/config.rs @@ -4,12 +4,10 @@ use std::ffi::OsString; use std::fmt::Write; use std::path::PathBuf; -use cursive::theme::{BaseColor, Effect, Style}; -use cursive::utils::markup::StyledString; use eyre::Context; use tracing::{instrument, warn}; -use crate::core::formatting::StyledStringBuilder; +use crate::core::formatting::{BaseColor, Effect, Style, StyledString, StyledStringBuilder}; use crate::git::{ConfigRead, GitRunInfo, GitRunOpts, Repo}; use super::effects::Effects; diff --git a/git-branchless-lib/src/core/formatting.rs b/git-branchless-lib/src/core/formatting.rs index a228a0975..506065b59 100644 --- a/git-branchless-lib/src/core/formatting.rs +++ b/git-branchless-lib/src/core/formatting.rs @@ -6,9 +6,9 @@ use std::fmt::Display; -use cursive::theme::{Effect, Style}; -use cursive::utils::markup::StyledString; -use cursive::utils::span::Span; +pub use cursive_core::theme::{BaseColor, Color, ColorType, Effect, Style}; +pub use cursive_core::utils::markup::StyledString; +pub use cursive_core::utils::span::Span; /// Pluralize a quantity, as appropriate. Example: /// @@ -369,7 +369,6 @@ fn render_style_as_ansi(content: &str, style: Style) -> eyre::Result { let Style { effects, color } = style; let output = { use console::style; - use cursive::theme::{BaseColor, Color, ColorType}; let output = content.to_string(); match color.front { ColorType::Palette(_) => { diff --git a/git-branchless-lib/src/core/node_descriptors.rs b/git-branchless-lib/src/core/node_descriptors.rs index 3c4cb9762..5ba64f769 100644 --- a/git-branchless-lib/src/core/node_descriptors.rs +++ b/git-branchless-lib/src/core/node_descriptors.rs @@ -8,8 +8,6 @@ use std::sync::{Arc, Mutex}; use std::time::SystemTime; use bstr::{ByteSlice, ByteVec}; -use cursive::theme::BaseColor; -use cursive::utils::markup::StyledString; use lazy_static::lazy_static; use regex::Regex; use tracing::instrument; @@ -18,15 +16,15 @@ use crate::core::config::{ get_commit_descriptors_branches, get_commit_descriptors_differential_revision, get_commit_descriptors_relative_time, }; +use crate::core::eventlog::{Event, EventCursor, EventReplayer}; +use crate::core::formatting::Glyphs; +use crate::core::formatting::{BaseColor, StyledString, StyledStringBuilder}; +use crate::core::repo_ext::RepoReferencesSnapshot; +use crate::core::rewrite::find_rewrite_target; use crate::git::{ CategorizedReferenceName, Commit, NonZeroOid, ReferenceName, Repo, ResolvedReferenceInfo, }; -use super::eventlog::{Event, EventCursor, EventReplayer}; -use super::formatting::{Glyphs, StyledStringBuilder}; -use super::repo_ext::RepoReferencesSnapshot; -use super::rewrite::find_rewrite_target; - /// An object which can be rendered in the smartlog. #[derive(Clone, Debug)] pub enum NodeObject<'repo> { diff --git a/git-branchless-lib/src/git/object.rs b/git-branchless-lib/src/git/object.rs index 0bd2fe010..ea9caba81 100644 --- a/git-branchless-lib/src/git/object.rs +++ b/git-branchless-lib/src/git/object.rs @@ -1,12 +1,10 @@ use std::path::Path; use bstr::{BString, ByteSlice}; -use cursive::theme::BaseColor; -use cursive::utils::markup::StyledString; use git2::message_trailers_bytes; use tracing::instrument; -use crate::core::formatting::{Glyphs, StyledStringBuilder}; +use crate::core::formatting::{BaseColor, Glyphs, StyledString, StyledStringBuilder}; use crate::core::node_descriptors::{ render_node_descriptors, CommitMessageDescriptor, CommitOidDescriptor, NodeObject, Redactor, }; diff --git a/git-branchless-lib/src/git/repo.rs b/git-branchless-lib/src/git/repo.rs index cbbafb484..86c0036ec 100644 --- a/git-branchless-lib/src/git/repo.rs +++ b/git-branchless-lib/src/git/repo.rs @@ -20,8 +20,6 @@ use std::{io, time}; use bstr::ByteVec; use chrono::NaiveDateTime; -use cursive::theme::BaseColor; -use cursive::utils::markup::StyledString; use git2::DiffOptions; use itertools::Itertools; use thiserror::Error; @@ -29,7 +27,7 @@ use tracing::{instrument, warn}; use crate::core::effects::{Effects, OperationType}; use crate::core::eventlog::EventTransactionId; -use crate::core::formatting::Glyphs; +use crate::core::formatting::{BaseColor, Glyphs, StyledString}; use crate::git::config::{Config, ConfigRead}; use crate::git::object::Blob; use crate::git::oid::{make_non_zero_oid, MaybeZeroOid, NonZeroOid}; diff --git a/git-branchless-navigation/Cargo.toml b/git-branchless-navigation/Cargo.toml index 6ae4ebac0..8b0b5b67c 100644 --- a/git-branchless-navigation/Cargo.toml +++ b/git-branchless-navigation/Cargo.toml @@ -7,7 +7,6 @@ repository = "https://github.com/arxanas/git-branchless" version = "0.9.0" [dependencies] -cursive = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } git-branchless-opts = { workspace = true } diff --git a/git-branchless-navigation/src/lib.rs b/git-branchless-navigation/src/lib.rs index 8361ee730..e8b73b298 100644 --- a/git-branchless-navigation/src/lib.rs +++ b/git-branchless-navigation/src/lib.rs @@ -17,9 +17,6 @@ use std::ffi::OsString; use std::fmt::Write; use std::time::SystemTime; -use cursive::theme::BaseColor; -use cursive::utils::markup::StyledString; - use lib::core::check_out::{check_out_commit, CheckOutCommitOptions, CheckoutTarget}; use lib::core::repo_ext::RepoExt; use lib::util::{ExitCode, EyreExitOr}; @@ -32,7 +29,7 @@ use lib::core::config::get_next_interactive; use lib::core::dag::{sorted_commit_set, CommitSet, Dag}; use lib::core::effects::Effects; use lib::core::eventlog::{EventLogDb, EventReplayer}; -use lib::core::formatting::Pluralize; +use lib::core::formatting::{BaseColor, Pluralize, StyledString}; use lib::core::node_descriptors::{ BranchesDescriptor, CommitMessageDescriptor, CommitOidDescriptor, DifferentialRevisionDescriptor, NodeDescriptor, Redactor, RelativeTimeDescriptor, diff --git a/git-branchless-record/Cargo.toml b/git-branchless-record/Cargo.toml index 9b5be0ab9..a81b5db3d 100644 --- a/git-branchless-record/Cargo.toml +++ b/git-branchless-record/Cargo.toml @@ -7,8 +7,6 @@ repository = "https://github.com/arxanas/git-branchless" version = "0.9.0" [dependencies] -cursive = { version = "0.20.0", default-features = false, features = [ "crossterm-backend", ] } -cursive_buffered_backend = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } git-branchless-invoke = { workspace = true } diff --git a/git-branchless-smartlog/Cargo.toml b/git-branchless-smartlog/Cargo.toml index e74bc1778..795e06265 100644 --- a/git-branchless-smartlog/Cargo.toml +++ b/git-branchless-smartlog/Cargo.toml @@ -7,7 +7,6 @@ repository = "https://github.com/arxanas/git-branchless" version = "0.9.0" [dependencies] -cursive_core = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } git-branchless-invoke = { workspace = true } diff --git a/git-branchless-smartlog/src/lib.rs b/git-branchless-smartlog/src/lib.rs index e3b3bdc3f..646442f4f 100644 --- a/git-branchless-smartlog/src/lib.rs +++ b/git-branchless-smartlog/src/lib.rs @@ -377,14 +377,13 @@ mod render { use std::cmp::Ordering; use std::collections::HashSet; - use cursive_core::theme::{BaseColor, Effect}; - use cursive_core::utils::markup::StyledString; use tracing::instrument; use lib::core::dag::{CommitSet, Dag}; use lib::core::effects::Effects; - use lib::core::formatting::{set_effect, Pluralize}; - use lib::core::formatting::{Glyphs, StyledStringBuilder}; + use lib::core::formatting::{ + set_effect, BaseColor, Effect, Glyphs, Pluralize, StyledString, StyledStringBuilder, + }; use lib::core::node_descriptors::{render_node_descriptors, NodeDescriptor}; use lib::git::{NonZeroOid, Repo}; diff --git a/git-branchless-submit/Cargo.toml b/git-branchless-submit/Cargo.toml index fc1680c6f..de674cb0b 100644 --- a/git-branchless-submit/Cargo.toml +++ b/git-branchless-submit/Cargo.toml @@ -9,7 +9,6 @@ version = "0.9.0" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -cursive_core = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } git-branchless-invoke = { workspace = true } diff --git a/git-branchless-submit/src/github.rs b/git-branchless-submit/src/github.rs index 39eef9c06..5bb23dbe7 100644 --- a/git-branchless-submit/src/github.rs +++ b/git-branchless-submit/src/github.rs @@ -5,8 +5,6 @@ use std::env; use std::fmt::{Debug, Write}; use std::hash::Hash; -use cursive_core::theme::Effect; -use cursive_core::utils::markup::StyledString; use indexmap::IndexMap; use itertools::Itertools; use lib::core::config::get_main_branch_name; @@ -15,6 +13,7 @@ use lib::core::dag::Dag; use lib::core::effects::Effects; use lib::core::effects::OperationType; use lib::core::eventlog::EventLogDb; +use lib::core::formatting::{Effect, StyledString}; use lib::core::repo_ext::RepoExt; use lib::core::repo_ext::RepoReferencesSnapshot; use lib::git::CategorizedReferenceName; diff --git a/git-branchless-submit/src/lib.rs b/git-branchless-submit/src/lib.rs index 6762b3207..3d8dbf530 100644 --- a/git-branchless-submit/src/lib.rs +++ b/git-branchless-submit/src/lib.rs @@ -18,7 +18,6 @@ use std::fmt::{Debug, Write}; use std::time::SystemTime; use branch_forge::BranchForge; -use cursive_core::theme::{BaseColor, Effect, Style}; use git_branchless_invoke::CommandContext; use git_branchless_test::{RawTestOptions, ResolvedTestOptions, Verbosity}; use github::GithubForge; @@ -27,7 +26,7 @@ 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::formatting::{BaseColor, Effect, Pluralize, Style, StyledStringBuilder}; use lib::core::repo_ext::{RepoExt, RepoReferencesSnapshot}; use lib::git::{GitRunInfo, NonZeroOid, Repo}; use lib::try_exit_code; diff --git a/git-branchless-submit/src/phabricator.rs b/git-branchless-submit/src/phabricator.rs index b21fb3c89..bd4842a0c 100644 --- a/git-branchless-submit/src/phabricator.rs +++ b/git-branchless-submit/src/phabricator.rs @@ -8,8 +8,6 @@ use std::process::{Command, Stdio}; use std::str::FromStr; use std::time::SystemTime; -use cursive_core::theme::Effect; -use cursive_core::utils::markup::StyledString; use git_branchless_opts::Revset; use git_branchless_test::{ run_tests, FixInfo, ResolvedTestOptions, TestOutput, TestResults, TestStatus, @@ -21,7 +19,10 @@ use lib::core::check_out::CheckOutCommitOptions; use lib::core::dag::{CommitSet, Dag}; use lib::core::effects::{Effects, OperationType, WithProgress}; use lib::core::eventlog::EventLogDb; -use lib::core::formatting::StyledStringBuilder; +use lib::core::formatting::{ + Effect, StyledString, + StyledStringBuilder, +}; use lib::core::rewrite::{ execute_rebase_plan, BuildRebasePlanError, BuildRebasePlanOptions, ExecuteRebasePlanOptions, ExecuteRebasePlanResult, RebasePlanBuilder, RebasePlanPermissions, RepoResource, diff --git a/git-branchless-test/Cargo.toml b/git-branchless-test/Cargo.toml index 443a5ed6e..9cbb92aa4 100644 --- a/git-branchless-test/Cargo.toml +++ b/git-branchless-test/Cargo.toml @@ -10,7 +10,6 @@ version = "0.9.0" bstr = { workspace = true } clap = { workspace = true } crossbeam = { workspace = true } -cursive = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } fslock = { workspace = true } diff --git a/git-branchless-test/src/lib.rs b/git-branchless-test/src/lib.rs index a86d3d89d..edc473c2e 100644 --- a/git-branchless-test/src/lib.rs +++ b/git-branchless-test/src/lib.rs @@ -24,8 +24,6 @@ use std::time::SystemTime; use bstr::ByteSlice; use clap::ValueEnum; use crossbeam::channel::{Receiver, RecvError}; -use cursive::theme::{BaseColor, Effect, Style}; -use cursive::utils::markup::StyledString; use eyre::WrapErr; use fslock::LockFile; @@ -43,7 +41,10 @@ use lib::core::effects::{icons, Effects, OperationIcon, OperationType}; use lib::core::eventlog::{ EventLogDb, EventReplayer, EventTransactionId, BRANCHLESS_TRANSACTION_ID_ENV_VAR, }; -use lib::core::formatting::{Glyphs, Pluralize, StyledStringBuilder}; +use lib::core::formatting::{ + BaseColor, Effect, Glyphs, Pluralize, Style, StyledString, + StyledStringBuilder, +}; use lib::core::repo_ext::RepoExt; use lib::core::rewrite::{ execute_rebase_plan, BuildRebasePlanOptions, ExecuteRebasePlanOptions, ExecuteRebasePlanResult, diff --git a/git-branchless-undo/src/lib.rs b/git-branchless-undo/src/lib.rs index 4dc39dcf2..3dc76a4f0 100644 --- a/git-branchless-undo/src/lib.rs +++ b/git-branchless-undo/src/lib.rs @@ -21,31 +21,31 @@ use std::time::SystemTime; use cursive_core::event::Key; use cursive_core::traits::Resizable; -use cursive_core::utils::markup::StyledString; use cursive_core::views::{ Dialog, EditView, LinearLayout, OnEventView, Panel, ScrollView, TextView, }; use cursive_core::{Cursive, CursiveRunner}; use eyre::Context; -use lib::core::check_out::{check_out_commit, CheckOutCommitOptions, CheckoutTarget}; -use lib::core::repo_ext::RepoExt; -use lib::try_exit_code; -use lib::util::{ExitCode, EyreExitOr}; use tracing::instrument; -use crate::tui::{with_siv, SingletonView}; use git_branchless_revset::resolve_default_smartlog_commits; use git_branchless_smartlog::{make_smartlog_graph, render_graph}; +use lib::core::check_out::{check_out_commit, CheckOutCommitOptions, CheckoutTarget}; use lib::core::dag::{CommitSet, Dag}; use lib::core::effects::Effects; use lib::core::eventlog::{Event, EventCursor, EventLogDb, EventReplayer, EventTransactionId}; -use lib::core::formatting::{Glyphs, Pluralize, StyledStringBuilder}; +use lib::core::formatting::{Glyphs, Pluralize, StyledString, StyledStringBuilder}; use lib::core::node_descriptors::{ BranchesDescriptor, CommitMessageDescriptor, CommitOidDescriptor, DifferentialRevisionDescriptor, ObsolescenceExplanationDescriptor, Redactor, RelativeTimeDescriptor, }; +use lib::core::repo_ext::RepoExt; use lib::git::{CategorizedReferenceName, GitRunInfo, MaybeZeroOid, Repo, ResolvedReferenceInfo}; +use lib::try_exit_code; +use lib::util::{ExitCode, EyreExitOr}; + +use crate::tui::{with_siv, SingletonView}; fn render_cursor_smartlog( effects: &Effects, @@ -999,7 +999,8 @@ pub fn undo( pub mod testing { use std::io::Read; - use cursive_core::{Cursive, CursiveRunner}; + pub use cursive_core::event::Key; + pub use cursive_core::{Cursive, CursiveRunner}; use lib::core::dag::Dag; use lib::core::effects::Effects; diff --git a/git-branchless/Cargo.toml b/git-branchless/Cargo.toml index 7bf56e572..0b2f6a155 100644 --- a/git-branchless/Cargo.toml +++ b/git-branchless/Cargo.toml @@ -21,7 +21,6 @@ bstr = { workspace = true } bugreport = { workspace = true } color-eyre = { workspace = true } console = { workspace = true } -cursive_core = { workspace = true } eden_dag = { workspace = true } eyre = { workspace = true } fslock = { workspace = true } diff --git a/git-branchless/src/commands/snapshot.rs b/git-branchless/src/commands/snapshot.rs index 693f0f318..c6b7275a3 100644 --- a/git-branchless/src/commands/snapshot.rs +++ b/git-branchless/src/commands/snapshot.rs @@ -4,12 +4,11 @@ use std::fmt::Write; use std::time::SystemTime; -use cursive_core::theme::BaseColor; -use cursive_core::utils::markup::StyledString; use eyre::Context; use lib::core::check_out::{create_snapshot, restore_snapshot}; use lib::core::effects::Effects; use lib::core::eventlog::EventLogDb; +use lib::core::formatting::{BaseColor, StyledString}; use lib::git::{GitRunInfo, GitRunResult, NonZeroOid, Repo, WorkingCopySnapshot}; use lib::util::{ExitCode, EyreExitOr}; diff --git a/git-branchless/src/commands/sync.rs b/git-branchless/src/commands/sync.rs index 5106c1841..8360cae23 100644 --- a/git-branchless/src/commands/sync.rs +++ b/git-branchless/src/commands/sync.rs @@ -1,6 +1,5 @@ //! Implements the `git sync` command. -use cursive_core::theme::BaseColor; use lib::try_exit_code; use std::fmt::Write; use std::time::SystemTime; @@ -17,7 +16,7 @@ use lib::core::config::get_restack_preserve_timestamps; use lib::core::dag::{sorted_commit_set, union_all, CommitSet, Dag}; use lib::core::effects::{Effects, OperationType, WithProgress}; use lib::core::eventlog::{EventLogDb, EventReplayer}; -use lib::core::formatting::{Pluralize, StyledStringBuilder}; +use lib::core::formatting::{BaseColor, Pluralize, StyledStringBuilder}; use lib::core::rewrite::{ execute_rebase_plan, BuildRebasePlanError, BuildRebasePlanOptions, ExecuteRebasePlanOptions, ExecuteRebasePlanResult, FailedMergeInfo, RebasePlan, RebasePlanBuilder, RebasePlanPermissions, diff --git a/git-branchless/tests/test_undo.rs b/git-branchless/tests/test_undo.rs index 9bf46e7f1..031459b13 100644 --- a/git-branchless/tests/test_undo.rs +++ b/git-branchless/tests/test_undo.rs @@ -2,7 +2,7 @@ use std::mem::swap; use std::rc::Rc; use std::sync::{Arc, Mutex}; -use git_branchless_undo::testing::{select_past_event, undo_events}; +use git_branchless_undo::testing::{select_past_event, undo_events, Cursive, CursiveRunner, Key}; use git_branchless_undo::tui::testing::{ screen_to_string, CursiveTestingBackend, CursiveTestingEvent, }; @@ -13,9 +13,6 @@ use lib::core::formatting::Glyphs; use lib::core::repo_ext::RepoExt; use lib::git::{GitRunInfo, GitVersion, Repo}; use lib::testing::{make_git, trim_lines, Git, GitInitOptions, GitRunOptions}; - -use cursive_core::event::Key; -use cursive_core::{Cursive, CursiveRunner}; use lib::util::ExitCode; fn run_select_past_event( From 829a1219679c2d585f75e4e22ae08f78331d0a0a Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Sun, 30 Jun 2024 13:40:49 -0700 Subject: [PATCH 3/4] feat(submit:phabricator): do not abort entire process on failure Currently, `git submit` for Phabricator will abort the entire operation if any commit fails to be submitted. This means that if `arc diff` succeeds on one commit and then fails on its child, the entire operation is aborted. However, the first `arc diff` had side effects, so the user gets diffs uploaded to Phabricator that are not reflected locally. Instead, we should confirm any passing commits and abort after we get a failing commit. This commit updates the Phabricator forge to handle the error case better and not produce garbage commits on Phabricator. --- git-branchless-lib/src/core/eventlog.rs | 1 - git-branchless-submit/src/branch_forge.rs | 2 +- git-branchless-submit/src/github.rs | 32 +++- git-branchless-submit/src/lib.rs | 113 ++++++++++---- git-branchless-submit/src/phabricator.rs | 147 +++++++++++------- .../tests/test_phabricator_forge.rs | 81 ++++++++++ 6 files changed, 285 insertions(+), 91 deletions(-) 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..f569c80ae 100644 --- a/git-branchless-submit/src/branch_forge.rs +++ b/git-branchless-submit/src/branch_forge.rs @@ -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, }, diff --git a/git-branchless-submit/src/github.rs b/git-branchless-submit/src/github.rs index 5bb23dbe7..f8e9474aa 100644 --- a/git-branchless-submit/src/github.rs +++ b/git-branchless-submit/src/github.rs @@ -289,6 +289,25 @@ impl Forge for GithubForge<'_> { }), options )?); + for (commit_oid, create_status) in created_branch.iter() { + match create_status { + CreateStatus::Created { .. } => {} + CreateStatus::Skipped | CreateStatus::Err => { + // FIXME: surface the inner branch forge error somehow? + writeln!( + effects.get_output_stream(), + "Could not create branch for commit: {}", + effects.get_glyphs().render( + self.repo.friendly_describe_commit_from_oid( + effects.get_glyphs(), + *commit_oid + )?, + )? + )?; + return Ok(Err(ExitCode(1))); + } + } + } created_branches.extend(created_branch.into_iter()); } @@ -297,7 +316,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 { @@ -308,11 +327,18 @@ 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()), }, + + Some( + CreateStatus::Skipped | CreateStatus::Err , + ) => { + warn!(?commits_to_create, ?created_branches, ?commit_oid, ?commit_status, "commit failed to be created"); + eyre::bail!("BUG: should have been handled in previous call to branch_forge.create: {commit_oid:?} has status {commit_status:?}"); + } None => commit_status.clone(), }; - (commit_oid, commit_status) + Ok((commit_oid, commit_status)) }) - .collect(); + .try_collect()?; // Create the pull requests only after creating all the branches because // we rely on the presence of a branch on each commit in the stack to diff --git a/git-branchless-submit/src/lib.rs b/git-branchless-submit/src/lib.rs index 3d8dbf530..a219b7111 100644 --- a/git-branchless-submit/src/lib.rs +++ b/git-branchless-submit/src/lib.rs @@ -140,18 +140,28 @@ 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. Possibly it had already been created, or it + /// couldn't be created due to a previous commit's error. + Skipped, + + /// An error occurred while trying to create the commit. + Err, } /// "Forge" refers to a Git hosting provider, such as GitHub, GitLab, etc. @@ -369,30 +379,52 @@ fn submit( (local, unsubmitted, to_update, to_skip) }); - let (submitted_commit_names, unsubmitted_commit_names): (BTreeSet, BTreeSet) = { + let (submitted_commit_names, unsubmitted_commit_names, create_error_commits): ( + BTreeSet, + BTreeSet, + BTreeSet, + ) = { let unsubmitted_commit_names: BTreeSet = unsubmitted_commits .values() .flat_map(|commit_status| commit_status.local_commit_name.clone()) .collect(); if create { - let created_commit_names = if dry_run { - unsubmitted_commit_names.clone() + if dry_run { + ( + unsubmitted_commit_names.clone(), + Default::default(), + Default::default(), + ) } 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, - ) - .collect() - }; - (created_commit_names, Default::default()) + let mut submitted_commit_names = BTreeSet::new(); + let mut error_commits = BTreeSet::new(); + for (commit_oid, create_status) in create_statuses { + match create_status { + CreateStatus::Created { + final_commit_oid: _, // currently not rendered + local_commit_name, + } => { + submitted_commit_names.insert(local_commit_name); + } + // For now, treat `Skipped` the same as `Err` as it would be + // a lot of work to render it differently in the output, and + // we may want to rethink the data structures before doing + // that. + CreateStatus::Skipped | CreateStatus::Err => { + error_commits.insert(commit_oid); + } + } + } + (submitted_commit_names, Default::default(), error_commits) + } } else { - (Default::default(), unsubmitted_commit_names) + ( + Default::default(), + unsubmitted_commit_names, + Default::default(), + ) } }; @@ -512,8 +544,33 @@ repository. To submit them, retry this operation with the --create option.", if dry_run { "are" } else { "were" }, )?; } + if !create_error_commits.is_empty() { + writeln!( + effects.get_output_stream(), + "Failed to create {}:", + Pluralize { + determiner: None, + amount: create_error_commits.len(), + unit: ("commit", "commits") + }, + )?; + for error_commit_oid in &create_error_commits { + let error_commit = repo.find_commit_or_fail(*error_commit_oid)?; + writeln!( + effects.get_output_stream(), + "{}", + effects + .get_glyphs() + .render(error_commit.friendly_describe(effects.get_glyphs())?)? + )?; + } + } - Ok(Ok(())) + if !create_error_commits.is_empty() { + 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 bd4842a0c..72983780e 100644 --- a/git-branchless-submit/src/phabricator.rs +++ b/git-branchless-submit/src/phabricator.rs @@ -8,7 +8,7 @@ use std::process::{Command, Stdio}; use std::str::FromStr; use std::time::SystemTime; -use git_branchless_opts::Revset; +use git_branchless_opts::{Revset, TestSearchStrategy}; use git_branchless_test::{ run_tests, FixInfo, ResolvedTestOptions, TestOutput, TestResults, TestStatus, TestingAbortedError, Verbosity, @@ -19,10 +19,7 @@ use lib::core::check_out::CheckOutCommitOptions; use lib::core::dag::{CommitSet, Dag}; use lib::core::effects::{Effects, OperationType, WithProgress}; use lib::core::eventlog::EventLogDb; -use lib::core::formatting::{ - Effect, StyledString, - StyledStringBuilder, -}; +use lib::core::formatting::{Effect, StyledString, StyledStringBuilder}; use lib::core::rewrite::{ execute_rebase_plan, BuildRebasePlanError, BuildRebasePlanOptions, ExecuteRebasePlanOptions, ExecuteRebasePlanResult, RebasePlanBuilder, RebasePlanPermissions, RepoResource, @@ -374,7 +371,7 @@ 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#"(git show | grep 'BROKEN') && exit 1 || git commit --amend --message "$(git show --no-patch --format=%B HEAD) Differential Revision: https://phabricator.example.com/D000$(git rev-list --count HEAD) " @@ -395,7 +392,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, @@ -430,10 +427,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 @@ -442,8 +452,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: _, @@ -453,66 +463,86 @@ 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, + match error_reason { + ErrorReason::NotTested => CreateStatus::Skipped, + ErrorReason::TestFailed => CreateStatus::Err, + }, + ); + 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::Skipped); + continue; + } }; let local_branch_name = { match self.get_revision_id(final_commit_oid)? { @@ -528,13 +558,14 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun )? )?, )?; - return Ok(Err(ExitCode(1))); + create_statuses.insert(commit_oid, CreateStatus::Err); + continue; } } }; create_statuses.insert( commit_oid, - CreateStatus { + CreateStatus::Created { final_commit_oid, local_commit_name: local_branch_name, }, @@ -543,12 +574,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( diff --git a/git-branchless-submit/tests/test_phabricator_forge.rs b/git-branchless-submit/tests/test_phabricator_forge.rs index 743f9f175..4b2da7597 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 @@ -140,6 +141,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 @@ -190,6 +192,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 @@ -230,3 +233,81 @@ fn test_submit_phabricator_update() -> eyre::Result<()> { 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: + BROKEN: test2.txt + 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 1 commit: D0002 + Failed to create 2 commits: + 5b9de4b BROKEN: test2.txt + e9d3664 create test3.txt + "###); + } + + { + 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 + "###); + } + + Ok(()) +} From 0552f9c75eb4b50a58a917eaff42cff9060b3e18 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Sun, 30 Jun 2024 13:40:49 -0700 Subject: [PATCH 4/4] feat(test): report `PrepareWorkingDirectoryError` reason --- git-branchless-lib/src/git/run.rs | 1 + git-branchless-submit/src/phabricator.rs | 4 +- git-branchless-test/src/lib.rs | 272 +++++++++++++++-------- 3 files changed, 182 insertions(+), 95 deletions(-) diff --git a/git-branchless-lib/src/git/run.rs b/git-branchless-lib/src/git/run.rs index b4846902a..b42cff1c2 100644 --- a/git-branchless-lib/src/git/run.rs +++ b/git-branchless-lib/src/git/run.rs @@ -42,6 +42,7 @@ impl std::fmt::Debug for GitRunInfo { } /// Options for invoking Git. +#[derive(Clone)] pub struct GitRunOpts { /// If set, a non-zero exit code will be treated as an error. pub treat_git_failure_as_error: bool, diff --git a/git-branchless-submit/src/phabricator.rs b/git-branchless-submit/src/phabricator.rs index 72983780e..f7c9f9825 100644 --- a/git-branchless-submit/src/phabricator.rs +++ b/git-branchless-submit/src/phabricator.rs @@ -444,7 +444,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun } }; match test_output.test_status { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal | TestStatus::AlreadyInProgress @@ -706,7 +706,7 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun .into_iter() .partition(|(_commit_oid, test_output)| match test_output.test_status { TestStatus::Passed { .. } => true, - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal | TestStatus::AlreadyInProgress diff --git a/git-branchless-test/src/lib.rs b/git-branchless-test/src/lib.rs index edc473c2e..3980bfc0f 100644 --- a/git-branchless-test/src/lib.rs +++ b/git-branchless-test/src/lib.rs @@ -42,8 +42,7 @@ use lib::core::eventlog::{ EventLogDb, EventReplayer, EventTransactionId, BRANCHLESS_TRANSACTION_ID_ENV_VAR, }; use lib::core::formatting::{ - BaseColor, Effect, Glyphs, Pluralize, Style, StyledString, - StyledStringBuilder, + BaseColor, Effect, Glyphs, Pluralize, Style, StyledString, StyledStringBuilder, }; use lib::core::repo_ext::RepoExt; use lib::core::rewrite::{ @@ -52,9 +51,10 @@ use lib::core::rewrite::{ }; use lib::git::{ get_latest_test_command_path, get_test_locks_dir, get_test_tree_dir, get_test_worktrees_dir, - make_test_command_slug, Commit, ConfigRead, GitRunInfo, GitRunResult, MaybeZeroOid, NonZeroOid, - Repo, SerializedNonZeroOid, SerializedTestResult, TestCommand, WorkingCopyChangesType, - TEST_ABORT_EXIT_CODE, TEST_INDETERMINATE_EXIT_CODE, TEST_SUCCESS_EXIT_CODE, + make_test_command_slug, Commit, ConfigRead, GitRunInfo, GitRunOpts, GitRunResult, MaybeZeroOid, + NonZeroOid, Repo, RepoError, SerializedNonZeroOid, SerializedTestResult, TestCommand, + WorkingCopyChangesType, TEST_ABORT_EXIT_CODE, TEST_INDETERMINATE_EXIT_CODE, + TEST_SUCCESS_EXIT_CODE, }; use lib::try_exit_code; use lib::util::{get_sh, ExitCode, EyreExitOr}; @@ -788,10 +788,10 @@ pub struct TestOutput { } /// The possible results of attempting to run a test. -#[derive(Clone, Debug)] +#[derive(Debug)] pub enum TestStatus { /// Attempting to set up the working directory for the repository failed. - CheckoutFailed, + CheckoutFailed(PrepareWorkingDirectoryError), /// Invoking the test command failed. SpawnTestFailed(String), @@ -866,7 +866,7 @@ impl TestStatus { #[instrument] fn get_icon(&self) -> &'static str { match self { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::AlreadyInProgress | TestStatus::ReadCacheFailed(_) @@ -880,7 +880,7 @@ impl TestStatus { #[instrument] fn get_style(&self) -> Style { match self { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::AlreadyInProgress | TestStatus::ReadCacheFailed(_) @@ -900,9 +900,10 @@ impl TestStatus { apply_fixes: bool, ) -> eyre::Result { let description = match self { - TestStatus::CheckoutFailed => StyledStringBuilder::new() + TestStatus::CheckoutFailed(err) => StyledStringBuilder::new() .append_styled("Failed to check out: ", self.get_style()) .append(commit.friendly_describe(glyphs)?) + .append(format!(": {err}")) .build(), TestStatus::SpawnTestFailed(err) => StyledStringBuilder::new() @@ -1079,7 +1080,7 @@ impl TestOutput { } let interactive = match self.test_status { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal | TestStatus::AlreadyInProgress @@ -1651,7 +1652,7 @@ fn event_loop( operation_type: _, } = job; let (maybe_testing_aborted_error, search_status) = match &test_output.test_status { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal | TestStatus::AlreadyInProgress @@ -1739,7 +1740,7 @@ fn print_summary( )?)? )?; match test_output.test_status { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::AlreadyInProgress | TestStatus::ReadCacheFailed(_) @@ -1948,7 +1949,7 @@ fn apply_fixes( }, interactive: _, } - | TestStatus::CheckoutFailed + | TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal | TestStatus::AlreadyInProgress @@ -2220,13 +2221,14 @@ fn run_test( TestFilesResult::Cached(test_output) => test_output, TestFilesResult::NotCached(test_files) => { match prepare_working_directory( + &effects, git_run_info, repo, event_tx_id, commit, *execution_strategy, worker_id, - )? { + ) { Err(err) => { info!(?err, "Failed to prepare working directory for testing"); let TestFiles { @@ -2244,7 +2246,7 @@ fn run_test( result_path, stdout_path, stderr_path, - test_status: TestStatus::CheckoutFailed, + test_status: TestStatus::CheckoutFailed(err), } } Ok(PreparedWorkingDirectory { @@ -2291,7 +2293,7 @@ fn run_test( .build(); progress.notify_status( match test_output.test_status { - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::AlreadyInProgress | TestStatus::ReadCacheFailed(_) @@ -2485,27 +2487,87 @@ struct PreparedWorkingDirectory { path: PathBuf, } -#[allow(dead_code)] // fields are not read except by `Debug` implementation` -#[derive(Debug)] -enum PrepareWorkingDirectoryError { - LockFailed(PathBuf), +#[allow(missing_docs)] +#[derive(Debug, Error)] +pub enum PrepareWorkingDirectoryError { + #[error("creating lock directory: {path}: {source}")] + CreateLockDir { + path: PathBuf, + #[source] + source: std::io::Error, + }, + + #[error("error when opening lock file at path: {path}: {source}")] + OpenLockFile { + path: PathBuf, + #[source] + source: fslock::Error, + }, + + #[error("lock unavailable at path: {path}")] + LockUnavailable { path: PathBuf }, + + #[error("error when acquiring lock at path: {path}: {source}")] + LockError { + path: PathBuf, + #[source] + source: fslock::Error, + }, + + #[error("repository has no working copy")] NoWorkingCopy, - CheckoutFailed(NonZeroOid), - CreateWorktreeFailed(PathBuf), + + #[error("could not check out to commit")] + CheckoutFailed(GitRunResult), + + #[error("could not get configuration for {item}: {source}")] + Config { + item: &'static str, + #[source] + source: RepoError, + }, + + #[error("could not create parent directory for worktrees: {path}: {source}")] + CreateWorktreeParentDirFailed { + path: PathBuf, + #[source] + source: std::io::Error, + }, + + #[error("could not convert worktree path to UTF-8: {path}")] + InvalidWorktreeName { path: PathBuf }, + + #[error("could not create worktree at path: {path}")] + CreateWorktreeFailed { + path: PathBuf, + git_run_result: GitRunResult, + }, + + #[error(transparent)] + Git(eyre::Error), } #[instrument] fn prepare_working_directory( + effects: &Effects, git_run_info: &GitRunInfo, repo: &Repo, event_tx_id: EventTransactionId, commit: &Commit, strategy: TestExecutionStrategy, worker_id: WorkerId, -) -> eyre::Result> { - let test_lock_dir_path = get_test_locks_dir(repo)?; - std::fs::create_dir_all(&test_lock_dir_path) - .wrap_err_with(|| format!("Creating test lock dir path: {test_lock_dir_path:?}"))?; +) -> Result { + let test_lock_dir_path = + get_test_locks_dir(repo).map_err(|err| PrepareWorkingDirectoryError::Config { + item: "test locks dir", + source: err, + })?; + std::fs::create_dir_all(&test_lock_dir_path).map_err(|err| { + PrepareWorkingDirectoryError::CreateLockDir { + path: test_lock_dir_path.clone(), + source: err, + } + })?; let lock_file_name = match strategy { TestExecutionStrategy::WorkingCopy => "working-copy.lock".to_string(), @@ -2514,102 +2576,120 @@ fn prepare_working_directory( } }; let lock_path = test_lock_dir_path.join(lock_file_name); - let mut lock_file = LockFile::open(&lock_path) - .wrap_err_with(|| format!("Opening working copy lock at {lock_path:?}"))?; - if !lock_file - .try_lock_with_pid() - .wrap_err_with(|| format!("Locking working copy with {lock_path:?}"))? - { - return Ok(Err(PrepareWorkingDirectoryError::LockFailed(lock_path))); + let mut lock_file = + LockFile::open(&lock_path).map_err(|err| PrepareWorkingDirectoryError::OpenLockFile { + path: lock_path.clone(), + source: err, + })?; + match lock_file.try_lock_with_pid() { + Ok(true) => {} + Ok(false) => { + return Err(PrepareWorkingDirectoryError::LockUnavailable { path: lock_path }); + } + Err(err) => { + return Err(PrepareWorkingDirectoryError::LockError { + path: lock_path, + source: err, + }); + } } + let git_run_opts = GitRunOpts { + treat_git_failure_as_error: false, + stdin: None, + }; match strategy { TestExecutionStrategy::WorkingCopy => { let working_copy_path = match repo.get_working_copy_path() { - None => return Ok(Err(PrepareWorkingDirectoryError::NoWorkingCopy)), + None => return Err(PrepareWorkingDirectoryError::NoWorkingCopy), Some(working_copy_path) => working_copy_path.to_owned(), }; - let GitRunResult { exit_code, stdout: _, stderr: _ } = + let git_run_result = // Don't show the `git reset` operation among the progress bars, // as we only want to see the testing status. git_run_info.run_silent( repo, Some(event_tx_id), &["reset", "--hard", &commit.get_oid().to_string()], - Default::default() - ).context("Checking out commit to prepare working directory")?; - if exit_code.is_success() { - Ok(Ok(PreparedWorkingDirectory { + git_run_opts, + ).map_err(PrepareWorkingDirectoryError::Git)?; + if git_run_result.exit_code.is_success() { + Ok(PreparedWorkingDirectory { lock_file, path: working_copy_path, - })) + }) } else { - Ok(Err(PrepareWorkingDirectoryError::CheckoutFailed( - commit.get_oid(), - ))) + Err(PrepareWorkingDirectoryError::CheckoutFailed(git_run_result)) } } TestExecutionStrategy::Worktree => { - let parent_dir = get_test_worktrees_dir(repo)?; - std::fs::create_dir_all(&parent_dir) - .wrap_err_with(|| format!("Creating worktree parent dir at {parent_dir:?}"))?; + let parent_dir = get_test_worktrees_dir(repo).map_err(|err| { + PrepareWorkingDirectoryError::Config { + item: "test worktrees dir", + source: err, + } + })?; + std::fs::create_dir_all(&parent_dir).map_err(|err| { + PrepareWorkingDirectoryError::CreateWorktreeParentDirFailed { + path: parent_dir.clone(), + source: err, + } + })?; let worktree_dir_name = format!("testing-worktree-{worker_id}"); let worktree_dir = parent_dir.join(worktree_dir_name); let worktree_dir_str = match worktree_dir.to_str() { Some(worktree_dir) => worktree_dir, None => { - return Ok(Err(PrepareWorkingDirectoryError::CreateWorktreeFailed( - worktree_dir, - ))); + return Err(PrepareWorkingDirectoryError::InvalidWorktreeName { + path: worktree_dir, + }); } }; if !worktree_dir.exists() { - let GitRunResult { - exit_code, - stdout: _, - stderr: _, - } = git_run_info.run_silent( - repo, - Some(event_tx_id), - &["worktree", "add", worktree_dir_str, "--force", "--detach"], - Default::default(), - )?; - if !exit_code.is_success() { - return Ok(Err(PrepareWorkingDirectoryError::CreateWorktreeFailed( - worktree_dir, - ))); + let git_run_result = git_run_info + .run_silent( + repo, + Some(event_tx_id), + &["worktree", "add", worktree_dir_str, "--force", "--detach"], + git_run_opts.clone(), + ) + .map_err(PrepareWorkingDirectoryError::Git)?; + if !git_run_result.exit_code.is_success() { + return Err(PrepareWorkingDirectoryError::CreateWorktreeFailed { + path: worktree_dir, + git_run_result, + }); } } - let GitRunResult { - exit_code, - stdout: _, - stderr: _, - } = git_run_info.run_silent( - repo, - Some(event_tx_id), - &[ - "-C", - worktree_dir_str, - "checkout", - "--force", - &commit.get_oid().to_string(), - ], - Default::default(), - )?; - if !exit_code.is_success() { - return Ok(Err(PrepareWorkingDirectoryError::CheckoutFailed( - commit.get_oid(), - ))); + { + let git_run_result = git_run_info + .run_silent( + repo, + Some(event_tx_id), + &[ + "-C", + worktree_dir_str, + "checkout", + "--force", + &commit.get_oid().to_string(), + ], + git_run_opts, + ) + .map_err(PrepareWorkingDirectoryError::Git)?; + if !git_run_result.exit_code.is_success() { + return Err(PrepareWorkingDirectoryError::CheckoutFailed(git_run_result)); + } } - Ok(Ok(PreparedWorkingDirectory { + + Ok(PreparedWorkingDirectory { lock_file, path: worktree_dir, - })) + }) } } } @@ -2781,7 +2861,7 @@ To abort testing entirely, run: {exit127}", fix_info, interactive: _, } => Some(fix_info), - TestStatus::CheckoutFailed + TestStatus::CheckoutFailed(_) | TestStatus::SpawnTestFailed(_) | TestStatus::TerminatedBySignal | TestStatus::AlreadyInProgress @@ -2986,6 +3066,8 @@ mod tests { let git = make_git()?; git.init_repo()?; + let glyphs = Glyphs::text(); + let effects = Effects::new_suppress_for_test(glyphs); let git_run_info = git.get_git_run_info(); let repo = git.get_repo()?; let conn = repo.get_db_conn()?; @@ -2996,45 +3078,49 @@ mod tests { let worker_id = 1; let _prepared_working_copy = prepare_working_directory( + &effects, &git_run_info, &repo, event_tx_id, &head_commit, TestExecutionStrategy::WorkingCopy, worker_id, - )? + ) .unwrap(); assert!(matches!( prepare_working_directory( + &effects, &git_run_info, &repo, event_tx_id, &head_commit, TestExecutionStrategy::WorkingCopy, worker_id - )?, - Err(PrepareWorkingDirectoryError::LockFailed(_)) + ), + Err(PrepareWorkingDirectoryError::LockUnavailable { .. }) )); let _prepared_worktree = prepare_working_directory( + &effects, &git_run_info, &repo, event_tx_id, &head_commit, TestExecutionStrategy::Worktree, worker_id, - )? + ) .unwrap(); assert!(matches!( prepare_working_directory( + &effects, &git_run_info, &repo, event_tx_id, &head_commit, TestExecutionStrategy::Worktree, worker_id - )?, - Err(PrepareWorkingDirectoryError::LockFailed(_)) + ), + Err(PrepareWorkingDirectoryError::LockUnavailable { .. }) )); Ok(())