From bc420dfec5f0cabf0ce934b6e4ba4023cc47433d Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Wed, 12 Oct 2022 23:10:24 -0700 Subject: [PATCH] fix(sync): fix syncing commit stacks built on local main branch If the local main branch had local commits (not on the upstream branch) and any descendant (non-main) branches, then we would see the commits shared by both and attempt to rebase them separately. This can work sometimes, but it can also cause problems like rebase cycles or marking public commits as rewritten. --- git-branchless/src/commands/sync.rs | 91 ++++++++++++----------- git-branchless/src/revset/mod.rs | 2 +- git-branchless/src/revset/resolve.rs | 14 ++++ git-branchless/tests/command/test_sync.rs | 35 ++++----- 4 files changed, 82 insertions(+), 60 deletions(-) diff --git a/git-branchless/src/commands/sync.rs b/git-branchless/src/commands/sync.rs index aacaf7dea..aaaba6e59 100644 --- a/git-branchless/src/commands/sync.rs +++ b/git-branchless/src/commands/sync.rs @@ -13,7 +13,7 @@ use lib::util::ExitCode; use rayon::{ThreadPool, ThreadPoolBuilder}; use crate::opts::{MoveOptions, Revset}; -use crate::revset::resolve_commits; +use crate::revset::{check_revset_syntax, resolve_commits}; use lib::core::config::get_restack_preserve_timestamps; use lib::core::dag::{commit_set_to_vec, sorted_commit_set, union_all, CommitSet, Dag}; use lib::core::effects::{Effects, OperationType}; @@ -59,6 +59,10 @@ pub fn sync( let now = SystemTime::now(); let event_tx_id = event_log_db.make_transaction_id(now, "sync fetch")?; + // Try to surface parse errors early, before potentially doing commit graph or network + // side-effects. + check_revset_syntax(&repo, &revsets)?; + if pull { let exit_code = git_run_info.run(effects, Some(event_tx_id), &["fetch", "--all"])?; if !exit_code.is_success() { @@ -66,31 +70,6 @@ pub fn sync( } } - let event_replayer = EventReplayer::from_event_log_db(effects, &repo, &event_log_db)?; - let event_cursor = event_replayer.make_default_cursor(); - let references_snapshot = repo.get_references_snapshot()?; - let mut dag = Dag::open_and_sync( - effects, - &repo, - &event_replayer, - event_cursor, - &references_snapshot, - )?; - - let commit_sets = match resolve_commits(effects, &repo, &mut dag, &revsets) { - Ok(commit_sets) => commit_sets, - Err(err) => { - err.describe(effects)?; - return Ok(ExitCode(1)); - } - }; - let root_commit_oids = if commit_sets.is_empty() { - get_stack_roots(&dag)? - } else { - dag.query().roots(union_all(&commit_sets))? - }; - let root_commits = sorted_commit_set(&repo, &dag, &root_commit_oids)?; - let MoveOptions { force_rewrite_public_commits, force_in_memory, @@ -128,7 +107,6 @@ pub fn sync( effects, git_run_info, &repo, - &mut dag, &event_log_db, &build_options, &execute_options, @@ -140,21 +118,18 @@ pub fn sync( } } - // The main branch OID might have changed since we synced with `master`, so read it again. - let main_branch_oid = repo.get_main_branch_oid()?; + // The main branch might have changed since we synced with `master`, so read its information again. + execute_sync_plans( effects, git_run_info, &repo, &event_log_db, - &mut dag, - &root_commit_oids, - root_commits, - main_branch_oid, &build_options, &execute_options, &thread_pool, &repo_pool, + revsets, ) } @@ -162,13 +137,23 @@ fn execute_main_branch_sync_plan( effects: &Effects, git_run_info: &GitRunInfo, repo: &Repo, - dag: &mut Dag, event_log_db: &EventLogDb, build_options: &BuildRebasePlanOptions, execute_options: &ExecuteRebasePlanOptions, thread_pool: &ThreadPool, repo_pool: &RepoPool, ) -> eyre::Result { + let event_replayer = EventReplayer::from_event_log_db(effects, repo, event_log_db)?; + let event_cursor = event_replayer.make_default_cursor(); + let references_snapshot = repo.get_references_snapshot()?; + let mut dag = Dag::open_and_sync( + effects, + repo, + &event_replayer, + event_cursor, + &references_snapshot, + )?; + let local_main_branch = repo.get_main_branch()?; let local_main_branch_oid = local_main_branch.get_oid()?; let local_main_branch_reference_name = local_main_branch.get_reference_name()?; @@ -251,7 +236,7 @@ fn execute_main_branch_sync_plan( ..build_options.clone() }; let permissions = match RebasePlanPermissions::verify_rewrite_set( - dag, + &dag, &build_options, &local_main_branch_commits, )? { @@ -261,7 +246,7 @@ fn execute_main_branch_sync_plan( return Ok(ExitCode(1)); } }; - let mut builder = RebasePlanBuilder::new(dag, permissions); + let mut builder = RebasePlanBuilder::new(&dag, permissions); let local_main_branch_roots = dag.query().roots(local_main_branch_commits)?; let root_commit_oid = match commit_set_to_vec(&local_main_branch_roots)? .into_iter() @@ -298,24 +283,46 @@ fn execute_sync_plans( git_run_info: &GitRunInfo, repo: &Repo, event_log_db: &EventLogDb, - dag: &mut Dag, - root_commit_oids: &CommitSet, - root_commits: Vec, - main_branch_oid: NonZeroOid, build_options: &BuildRebasePlanOptions, execute_options: &ExecuteRebasePlanOptions, thread_pool: &ThreadPool, repo_pool: &ResourcePool, + revsets: Vec, ) -> eyre::Result { + let event_replayer = EventReplayer::from_event_log_db(effects, repo, event_log_db)?; + let event_cursor = event_replayer.make_default_cursor(); + let references_snapshot = repo.get_references_snapshot()?; + let mut dag = Dag::open_and_sync( + effects, + repo, + &event_replayer, + event_cursor, + &references_snapshot, + )?; + let commit_sets = match resolve_commits(effects, repo, &mut dag, &revsets) { + Ok(commit_sets) => commit_sets, + Err(err) => { + err.describe(effects)?; + return Ok(ExitCode(1)); + } + }; + let main_branch_oid = repo.get_main_branch_oid()?; + let root_commit_oids = if commit_sets.is_empty() { + get_stack_roots(&dag)? + } else { + dag.query().roots(union_all(&commit_sets))? + }; + + let root_commits = sorted_commit_set(repo, &dag, &root_commit_oids)?; let permissions = - match RebasePlanPermissions::verify_rewrite_set(dag, build_options, root_commit_oids)? { + match RebasePlanPermissions::verify_rewrite_set(&dag, build_options, &root_commit_oids)? { Ok(permissions) => permissions, Err(err) => { err.describe(effects, repo)?; return Ok(ExitCode(1)); } }; - let builder = RebasePlanBuilder::new(dag, permissions); + let builder = RebasePlanBuilder::new(&dag, permissions); let root_commit_oids = root_commits .into_iter() diff --git a/git-branchless/src/revset/mod.rs b/git-branchless/src/revset/mod.rs index aaabfcde3..c57b3fe7f 100644 --- a/git-branchless/src/revset/mod.rs +++ b/git-branchless/src/revset/mod.rs @@ -11,7 +11,7 @@ mod resolve; pub use ast::Expr; pub use eval::eval; pub use parser::parse; -pub use resolve::resolve_commits; +pub use resolve::{check_revset_syntax, resolve_commits}; use lalrpop_util::lalrpop_mod; lalrpop_mod!( diff --git a/git-branchless/src/revset/resolve.rs b/git-branchless/src/revset/resolve.rs index 3951d4202..59e7955f8 100644 --- a/git-branchless/src/revset/resolve.rs +++ b/git-branchless/src/revset/resolve.rs @@ -6,6 +6,7 @@ use lib::git::Repo; use tracing::instrument; use crate::opts::Revset; +use crate::revset::Expr; use super::eval::EvalError; use super::parser::ParseError; @@ -48,6 +49,17 @@ impl ResolveError { } } +/// Check for syntax errors in the provided revsets without actually evaluating them. +pub fn check_revset_syntax(repo: &Repo, revsets: &[Revset]) -> Result<(), ParseError> { + for Revset(revset) in revsets { + if let Ok(Some(_)) = repo.revparse_single_commit(revset) { + continue; + } + let _expr: Expr = parse(revset)?; + } + Ok(()) +} + /// Parse strings which refer to commits, such as: /// /// - Full OIDs. @@ -62,6 +74,8 @@ pub fn resolve_commits( ) -> Result, ResolveError> { let mut commit_sets = Vec::new(); for Revset(revset) in revsets { + // NB: also update `check_parse_revsets` + // Handle syntax that's supported by Git, but which we haven't // implemented in the revset language. if let Ok(Some(commit)) = repo.revparse_single_commit(revset) { diff --git a/git-branchless/tests/command/test_sync.rs b/git-branchless/tests/command/test_sync.rs index d9c8aa060..df400c6a1 100644 --- a/git-branchless/tests/command/test_sync.rs +++ b/git-branchless/tests/command/test_sync.rs @@ -135,19 +135,20 @@ fn test_sync_pull() -> eyre::Result<()> { original_repo.init_repo()?; original_repo.commit_file("test1", 1)?; original_repo.commit_file("test2", 2)?; - original_repo.run(&["checkout", "-b", "foo"])?; - original_repo.commit_file("test3", 3)?; original_repo.clone_repo_into(&cloned_repo, &["--branch", "master"])?; cloned_repo.init_repo_with_options(&GitInitOptions { make_initial_commit: false, ..Default::default() })?; - cloned_repo.run(&["checkout", "foo"])?; + cloned_repo.detach_head()?; + original_repo.commit_file("test3", 3)?; original_repo.commit_file("test4", 4)?; - original_repo.run(&["checkout", "master"])?; original_repo.commit_file("test5", 5)?; + cloned_repo.commit_file("test3", 3)?; + cloned_repo.commit_file("test4", 4)?; + cloned_repo.commit_file("test6", 6)?; { let (stdout, _stderr) = cloned_repo.run(&["smartlog"])?; @@ -155,7 +156,11 @@ fn test_sync_pull() -> eyre::Result<()> { : O 96d1c37 (master) create test2.txt | - @ 70deb1e (> foo) create test3.txt + o 70deb1e create test3.txt + | + o 355e173 create test4.txt + | + @ 6ac5566 create test6.txt "###); } @@ -164,17 +169,13 @@ fn test_sync_pull() -> eyre::Result<()> { let stdout: String = remove_nondeterministic_lines(stdout); insta::assert_snapshot!(stdout, @r###" branchless: running command: fetch --all - Fast-forwarding branch master to d2e18e3 create test5.txt + Fast-forwarding branch master to f81d55c create test5.txt Attempting rebase in-memory... - [1/1] Committed as: 8e521a1 create test3.txt - branchless: processing 1 update: branch foo + [1/1] Committed as: 2831fb5 create test6.txt branchless: processing 1 rewritten commit - branchless: running command: checkout foo - Your branch and 'origin/foo' have diverged, - and have 2 and 2 different commits each, respectively. - (use "git pull" to merge the remote branch into yours) + branchless: running command: checkout 2831fb5864ee099dc3e448a38dcb3c8527149510 In-memory rebase succeeded. - Synced 70deb1e create test3.txt + Synced 6ac5566 create test6.txt "###); } @@ -182,9 +183,9 @@ fn test_sync_pull() -> eyre::Result<()> { let (stdout, _stderr) = cloned_repo.run(&["smartlog"])?; insta::assert_snapshot!(stdout, @r###" : - O d2e18e3 (master) create test5.txt + O f81d55c (master) create test5.txt | - @ 8e521a1 (> foo) create test3.txt + @ 2831fb5 create test6.txt "###); } @@ -193,8 +194,8 @@ fn test_sync_pull() -> eyre::Result<()> { let stdout: String = remove_nondeterministic_lines(stdout); insta::assert_snapshot!(stdout, @r###" branchless: running command: fetch --all - Not updating branch master at d2e18e3 create test5.txt - Not moving up-to-date stack at 8e521a1 create test3.txt + Not updating branch master at f81d55c create test5.txt + Not moving up-to-date stack at 2831fb5 create test6.txt "###); }