From f6a9523b752fb23e2740a2be2eae78e1d765bc55 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 29 May 2021 23:52:50 -0700 Subject: [PATCH] revsets: resolve symbol as change id if nothing else matches This patch makes it so we attempt to resolve a symbol as the non-obsolete commits in a change id if all other resolutions fail. This addresses issue #15. I decided to not require any operator for looking up by change id. I want to make it as easy as possible to use change ids instead of commit ids to see how well it works to interact mostly with change ids instead of commit ids (I'll try to test that by using it myself). --- lib/src/evolution.rs | 54 +++++++++++++++++- lib/src/revset.rs | 31 ++++++++++- lib/tests/test_revset.rs | 115 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 197 insertions(+), 3 deletions(-) diff --git a/lib/src/evolution.rs b/lib/src/evolution.rs index 51a07f9f0..01784a273 100644 --- a/lib/src/evolution.rs +++ b/lib/src/evolution.rs @@ -18,7 +18,7 @@ use std::sync::Arc; use crate::commit::Commit; use crate::commit_builder::CommitBuilder; use crate::dag_walk::{bfs, closest_common_node, leaves}; -use crate::index::IndexPosition; +use crate::index::{HexPrefix, IndexPosition, PrefixResolution}; use crate::repo::{MutableRepo, ReadonlyRepo, RepoRef}; use crate::repo_path::RepoPath; use crate::rewrite::{merge_commit_trees, rebase_commit}; @@ -139,6 +139,28 @@ impl State { .map_or(false, |non_obsoletes| non_obsoletes.len() > 1) } + // TODO: We should probably add a change id table to the commit index and move + // this there + fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + let mut result = PrefixResolution::NoMatch; + for change_id in self.non_obsoletes_by_changeid.keys() { + if change_id.hex().starts_with(prefix.hex()) { + if result != PrefixResolution::NoMatch { + return PrefixResolution::AmbiguousMatch; + } + result = PrefixResolution::SingleMatch(change_id.clone()); + } + } + result + } + + fn non_obsoletes(&self, change_id: &ChangeId) -> HashSet { + self.non_obsoletes_by_changeid + .get(change_id) + .cloned() + .unwrap_or_else(HashSet::new) + } + fn add_commit(&mut self, commit: &Commit) { self.add_commit_data( commit.id(), @@ -339,6 +361,13 @@ impl EvolutionRef<'_> { } } + pub fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + match self { + EvolutionRef::Readonly(evolution) => evolution.resolve_change_id_prefix(prefix), + EvolutionRef::Mutable(evolution) => evolution.resolve_change_id_prefix(prefix), + } + } + pub fn is_divergent(&self, change_id: &ChangeId) -> bool { match self { EvolutionRef::Readonly(evolution) => evolution.is_divergent(change_id), @@ -346,6 +375,13 @@ impl EvolutionRef<'_> { } } + pub fn non_obsoletes(&self, change_id: &ChangeId) -> HashSet { + match self { + EvolutionRef::Readonly(evolution) => evolution.non_obsoletes(change_id), + EvolutionRef::Mutable(evolution) => evolution.non_obsoletes(change_id), + } + } + /// Given a current parent, finds the new parent candidates. If the current /// parent is not obsolete, then a singleton set of that commit will be /// returned. @@ -402,6 +438,14 @@ impl ReadonlyEvolution { self.state.is_divergent(change_id) } + pub fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + self.state.resolve_change_id_prefix(prefix) + } + + pub fn non_obsoletes(&self, change_id: &ChangeId) -> HashSet { + self.state.non_obsoletes(change_id) + } + pub fn new_parent(&self, repo: RepoRef, old_parent_id: &CommitId) -> HashSet { self.state.new_parent(repo, old_parent_id) } @@ -434,6 +478,14 @@ impl MutableEvolution { self.state.is_divergent(change_id) } + pub fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + self.state.resolve_change_id_prefix(prefix) + } + + pub fn non_obsoletes(&self, change_id: &ChangeId) -> HashSet { + self.state.non_obsoletes(change_id) + } + pub fn new_parent(&self, repo: RepoRef, old_parent_id: &CommitId) -> HashSet { self.state.new_parent(repo, old_parent_id) } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 5e7aa45d2..854c7bf94 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -34,6 +34,8 @@ pub enum RevsetError { NoSuchRevision(String), #[error("Commit id prefix \"{0}\" is ambiguous")] AmbiguousCommitIdPrefix(String), + #[error("Change id prefix \"{0}\" is ambiguous")] + AmbiguousChangeIdPrefix(String), #[error("Unexpected error from store: {0}")] StoreError(#[from] StoreError), } @@ -74,8 +76,29 @@ 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> { + if let Some(hex_prefix) = HexPrefix::new(change_id_prefix.to_owned()) { + let evolution = repo.evolution(); + match evolution.resolve_change_id_prefix(&hex_prefix) { + PrefixResolution::NoMatch => { + Err(RevsetError::NoSuchRevision(change_id_prefix.to_owned())) + } + PrefixResolution::AmbiguousMatch => Err(RevsetError::AmbiguousChangeIdPrefix( + change_id_prefix.to_owned(), + )), + PrefixResolution::SingleMatch(change_id) => { + Ok(evolution.non_obsoletes(&change_id).into_iter().collect()) + } + } + } else { + Err(RevsetError::NoSuchRevision(change_id_prefix.to_owned())) + } +} + pub fn resolve_symbol(repo: RepoRef, symbol: &str) -> Result, RevsetError> { - // TODO: Support change ids. if symbol == "@" { Ok(vec![repo.view().checkout().clone()]) } else if symbol == "root" { @@ -93,6 +116,12 @@ 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); + if !matches!(change_id_result, Err(RevsetError::NoSuchRevision(_))) { + return change_id_result; + } + Err(RevsetError::NoSuchRevision(symbol.to_owned())) } } diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 090224645..076259098 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -16,8 +16,8 @@ use jujutsu_lib::commit_builder::CommitBuilder; use jujutsu_lib::repo::RepoRef; use jujutsu_lib::revset::{parse, resolve_symbol, RevsetError}; use jujutsu_lib::store::{CommitId, MillisSinceEpoch, Signature, Timestamp}; -use jujutsu_lib::testutils; use jujutsu_lib::testutils::CommitGraphBuilder; +use jujutsu_lib::{git, testutils}; use test_case::test_case; #[test_case(false ; "local store")] @@ -119,6 +119,119 @@ fn test_resolve_symbol_commit_id() { tx.discard(); } +#[test] +fn test_resolve_symbol_change_id() { + let settings = testutils::user_settings(); + // Test only with git so we can get predictable change ids + let (_temp_dir, repo) = testutils::init_repo(&settings, true); + + let git_repo = repo.store().git_repo().unwrap(); + // Add some commits that will end up having change ids with common prefixes + let empty_tree_id = git_repo.treebuilder(None).unwrap().write().unwrap(); + let git_author = git2::Signature::new( + "git author", + "git.author@example.com", + &git2::Time::new(1000, 60), + ) + .unwrap(); + let git_committer = git2::Signature::new( + "git committer", + "git.committer@example.com", + &git2::Time::new(2000, -480), + ) + .unwrap(); + let git_tree = git_repo.find_tree(empty_tree_id).unwrap(); + let mut git_commit_ids = vec![]; + for i in &[133, 664, 840] { + let git_commit_id = git_repo + .commit( + Some(&format!("refs/heads/branch{}", i)), + &git_author, + &git_committer, + &format!("test {}", i), + &git_tree, + &[], + ) + .unwrap(); + git_commit_ids.push(git_commit_id); + } + + let mut tx = repo.start_transaction("test"); + git::import_refs(tx.mut_repo(), &git_repo).unwrap(); + let repo = tx.commit(); + + // Test the test setup + assert_eq!( + hex::encode(git_commit_ids[0].as_bytes()), + // "04e12a5467bba790efb88a9870894ec208b16bf1" reversed + "8fd68d104372910e19511df709e5dde62a548720" + ); + assert_eq!( + hex::encode(git_commit_ids[1].as_bytes()), + // "040b3ba3a51d8edbc4c5855cbd09de71d4c29cca" reversed + "5339432b8e7b90bd3aa1a323db71b8a5c5dcd020" + ); + assert_eq!( + hex::encode(git_commit_ids[2].as_bytes()), + // "04e1c7082e4e34f3f371d8a1a46770b861b9b547" reversed + "e2ad9d861d0ee625851b8ecfcf2c727410e38720" + ); + + // Test lookup by full change id + let repo_ref = repo.as_repo_ref(); + assert_eq!( + resolve_symbol(repo_ref, "04e12a5467bba790efb88a9870894ec2"), + Ok(vec![CommitId::from_hex( + "8fd68d104372910e19511df709e5dde62a548720" + )]) + ); + assert_eq!( + resolve_symbol(repo_ref, "040b3ba3a51d8edbc4c5855cbd09de71"), + Ok(vec![CommitId::from_hex( + "5339432b8e7b90bd3aa1a323db71b8a5c5dcd020" + )]) + ); + assert_eq!( + resolve_symbol(repo_ref, "04e1c7082e4e34f3f371d8a1a46770b8"), + Ok(vec![CommitId::from_hex( + "e2ad9d861d0ee625851b8ecfcf2c727410e38720" + )]) + ); + + // Test change id prefix + assert_eq!( + resolve_symbol(repo_ref, "04e12"), + Ok(vec![CommitId::from_hex( + "8fd68d104372910e19511df709e5dde62a548720" + )]) + ); + assert_eq!( + resolve_symbol(repo_ref, "04e1c"), + Ok(vec![CommitId::from_hex( + "e2ad9d861d0ee625851b8ecfcf2c727410e38720" + )]) + ); + assert_eq!( + resolve_symbol(repo_ref, "04e1"), + Err(RevsetError::AmbiguousChangeIdPrefix("04e1".to_string())) + ); + assert_eq!( + resolve_symbol(repo_ref, ""), + // Commit id is checked first, so this is considered an ambiguous commit id + Err(RevsetError::AmbiguousCommitIdPrefix("".to_string())) + ); + assert_eq!( + resolve_symbol(repo_ref, "04e13"), + Err(RevsetError::NoSuchRevision("04e13".to_string())) + ); + + // Test non-hex string + assert_eq!( + resolve_symbol(repo_ref, "foo"), + Err(RevsetError::NoSuchRevision("foo".to_string())) + ); +} + #[test_case(false ; "local store")] #[test_case(true ; "git store")] fn test_resolve_symbol_checkout(use_git: bool) {