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

@@ -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,38 +59,17 @@ 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() {
  return Ok(exit_code);
  }
  }
 
- 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,35 +118,42 @@ 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,
  )
 }
 
 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<ExitCode> {
+ 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<Commit>,
- main_branch_oid: NonZeroOid,
  build_options: &BuildRebasePlanOptions,
  execute_options: &ExecuteRebasePlanOptions,
  thread_pool: &ThreadPool,
  repo_pool: &ResourcePool<RepoResource>,
+ revsets: Vec<Revset>,
 ) -> eyre::Result<ExitCode> {
+ 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()

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!(

git-branchless/src/revset/resolve.rs

@@ -48,6 +48,19 @@ 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;
+ }
+ if let Err(err) = parse(revset) {
+ return Err(err);
+ }
+ }
+ Ok(())
+}
+
 /// Parse strings which refer to commits, such as:
 ///
 /// - Full OIDs.
@@ -62,6 +75,8 @@ pub fn resolve_commits(
 ) -> Result<Vec<CommitSet>, 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) {

git-branchless/tests/command/test_sync.rs

@@ -135,27 +135,32 @@ 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"])?;
  insta::assert_snapshot!(stdout, @r###"
  :
  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,27 +169,23 @@ fn test_sync_pull() -> eyre::Result<()> {
  let stdout: String = remove_nondeterministic_lines(stdout);
  insta::assert_snapshot!(stdout, @r###"
  branchless: running command: <git-executable> 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: <git-executable> 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: <git-executable> checkout 2831fb5864ee099dc3e448a38dcb3c8527149510
  In-memory rebase succeeded.
- Synced 70deb1e create test3.txt
+ Synced 6ac5566 create test6.txt
  "###);
  }
 
  {
  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: <git-executable> 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
  "###);
  }