From 61d87fe296cdc6817ffca79095cf8cdb129c74dc Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Mon, 13 Nov 2023 17:13:42 -0800
Subject: [PATCH] merged_tree: make `TreeEntriesIterator` not self-referential
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

While importing the `ouroboros` crate and the `aliasable` crate it
depends on, the "unsafe Rust reviewer" expressed some concern that
they contain a lot of unsafe code that's hard to review. We can avoid
the unsafe code altogether by making `TreeEntriesIterator` not
self-refential. Instead, we can collect the matching entries in an
individual tree up front. It does have some performance cost:


```
❯ hyperfine --warmup 3 --runs 30 \
      '/tmp/jj-before --ignore-working-copy files -r v6.0' \
      '/tmp/jj-after --ignore-working-copy files -r v6.0'
Benchmark 1: /tmp/jj-before --ignore-working-copy files -r v6.0
  Time (mean ± σ):     461.4 ms ±  14.3 ms    [User: 232.1 ms, System: 229.4 ms]
  Range (min … max):   443.4 ms … 496.3 ms    30 runs

Benchmark 2: /tmp/jj-after --ignore-working-copy files -r v6.0
  Time (mean ± σ):     482.0 ms ±  14.3 ms    [User: 257.2 ms, System: 224.9 ms]
  Range (min … max):   461.8 ms … 513.3 ms    30 runs

Summary
  '/tmp/jj-before --ignore-working-copy files -r v6.0' ran
    1.04 ± 0.04 times faster than '/tmp/jj-after --ignore-working-copy files -r v6.0'
```

I think that's acceptable.
---
 lib/src/merged_tree.rs | 49 +++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs
index 766e9bff4..c61d3d9b1 100644
--- a/lib/src/merged_tree.rs
+++ b/lib/src/merged_tree.rs
@@ -575,25 +575,37 @@ pub struct TreeEntriesIterator<'matcher> {
     matcher: &'matcher dyn Matcher,
 }
 
-#[ouroboros::self_referencing]
 struct TreeEntriesDirItem {
     tree: MergedTree,
-    #[borrows(tree)]
-    #[not_covariant]
-    entry_iterator: TreeEntriesNonRecursiveIterator<'this>,
+    entries: Vec<(RepoPath, MergedTreeValue)>,
 }
 
-impl From<MergedTree> for TreeEntriesDirItem {
-    fn from(tree: MergedTree) -> Self {
-        TreeEntriesDirItem::new(tree, |tree| tree.entries_non_recursive())
+impl TreeEntriesDirItem {
+    fn new(tree: MergedTree, matcher: &dyn Matcher) -> Self {
+        let mut entries = vec![];
+        let dir = tree.dir();
+        for (name, value) in tree.entries_non_recursive() {
+            let path = dir.join(name);
+            let value = value.to_merge();
+            if value.is_tree() {
+                // TODO: Handle the other cases (specific files and trees)
+                if matcher.visit(&path).is_nothing() {
+                    continue;
+                }
+            } else if !matcher.matches(&path) {
+                continue;
+            }
+            entries.push((path, value));
+        }
+        entries.reverse();
+        TreeEntriesDirItem { tree, entries }
     }
 }
 
 impl<'matcher> TreeEntriesIterator<'matcher> {
     fn new(tree: MergedTree, matcher: &'matcher dyn Matcher) -> Self {
-        // TODO: Restrict walk according to Matcher::visit()
         Self {
-            stack: vec![TreeEntriesDirItem::from(tree)],
+            stack: vec![TreeEntriesDirItem::new(tree, matcher)],
             matcher,
         }
     }
@@ -604,19 +616,16 @@ impl Iterator for TreeEntriesIterator<'_> {
 
     fn next(&mut self) -> Option<Self::Item> {
         while let Some(top) = self.stack.last_mut() {
-            if let (tree, Some((name, value))) = top.with_mut(|x| (x.tree, x.entry_iterator.next()))
-            {
-                let path = tree.dir().join(name);
-                let value = value.to_merge();
+            if let Some((path, value)) = top.entries.pop() {
                 if value.is_tree() {
-                    // TODO: Handle the other cases (specific files and trees)
-                    if self.matcher.visit(&path).is_nothing() {
-                        continue;
-                    }
-                    let tree_merge = value.to_tree_merge(tree.store(), &path).unwrap().unwrap();
+                    let tree_merge = value
+                        .to_tree_merge(top.tree.store(), &path)
+                        .unwrap()
+                        .unwrap();
                     let merged_tree = MergedTree::Merge(tree_merge);
-                    self.stack.push(TreeEntriesDirItem::from(merged_tree));
-                } else if self.matcher.matches(&path) {
+                    self.stack
+                        .push(TreeEntriesDirItem::new(merged_tree, self.matcher));
+                } else {
                     return Some((path, value));
                 }
             } else {