From 9ec27645cfe6a031cec62cd57687b778e05556ea Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 25 Sep 2021 14:36:34 -0700 Subject: [PATCH] revsets: remove `all_heads()` Now that we remove hidden heads whenever a transaction commits, `non_obsolete_heads()` should always be the same as `all_heads()`, except during a transaction. I don't think we depend on the difference even during a transaction. Let's simplify a bit by removing the revset function `all_heads()` and renaming `non_obsolete_heads()` to `heads()`. This is part of issue #32. --- lib/src/revset.rs | 95 ++++++++++++----------------------- lib/src/rewrite.rs | 3 +- lib/tests/test_revset.rs | 105 +++++++-------------------------------- src/commands.rs | 6 +-- 4 files changed, 54 insertions(+), 155 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 383d48e30..6c5561635 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -95,10 +95,7 @@ fn resolve_commit_id(repo: RepoRef, symbol: &str) -> Result, Revse Err(RevsetError::NoSuchRevision(symbol.to_owned())) } -fn resolve_non_obsolete_change_id( - repo: RepoRef, - change_id_prefix: &str, -) -> Result, RevsetError> { +fn resolve_change_id(repo: RepoRef, change_id_prefix: &str) -> Result, RevsetError> { if let Some(hex_prefix) = HexPrefix::new(change_id_prefix.to_owned()) { let evolution = repo.evolution(); match evolution.resolve_change_id_prefix(&hex_prefix) { @@ -146,8 +143,8 @@ pub fn resolve_symbol(repo: RepoRef, symbol: &str) -> Result, Revs return commit_id_result; } - // Try to resolve as a change id (the non-obsolete commits in the change). - let change_id_result = resolve_non_obsolete_change_id(repo, symbol); + // Try to resolve as a change id. + let change_id_result = resolve_change_id(repo, symbol); if !matches!(change_id_result, Err(RevsetError::NoSuchRevision(_))) { return change_id_result; } @@ -191,7 +188,7 @@ pub enum RevsetExpression { roots: Rc, heads: Rc, }, - AllHeads, + Heads, PublicHeads, Branches, Tags, @@ -215,6 +212,10 @@ impl RevsetExpression { Rc::new(RevsetExpression::None) } + pub fn all() -> Rc { + RevsetExpression::heads().ancestors() + } + pub fn symbol(value: String) -> Rc { Rc::new(RevsetExpression::Symbol(value)) } @@ -227,8 +228,8 @@ impl RevsetExpression { Rc::new(RevsetExpression::Commits(commit_ids)) } - pub fn all_heads() -> Rc { - Rc::new(RevsetExpression::AllHeads) + pub fn heads() -> Rc { + Rc::new(RevsetExpression::Heads) } pub fn public_heads() -> Rc { @@ -247,14 +248,6 @@ impl RevsetExpression { Rc::new(RevsetExpression::GitRefs) } - pub fn all_non_obsolete_heads() -> Rc { - RevsetExpression::all_heads().non_obsolete_heads() - } - - pub fn all_non_obsolete_commits() -> Rc { - RevsetExpression::all_non_obsolete_heads().ancestors() - } - /// Non-obsolete heads among `self`. pub fn non_obsolete_heads(self: &Rc) -> Rc { Rc::new(RevsetExpression::NonObsoleteHeads(self.clone())) @@ -411,7 +404,7 @@ fn parse_range_expression_rule( if let Some(next) = pairs.next() { match next.as_rule() { Rule::descendants_op => { - expression = expression.descendants(&RevsetExpression::all_non_obsolete_heads()); + expression = expression.descendants(&RevsetExpression::heads()); } Rule::dag_range_op => { let heads_expression = @@ -438,7 +431,7 @@ fn parse_children_expression_rule( for operator in pairs { match operator.as_rule() { Rule::children_op => { - expression = expression.children(&RevsetExpression::all_non_obsolete_heads()); + expression = expression.children(&RevsetExpression::heads()); } _ => { panic!( @@ -524,7 +517,7 @@ fn parse_function_expression( if arg_count == 1 { let expression = parse_expression_rule(argument_pairs.next().unwrap().into_inner())?; - Ok(expression.children(&RevsetExpression::all_non_obsolete_heads())) + Ok(expression.children(&RevsetExpression::heads())) } else { Err(RevsetParseError::InvalidFunctionArguments { name, @@ -546,7 +539,7 @@ fn parse_function_expression( if arg_count == 1 { let expression = parse_expression_rule(argument_pairs.next().unwrap().into_inner())?; - Ok(expression.descendants(&RevsetExpression::all_non_obsolete_heads())) + Ok(expression.descendants(&RevsetExpression::heads())) } else { Err(RevsetParseError::InvalidFunctionArguments { name, @@ -554,9 +547,9 @@ fn parse_function_expression( }) } } - "all_heads" => { + "heads" => { if arg_count == 0 { - Ok(RevsetExpression::all_heads()) + Ok(RevsetExpression::heads()) } else { Err(RevsetParseError::InvalidFunctionArguments { name, @@ -564,21 +557,6 @@ fn parse_function_expression( }) } } - "non_obsolete_heads" => { - if arg_count == 0 { - Ok(RevsetExpression::all_non_obsolete_heads()) - } else if arg_count == 1 { - Ok( - parse_expression_rule(argument_pairs.next().unwrap().into_inner())? - .non_obsolete_heads(), - ) - } else { - Err(RevsetParseError::InvalidFunctionArguments { - name, - message: "Expected 0 or 1 argument".to_string(), - }) - } - } "public_heads" => { if arg_count == 0 { Ok(RevsetExpression::public_heads()) @@ -627,7 +605,7 @@ fn parse_function_expression( }); } let candidates = if arg_count == 0 { - RevsetExpression::all_non_obsolete_commits() + RevsetExpression::all() } else { parse_expression_rule(argument_pairs.next().unwrap().into_inner())? }; @@ -645,7 +623,7 @@ fn parse_function_expression( argument_pairs.next().unwrap().into_inner(), )?; let candidates = if arg_count == 1 { - RevsetExpression::all_non_obsolete_commits() + RevsetExpression::all() } else { parse_expression_rule(argument_pairs.next().unwrap().into_inner())? }; @@ -1042,7 +1020,7 @@ pub fn evaluate_expression<'repo>( index_entries: result, })) } - RevsetExpression::AllHeads => { + RevsetExpression::Heads => { let index = repo.index(); let heads = repo.view().heads(); let mut index_entries = heads @@ -1240,10 +1218,6 @@ mod tests { heads: checkout_symbol.clone() }) ); - assert_eq!( - foo_symbol.non_obsolete_heads(), - Rc::new(RevsetExpression::NonObsoleteHeads(foo_symbol.clone())) - ); assert_eq!( foo_symbol.with_parent_count(3..5), Rc::new(RevsetExpression::ParentCount { @@ -1299,14 +1273,14 @@ mod tests { // Parse the "children" operator assert_eq!( parse("@:"), - Ok(checkout_symbol.children(&RevsetExpression::all_non_obsolete_heads())) + Ok(checkout_symbol.children(&RevsetExpression::heads())) ); // Parse the "ancestors" operator assert_eq!(parse(",,@"), Ok(checkout_symbol.ancestors())); // Parse the "descendants" operator assert_eq!( parse("@,,"), - Ok(checkout_symbol.descendants(&RevsetExpression::all_non_obsolete_heads())) + Ok(checkout_symbol.descendants(&RevsetExpression::heads())) ); // Parse the "dag range" operator assert_eq!(parse("foo,,bar"), Ok(foo_symbol.descendants(&bar_symbol))); @@ -1326,11 +1300,11 @@ mod tests { assert_matches!(parse("foo | :"), Err(RevsetParseError::SyntaxError(_))); // Space is allowed around infix operators and function arguments assert_eq!( - parse(" description( arg1 , arg2 ) - parents( arg1 ) - all_heads( ) "), + parse(" description( arg1 , arg2 ) - parents( arg1 ) - heads( ) "), Ok(RevsetExpression::symbol("arg2".to_string()) .with_description("arg1".to_string()) .minus(&RevsetExpression::symbol("arg1".to_string()).parents()) - .minus(&RevsetExpression::all_heads())) + .minus(&RevsetExpression::heads())) ); } @@ -1346,9 +1320,9 @@ mod tests { assert_eq!( parse("foo:::"), Ok(foo_symbol - .children(&RevsetExpression::all_non_obsolete_heads()) - .children(&RevsetExpression::all_non_obsolete_heads()) - .children(&RevsetExpression::all_non_obsolete_heads())) + .children(&RevsetExpression::heads()) + .children(&RevsetExpression::heads()) + .children(&RevsetExpression::heads())) ); // Parse repeated "ancestors"/"descendants"/"dag range" operators assert_matches!(parse(",,foo,,"), Err(RevsetParseError::SyntaxError(_))); @@ -1361,21 +1335,15 @@ mod tests { // The former bind more strongly. assert_eq!( parse(":foo:"), - Ok(foo_symbol - .parents() - .children(&RevsetExpression::all_non_obsolete_heads())) + Ok(foo_symbol.parents().children(&RevsetExpression::heads())) ); assert_eq!( parse(":foo,,"), - Ok(foo_symbol - .parents() - .descendants(&RevsetExpression::all_non_obsolete_heads())) + Ok(foo_symbol.parents().descendants(&RevsetExpression::heads())) ); assert_eq!( parse(",,foo:"), - Ok(foo_symbol - .children(&RevsetExpression::all_non_obsolete_heads()) - .ancestors()) + Ok(foo_symbol.children(&RevsetExpression::heads()).ancestors()) ); } @@ -1402,11 +1370,10 @@ mod tests { Ok(RevsetExpression::symbol("bar".to_string()).with_description("foo".to_string())) ); assert_eq!( - parse("description(all_heads(),bar)"), + parse("description(heads(),bar)"), Err(RevsetParseError::InvalidFunctionArguments { name: "description".to_string(), - message: "Expected function argument of type string, found: all_heads()" - .to_string() + message: "Expected function argument of type string, found: heads()".to_string() }) ); assert_eq!( diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 1ddbb9fab..a073ee6a1 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -138,8 +138,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { let new_commits_expression = RevsetExpression::commits(rewritten.values().flatten().cloned().collect()); - let to_visit_expression = - old_commits_expression.descendants(&RevsetExpression::all_heads()); + let to_visit_expression = old_commits_expression.descendants(&RevsetExpression::heads()); let to_visit_revset = to_visit_expression .evaluate(mut_repo.as_repo_ref()) .unwrap(); diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 74418bbae..584ca034a 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -496,18 +496,13 @@ fn test_evaluate_expression_children(use_git: bool) { .set_parents(vec![commit1.id().clone()]) .write_to_repo(mut_repo); let commit3 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit1.id().clone()]) - .set_predecessors(vec![commit2.id().clone()]) - .set_change_id(commit2.change_id().clone()) + .set_parents(vec![commit2.id().clone()]) .write_to_repo(mut_repo); let commit4 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit3.id().clone()]) - .write_to_repo(mut_repo); - let commit5 = testutils::create_random_commit(&settings, &repo) .set_parents(vec![commit1.id().clone()]) .write_to_repo(mut_repo); - let commit6 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit4.id().clone(), commit5.id().clone()]) + let commit5 = testutils::create_random_commit(&settings, &repo) + .set_parents(vec![commit3.id().clone(), commit4.id().clone()]) .write_to_repo(mut_repo); // Can find children of the root commit @@ -516,23 +511,17 @@ fn test_evaluate_expression_children(use_git: bool) { vec![commit1.id().clone(), wc_commit.id().clone()] ); - // Children do not include hidden commits (commit2) - assert_eq!( - resolve_commit_ids(mut_repo.as_repo_ref(), &format!("{}:", commit1.id().hex())), - vec![commit5.id().clone(), commit3.id().clone()] - ); - // Children of all commits in input are returned, including those already in the // input set assert_eq!( resolve_commit_ids( mut_repo.as_repo_ref(), - &format!("({} | {}):", commit1.id().hex(), commit3.id().hex()) + &format!("({} | {}):", commit1.id().hex(), commit2.id().hex()) ), vec![ - commit5.id().clone(), commit4.id().clone(), - commit3.id().clone() + commit3.id().clone(), + commit2.id().clone() ] ); @@ -540,9 +529,9 @@ fn test_evaluate_expression_children(use_git: bool) { assert_eq!( resolve_commit_ids( mut_repo.as_repo_ref(), - &format!("({} | {}):", commit4.id().hex(), commit5.id().hex()) + &format!("({} | {}):", commit3.id().hex(), commit4.id().hex()) ), - vec![commit6.id().clone()] + vec![commit5.id().clone()] ); tx.discard(); @@ -740,29 +729,23 @@ fn test_evaluate_expression_descendants(use_git: bool) { .set_parents(vec![commit1.id().clone()]) .write_to_repo(mut_repo); let commit3 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit1.id().clone()]) - .set_predecessors(vec![commit2.id().clone()]) - .set_change_id(commit2.change_id().clone()) + .set_parents(vec![commit2.id().clone()]) .write_to_repo(mut_repo); let commit4 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit3.id().clone()]) - .write_to_repo(mut_repo); - let commit5 = testutils::create_random_commit(&settings, &repo) .set_parents(vec![commit1.id().clone()]) .write_to_repo(mut_repo); - let commit6 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit4.id().clone(), commit5.id().clone()]) + let commit5 = testutils::create_random_commit(&settings, &repo) + .set_parents(vec![commit3.id().clone(), commit4.id().clone()]) .write_to_repo(mut_repo); - // The descendants of the root commit is all the non-hidden commits in the repo - // (commit2 is excluded) + // The descendants of the root commit are all the commits in the repo assert_eq!( resolve_commit_ids(mut_repo.as_repo_ref(), "root,,"), vec![ - commit6.id().clone(), commit5.id().clone(), commit4.id().clone(), commit3.id().clone(), + commit2.id().clone(), commit1.id().clone(), wc_commit.id().clone(), root_commit.id().clone(), @@ -771,11 +754,11 @@ fn test_evaluate_expression_descendants(use_git: bool) { // Can find descendants of a specific commit assert_eq!( - resolve_commit_ids(mut_repo.as_repo_ref(), &format!("{},,", commit3.id().hex())), + resolve_commit_ids(mut_repo.as_repo_ref(), &format!("{},,", commit2.id().hex())), vec![ - commit6.id().clone(), - commit4.id().clone(), + commit5.id().clone(), commit3.id().clone(), + commit2.id().clone(), ] ); @@ -784,7 +767,7 @@ fn test_evaluate_expression_descendants(use_git: bool) { #[test_case(false ; "local backend")] #[test_case(true ; "git backend")] -fn test_evaluate_expression_all_heads(use_git: bool) { +fn test_evaluate_expression_heads(use_git: bool) { let settings = testutils::user_settings(); let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); @@ -796,7 +779,7 @@ fn test_evaluate_expression_all_heads(use_git: bool) { let commit2 = graph_builder.commit_with_parents(&[&commit1]); assert_eq!( - resolve_commit_ids(mut_repo.as_repo_ref(), "all_heads()"), + resolve_commit_ids(mut_repo.as_repo_ref(), "heads()"), vec![commit2.id().clone(), wc_commit.id().clone()] ); @@ -907,53 +890,6 @@ fn test_evaluate_expression_git_refs(use_git: bool) { tx.discard(); } -#[test_case(false ; "local backend")] -#[test_case(true ; "git backend")] -fn test_evaluate_expression_obsolete(use_git: bool) { - let settings = testutils::user_settings(); - let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); - - let mut tx = repo.start_transaction("test"); - let mut_repo = tx.mut_repo(); - - let root_commit = repo.store().root_commit(); - let wc_commit = repo.working_copy_locked().current_commit(); - let commit1 = testutils::create_random_commit(&settings, &repo).write_to_repo(mut_repo); - let commit2 = testutils::create_random_commit(&settings, &repo) - .set_predecessors(vec![commit1.id().clone()]) - .set_change_id(commit1.change_id().clone()) - .write_to_repo(mut_repo); - let commit3 = testutils::create_random_commit(&settings, &repo) - .set_predecessors(vec![commit2.id().clone()]) - .set_change_id(commit2.change_id().clone()) - .write_to_repo(mut_repo); - let commit4 = testutils::create_random_commit(&settings, &repo) - .set_parents(vec![commit3.id().clone()]) - .set_pruned(true) - .write_to_repo(mut_repo); - - assert_eq!( - resolve_commit_ids(mut_repo.as_repo_ref(), "non_obsolete_heads()"), - vec![commit3.id().clone(), wc_commit.id().clone()] - ); - assert_eq!( - resolve_commit_ids( - mut_repo.as_repo_ref(), - &format!("non_obsolete_heads({})", commit4.id().hex()) - ), - vec![commit3.id().clone()] - ); - assert_eq!( - resolve_commit_ids( - mut_repo.as_repo_ref(), - &format!("non_obsolete_heads({})", commit1.id().hex()) - ), - vec![root_commit.id().clone()] - ); - - tx.discard(); -} - #[test_case(false ; "local backend")] #[test_case(true ; "git backend")] fn test_evaluate_expression_merges(use_git: bool) { @@ -1023,10 +959,7 @@ fn test_evaluate_expression_description(use_git: bool) { ); // Searches only among candidates if specified assert_eq!( - resolve_commit_ids( - mut_repo.as_repo_ref(), - "description(\"commit 2\",all_heads())" - ), + resolve_commit_ids(mut_repo.as_repo_ref(), "description(\"commit 2\",heads())"), vec![] ); diff --git a/src/commands.rs b/src/commands.rs index d0bad5e96..0e8914a2a 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -691,7 +691,7 @@ With the `--from` and/or `--to` options, shows the difference from/to the given .long("revisions") .short("r") .takes_value(true) - .default_value(",,non_obsolete_heads()") + .default_value(",,heads()") .help("Which revisions to show"), ) .arg( @@ -2543,8 +2543,8 @@ fn cmd_rebase( // Manually rebase children because we don't want to rebase them onto the // rewritten commit. (But we still want to record the commit as rewritten so // branches and the working copy get updated to the rewritten commit.) - let children_expression = RevsetExpression::commit(old_commit.id().clone()) - .children(&RevsetExpression::all_non_obsolete_heads()); + let children_expression = + RevsetExpression::commit(old_commit.id().clone()).children(&RevsetExpression::heads()); let mut num_rebased_descendants = 0; let store = repo_command.repo.store(); for child_index_entry in children_expression