From 9e8a7e2ba61c57eb302bc007e47d820ea936f62d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 4 Apr 2021 22:56:10 -0700 Subject: [PATCH] revsets: move code for resolving symbol to commit to new module --- lib/src/lib.rs | 1 + lib/src/revset.rs | 68 ++++++++++++++++++++ lib/tests/test_revset.rs | 135 +++++++++++++++++++++++++++++++++++++++ src/commands.rs | 53 +++++---------- 4 files changed, 221 insertions(+), 36 deletions(-) create mode 100644 lib/src/revset.rs create mode 100644 lib/tests/test_revset.rs diff --git a/lib/src/lib.rs b/lib/src/lib.rs index bc8793c12..47f2a9911 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -39,6 +39,7 @@ pub mod operation; pub mod protos; pub mod repo; pub mod repo_path; +pub mod revset; pub mod rewrite; pub mod settings; pub mod simple_op_store; diff --git a/lib/src/revset.rs b/lib/src/revset.rs new file mode 100644 index 000000000..bd6d3a62a --- /dev/null +++ b/lib/src/revset.rs @@ -0,0 +1,68 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use thiserror::Error; + +use crate::commit::Commit; +use crate::index::{HexPrefix, PrefixResolution}; +use crate::repo::RepoRef; +use crate::store::{CommitId, StoreError}; + +#[derive(Debug, Error, PartialEq, Eq)] +pub enum RevsetError { + #[error("Revision \"{0}\" doesn't exist")] + NoSuchRevision(String), + #[error("Commit id prefix \"{0}\" is ambiguous")] + AmbiguousCommitIdPrefix(String), + #[error("Unexpected error from store: {0}")] + StoreError(#[from] StoreError), +} + +// TODO: Decide if we should allow a single symbol to resolve to multiple +// revisions. For example, we may want to resolve a change id to all the +// matching commits. Depending on how we decide to handle divergent git refs and +// similar, we may also want those to resolve to multiple commits. +pub fn resolve_symbol(repo: RepoRef, symbol: &str) -> Result { + // TODO: Support git refs and change ids. + if symbol == "@" { + Ok(repo.store().get_commit(repo.view().checkout())?) + } else if symbol == "root" { + Ok(repo.store().root_commit()) + } else { + // Try to resolve as a commit id. First check if it's a full commit id. + if let Ok(binary_commit_id) = hex::decode(symbol) { + let commit_id = CommitId(binary_commit_id); + match repo.store().get_commit(&commit_id) { + Ok(commit) => return Ok(commit), + Err(StoreError::NotFound) => {} // fall through + Err(err) => return Err(RevsetError::StoreError(err)), + } + } + let commit_id = resolve_commit_id_prefix(repo, symbol)?; + Ok(repo.store().get_commit(&commit_id)?) + } +} + +fn resolve_commit_id_prefix(repo: RepoRef, symbol: &str) -> Result { + match repo + .index() + .resolve_prefix(&HexPrefix::new(symbol.to_owned())) + { + PrefixResolution::NoMatch => Err(RevsetError::NoSuchRevision(symbol.to_owned())), + PrefixResolution::AmbiguousMatch => { + Err(RevsetError::AmbiguousCommitIdPrefix(symbol.to_owned())) + } + PrefixResolution::SingleMatch(id) => Ok(id), + } +} diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs new file mode 100644 index 000000000..94a831ff6 --- /dev/null +++ b/lib/tests/test_revset.rs @@ -0,0 +1,135 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use jujube_lib::commit_builder::CommitBuilder; +use jujube_lib::revset::{resolve_symbol, RevsetError}; +use jujube_lib::store::{MillisSinceEpoch, Signature, Timestamp}; +use jujube_lib::testutils; +use test_case::test_case; + +#[test_case(false ; "local store")] +#[test_case(true ; "git store")] +fn test_resolve_symbol_root(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + assert_eq!( + resolve_symbol(repo.as_repo_ref(), "root").unwrap(), + repo.store().root_commit() + ); +} + +#[test] +fn test_resolve_symbol_commit_id() { + let settings = testutils::user_settings(); + // Test only with git so we can get predictable commit ids + let (_temp_dir, repo) = testutils::init_repo(&settings, true); + + let mut tx = repo.start_transaction("test"); + let mut_repo = tx.mut_repo(); + let signature = Signature { + name: "test".to_string(), + email: "test".to_string(), + timestamp: Timestamp { + timestamp: MillisSinceEpoch(0), + tz_offset: 0, + }, + }; + + let mut commits = vec![]; + for i in &[1, 167, 895] { + let commit = CommitBuilder::for_new_commit( + &settings, + repo.store(), + repo.store().empty_tree_id().clone(), + ) + .set_description(format!("test {}", i)) + .set_author(signature.clone()) + .set_committer(signature.clone()) + .write_to_repo(mut_repo); + commits.push(commit); + } + + // Test the test setup + assert_eq!( + commits[0].id().hex(), + "0454de3cae04c46cda37ba2e8873b4c17ff51dcb" + ); + assert_eq!( + commits[1].id().hex(), + "045f56cd1b17e8abde86771e2705395dcde6a957" + ); + assert_eq!( + commits[2].id().hex(), + "0468f7da8de2ce442f512aacf83411d26cd2e0cf" + ); + + // Test lookup by full commit id + let repo_ref = mut_repo.as_repo_ref(); + assert_eq!( + resolve_symbol(repo_ref, "0454de3cae04c46cda37ba2e8873b4c17ff51dcb").unwrap(), + commits[0] + ); + assert_eq!( + resolve_symbol(repo_ref, "045f56cd1b17e8abde86771e2705395dcde6a957").unwrap(), + commits[1] + ); + assert_eq!( + resolve_symbol(repo_ref, "0468f7da8de2ce442f512aacf83411d26cd2e0cf").unwrap(), + commits[2] + ); + + // Test commit id prefix + assert_eq!(resolve_symbol(repo_ref, "046").unwrap(), commits[2]); + assert_eq!( + resolve_symbol(repo_ref, "04"), + Err(RevsetError::AmbiguousCommitIdPrefix("04".to_string())) + ); + assert_eq!( + resolve_symbol(repo_ref, ""), + Err(RevsetError::AmbiguousCommitIdPrefix("".to_string())) + ); + assert_eq!( + resolve_symbol(repo_ref, "040"), + Err(RevsetError::NoSuchRevision("040".to_string())) + ); + + tx.discard(); +} + +#[test_case(false ; "local store")] +#[test_case(true ; "git store")] +fn test_resolve_symbol_checkout(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 commit1 = testutils::create_random_commit(&settings, &repo).write_to_repo(mut_repo); + let commit2 = testutils::create_random_commit(&settings, &repo).write_to_repo(mut_repo); + + mut_repo.set_checkout(commit1.id().clone()); + assert_eq!( + resolve_symbol(mut_repo.as_repo_ref(), "@").unwrap(), + commit1 + ); + mut_repo.set_checkout(commit2.id().clone()); + assert_eq!( + resolve_symbol(mut_repo.as_repo_ref(), "@").unwrap(), + commit2 + ); + + tx.discard(); +} diff --git a/src/commands.rs b/src/commands.rs index 134748379..dea93ee68 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -34,19 +34,20 @@ use jujube_lib::dag_walk::{topo_order_reverse, walk_ancestors}; use jujube_lib::evolution::{evolve, EvolveListener}; use jujube_lib::files::DiffLine; use jujube_lib::git::GitFetchError; -use jujube_lib::index::{HexPrefix, PrefixResolution}; +use jujube_lib::index::HexPrefix; use jujube_lib::op_store::{OpStore, OpStoreError, OperationId}; use jujube_lib::operation::Operation; use jujube_lib::repo::{MutableRepo, ReadonlyRepo, RepoLoadError, RepoLoader}; use jujube_lib::repo_path::RepoPath; +use jujube_lib::revset::RevsetError; use jujube_lib::rewrite::{back_out_commit, merge_commit_trees, rebase_commit}; use jujube_lib::settings::UserSettings; -use jujube_lib::store::{CommitId, StoreError, Timestamp, TreeValue}; +use jujube_lib::store::{CommitId, Timestamp, TreeValue}; use jujube_lib::store_wrapper::StoreWrapper; use jujube_lib::tree::Tree; use jujube_lib::trees::Diff; use jujube_lib::working_copy::{CheckoutStats, WorkingCopy}; -use jujube_lib::{conflicts, files, git}; +use jujube_lib::{conflicts, files, git, revset}; use pest::Parser; use self::chrono::{FixedOffset, TimeZone, Utc}; @@ -93,6 +94,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: RevsetError) -> Self { + CommandError::UserError(format!("{}", err)) + } +} + fn get_repo(ui: &Ui, matches: &ArgMatches) -> Result, CommandError> { let wc_path_str = matches.value_of("repository").unwrap(); let wc_path = ui.cwd().join(wc_path_str); @@ -105,19 +112,6 @@ fn get_repo(ui: &Ui, matches: &ArgMatches) -> Result, CommandE } } -fn resolve_commit_id_prefix( - repo: &ReadonlyRepo, - prefix: &HexPrefix, -) -> Result { - match repo.index().resolve_prefix(prefix) { - PrefixResolution::NoMatch => Err(CommandError::UserError(String::from("No such commit"))), - PrefixResolution::AmbiguousMatch => { - Err(CommandError::UserError(String::from("Ambiguous prefix"))) - } - PrefixResolution::SingleMatch(id) => Ok(id), - } -} - fn resolve_revision_arg( ui: &Ui, repo: &mut ReadonlyRepo, @@ -131,18 +125,19 @@ fn resolve_single_rev( repo: &mut ReadonlyRepo, revision_str: &str, ) -> Result { + // If we're looking up the working copy commit ("@"), make sure that it is up to + // date (the lib crate only looks at the checkout in the view). if revision_str == "@" { let owned_wc = repo.working_copy().clone(); - let wc = owned_wc.lock().unwrap(); // TODO: Avoid committing every time this function is called. - Ok(wc.commit(ui.settings(), repo)) - } else if revision_str == "@^" { + owned_wc.lock().unwrap().commit(ui.settings(), repo); + } + + if revision_str == "@^" { let commit = repo.store().get_commit(repo.view().checkout()).unwrap(); assert!(commit.is_open()); let parents = commit.parents(); Ok(parents[0].clone()) - } else if revision_str == "root" { - Ok(repo.store().root_commit()) } else if revision_str.starts_with("desc(") && revision_str.ends_with(')') { let needle = revision_str[5..revision_str.len() - 1].to_string(); let mut matches = vec![]; @@ -160,21 +155,7 @@ fn resolve_single_rev( .pop() .ok_or_else(|| CommandError::UserError(String::from("No matching commit"))) } else { - if let Ok(binary_commit_id) = hex::decode(revision_str) { - let commit_id = CommitId(binary_commit_id); - match repo.store().get_commit(&commit_id) { - Ok(commit) => return Ok(commit), - Err(StoreError::NotFound) => {} // fall through - Err(err) => { - return Err(CommandError::InternalError(format!( - "Failed to read commit: {}", - err - ))) - } - } - } - let id = resolve_commit_id_prefix(repo, &HexPrefix::new(revision_str.to_owned()))?; - Ok(repo.store().get_commit(&id).unwrap()) + Ok(revset::resolve_symbol(repo.as_repo_ref(), revision_str)?) } }