diff --git a/bundle2-resolver/src/pushrebase.rs b/bundle2-resolver/src/pushrebase.rs index 2d32592d46..be0e305def 100644 --- a/bundle2-resolver/src/pushrebase.rs +++ b/bundle2-resolver/src/pushrebase.rs @@ -114,11 +114,21 @@ pub fn do_pushrebase( cloned!(repo); move |(head, root)| { // Calculate client changed files only once, since they won't change - find_changed_files(&repo, root, head, /* reject_merges */ false).and_then( - move |client_cf| { - rebase_in_loop(repo, config, onto_bookmark, head, root, client_cf) - }, - ) + ( + find_changed_files(&repo, root, head, /* reject_merges */ false), + fetch_bonsai_range(&repo, root, head), + ).into_future() + .and_then(move |(client_cf, client_bcs)| { + rebase_in_loop( + repo, + config, + onto_bookmark, + head, + root, + client_cf, + client_bcs, + ) + }) } }) } @@ -130,24 +140,35 @@ fn rebase_in_loop( head: ChangesetId, root: ChangesetId, client_cf: Vec, + client_bcs: Vec, ) -> BoxFuture { loop_fn( (root.clone(), 0), move |(latest_rebase_attempt, retry_num)| { get_bookmark_value(&repo, &onto_bookmark).and_then({ - cloned!(client_cf, onto_bookmark, repo, config); + cloned!(client_cf, client_bcs, onto_bookmark, repo, config); move |bookmark_val| { - find_changed_files( - &repo, - latest_rebase_attempt.clone(), - bookmark_val, - /* reject_merges */ true, - ).and_then(|server_cf| { - match check_case_conflicts(server_cf.iter().chain(&client_cf)) { - Some(path) => Err(PushrebaseError::PotentialCaseConflict(path)), - None => intersect_changed_files(server_cf, client_cf), - } - }) + fetch_bonsai_range(&repo, latest_rebase_attempt, bookmark_val) + .and_then(move |server_bcs| { + match check_case_conflicts( + server_bcs.iter().rev().chain(client_bcs.iter().rev()), + ) { + Some(path) => Err(PushrebaseError::PotentialCaseConflict(path)), + None => Ok(()), + } + }) + .and_then({ + cloned!(repo); + move |()| { + find_changed_files( + &repo, + latest_rebase_attempt.clone(), + bookmark_val, + /* reject_merges */ true, + ) + } + }) + .and_then(|server_cf| intersect_changed_files(server_cf, client_cf)) .and_then(move |()| { do_rebase( repo, @@ -315,6 +336,20 @@ fn find_changed_files_between_manfiests( .from_err() } +// from larger generation number to smaller +fn fetch_bonsai_range( + repo: &Arc, + ancestor: ChangesetId, + descendant: ChangesetId, +) -> impl Future, Error = PushrebaseError> { + cloned!(repo); + RangeNodeStream::new(&repo, ancestor, descendant) + .map(move |id| repo.get_bonsai_changeset(id)) + .buffered(100) + .collect() + .from_err() +} + fn find_changed_files( repo: &Arc, ancestor: ChangesetId, @@ -900,6 +935,96 @@ mod tests { }); } + #[test] + fn pushrebase_caseconflicting_rename() { + async_unit::tokio_unit_test(|| { + let repo = linear::getrepo(None); + let root = repo.get_bonsai_from_hg(&HgChangesetId::from_str( + "2d7d4ba9ce0a6ffd222de7785b249ead9c51c536", + ).unwrap()) + .wait() + .unwrap() + .unwrap(); + + let bcs_id_1 = create_commit( + repo.clone(), + vec![root], + store_files(btreemap!{"FILE" => Some("file")}, repo.clone()), + ); + let bcs_id_2 = create_commit( + repo.clone(), + vec![bcs_id_1], + store_files(btreemap!{"FILE" => None}, repo.clone()), + ); + let bcs_id_3 = create_commit( + repo.clone(), + vec![bcs_id_2], + store_files(btreemap!{"file" => Some("file")}, repo.clone()), + ); + let hgcss = vec![ + repo.get_hg_from_bonsai_changeset(bcs_id_1).wait().unwrap(), + repo.get_hg_from_bonsai_changeset(bcs_id_2).wait().unwrap(), + repo.get_hg_from_bonsai_changeset(bcs_id_3).wait().unwrap(), + ]; + + let book = Bookmark::new("master").unwrap(); + set_bookmark( + repo.clone(), + &book, + "a5ffa77602a066db7d5cfb9fb5823a0895717c5a", + ); + + do_pushrebase(Arc::new(repo), Default::default(), book, hgcss) + .wait() + .expect("push-rebase failed"); + }) + } + + #[test] + fn pushrebase_caseconflicting_dirs() { + async_unit::tokio_unit_test(|| { + let repo = linear::getrepo(None); + let root = repo.get_bonsai_from_hg(&HgChangesetId::from_str( + "2d7d4ba9ce0a6ffd222de7785b249ead9c51c536", + ).unwrap()) + .wait() + .unwrap() + .unwrap(); + + let bcs_id_1 = create_commit( + repo.clone(), + vec![root], + store_files( + btreemap!{"DIR/a" => Some("a"), "DIR/b" => Some("b")}, + repo.clone(), + ), + ); + let bcs_id_2 = create_commit( + repo.clone(), + vec![bcs_id_1], + store_files( + btreemap!{"dir/a" => Some("a"), "DIR/a" => None, "DIR/b" => None}, + repo.clone(), + ), + ); + let hgcss = vec![ + repo.get_hg_from_bonsai_changeset(bcs_id_1).wait().unwrap(), + repo.get_hg_from_bonsai_changeset(bcs_id_2).wait().unwrap(), + ]; + + let book = Bookmark::new("master").unwrap(); + set_bookmark( + repo.clone(), + &book, + "a5ffa77602a066db7d5cfb9fb5823a0895717c5a", + ); + + do_pushrebase(Arc::new(repo), Default::default(), book, hgcss) + .wait() + .expect("push-rebase failed"); + }) + } + #[test] fn pushrebase_recursion_limit() { async_unit::tokio_unit_test(|| { diff --git a/mononoke-types/src/path.rs b/mononoke-types/src/path.rs index 5ee1fad75d..96d0a62890 100644 --- a/mononoke-types/src/path.rs +++ b/mononoke-types/src/path.rs @@ -20,6 +20,7 @@ use heapsize::HeapSizeOf; use quickcheck::{Arbitrary, Gen}; +use bonsai_changeset::BonsaiChangeset; use errors::*; use thrift; @@ -702,7 +703,7 @@ impl fmt::Debug for MPath { } } -struct CaseConflictTrie { +pub struct CaseConflictTrie { children: HashMap, lowercase: HashSet, } @@ -715,6 +716,10 @@ impl CaseConflictTrie { } } + fn is_empty(&self) -> bool { + self.children.is_empty() + } + /// Returns `true` if element was added successfully, or `false` /// if trie already contains case conflicting entry. fn add<'p, P: IntoIterator>(&mut self, path: P) -> bool { @@ -742,37 +747,103 @@ impl CaseConflictTrie { } } } + + /// Remove path from a trie + /// + /// Returns `true` if path was removed, otherwise `false`. + fn remove<'p, P: IntoIterator>(&mut self, path: P) -> bool { + let mut iter = path.into_iter(); + match iter.next() { + None => true, + Some(element) => { + let (found, remove) = match self.children.get_mut(&element) { + None => return false, + Some(child) => (child.remove(iter), child.is_empty()), + }; + if remove { + self.children.remove(&element); + if let Ok(ref element) = String::from_utf8(element.to_bytes()) { + self.lowercase.remove(&element.to_lowercase()); + } + } + found + } + } + } } -impl

FromIterator

for CaseConflictTrie -where - P: ::std::borrow::Borrow, -{ - fn from_iter>(iter: I) -> Self { - let mut trie = CaseConflictTrie::new(); - for mpath in iter { - trie.add(mpath.borrow()); +pub trait CaseConflictTrieUpdate { + fn apply(self, trie: &mut CaseConflictTrie) -> Option; +} + +impl<'a> CaseConflictTrieUpdate for &'a MPath { + fn apply(self, trie: &mut CaseConflictTrie) -> Option { + if !trie.add(self) { + return Some(self.clone()); + } else { + None } - trie + } +} + +impl CaseConflictTrieUpdate for MPath { + fn apply(self, trie: &mut CaseConflictTrie) -> Option { + if !trie.add(&self) { + return Some(self); + } else { + None + } + } +} + +impl<'a> CaseConflictTrieUpdate for &'a BonsaiChangeset { + fn apply(self, trie: &mut CaseConflictTrie) -> Option { + // we need apply deletion first + for (path, change) in self.file_changes() { + if change.is_none() { + trie.remove(path); + } + } + for (path, change) in self.file_changes() { + if change.is_some() { + if !trie.add(path) { + return Some(path.clone()); + } + } + } + return None; } } /// Returns first path that would introduce a case-conflict, if any pub fn check_case_conflicts(iter: I) -> Option where - P: ::std::borrow::Borrow, + P: CaseConflictTrieUpdate, I: IntoIterator, { let mut trie = CaseConflictTrie::new(); - for path in iter { - let path = path.borrow(); - if !trie.add(path) { - return Some(path.clone()); + for update in iter { + let conflict = update.apply(&mut trie); + if conflict.is_some() { + return conflict; } } None } +impl

FromIterator

for CaseConflictTrie +where + P: CaseConflictTrieUpdate, +{ + fn from_iter>(iter: I) -> Self { + let mut trie = CaseConflictTrie::new(); + for update in iter { + update.apply(&mut trie); + } + trie + } +} + #[cfg(test)] mod test { use quickcheck::TestResult; @@ -960,6 +1031,8 @@ mod test { assert!(trie.add(&MPath::new("a/b/c").unwrap())); assert!(trie.add(&MPath::new("a/B/d").unwrap()) == false); assert!(trie.add(&MPath::new("a/b/C").unwrap()) == false); + assert!(trie.remove(&MPath::new("a/b/c").unwrap())); + assert!(trie.add(&MPath::new("a/B/c").unwrap())); let paths = vec![ MPath::new("a/b/c").unwrap(),