correctly handle case conflicting reenames

Summary:
Correctly handles case conflicting renames (only change in casing).
-  path can now be removed from `CaseConlictingTrie`
- `check_case_conflicts` operates on `BonsaiChangeset` in pushrebase logic

Reviewed By: StanislavGlebik

Differential Revision: D10447522

fbshipit-source-id: d5342e7aa48154debee123b38bf3168e3371baa6

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<MPath>,
+    client_bcs: Vec<BonsaiChangeset>,
 ) -> BoxFuture<PushrebaseSuccessResult, PushrebaseError> {
     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<BlobRepo>,
+    ancestor: ChangesetId,
+    descendant: ChangesetId,
+) -> impl Future<Item = Vec<BonsaiChangeset>, 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<BlobRepo>,
     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(|| {

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<MPathElement, CaseConflictTrie>,
     lowercase: HashSet<String>,
 }
@@ -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<Item = &'p MPathElement>>(&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<Item = &'p MPathElement>>(&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<P> FromIterator<P> for CaseConflictTrie
-where
-    P: ::std::borrow::Borrow<MPath>,
-{
-    fn from_iter<I: IntoIterator<Item = P>>(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<MPath>;
+}
+
+impl<'a> CaseConflictTrieUpdate for &'a MPath {
+    fn apply(self, trie: &mut CaseConflictTrie) -> Option<MPath> {
+        if !trie.add(self) {
+            return Some(self.clone());
+        } else {
+            None
         }
-        trie
+    }
+}
+
+impl CaseConflictTrieUpdate for MPath {
+    fn apply(self, trie: &mut CaseConflictTrie) -> Option<MPath> {
+        if !trie.add(&self) {
+            return Some(self);
+        } else {
+            None
+        }
+    }
+}
+
+impl<'a> CaseConflictTrieUpdate for &'a BonsaiChangeset {
+    fn apply(self, trie: &mut CaseConflictTrie) -> Option<MPath> {
+        // 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<P, I>(iter: I) -> Option<MPath>
 where
-    P: ::std::borrow::Borrow<MPath>,
+    P: CaseConflictTrieUpdate,
     I: IntoIterator<Item = P>,
 {
     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<P> FromIterator<P> for CaseConflictTrie
+where
+    P: CaseConflictTrieUpdate,
+{
+    fn from_iter<I: IntoIterator<Item = P>>(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(),