From 09a423f0d4a45d535a970023e7ab005c35992b9d Mon Sep 17 00:00:00 2001
From: Fintan Halpenny <fintan.halpenny@gmail.com>
Date: Thu, 29 Dec 2022 13:18:39 +0000
Subject: [PATCH] surf: fix comparison of tree entries

Comparing tree entries would only take into account their names. While
this is useful, it is also useful to compare by their entry kind
first, where trees take precedence over blobs.

A test is added to ensure the ordering is respected. While testing, it
was noticed that the entries aren't sorted when be passed in. When a
`Tree` is constructed it will now sort the entries first, ensure they
are given back in sorted order.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
---
 radicle-surf/src/object/tree.rs | 45 ++++++++++++++++++++++++++++-----
 radicle-surf/t/src/source.rs    | 26 +++++++++++++++++++
 2 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/radicle-surf/src/object/tree.rs b/radicle-surf/src/object/tree.rs
index ee142a0..3378edd 100644
--- a/radicle-surf/src/object/tree.rs
+++ b/radicle-surf/src/object/tree.rs
@@ -36,14 +36,16 @@ use crate::{fs, object::commit};
 pub struct Tree {
     /// The object id of this tree.
     id: Oid,
+    /// The first descendant entries for this tree.
     entries: Vec<TreeEntry>,
     /// The commit object that created this tree object.
     commit: commit::Header,
 }
 
 impl Tree {
-    /// Creates a new tree.
-    pub(crate) fn new(id: Oid, entries: Vec<TreeEntry>, commit: commit::Header) -> Self {
+    /// Creates a new tree, ensuring the `entries` are sorted.
+    pub(crate) fn new(id: Oid, mut entries: Vec<TreeEntry>, commit: commit::Header) -> Self {
+        entries.sort();
         Self {
             id,
             entries,
@@ -106,13 +108,40 @@ impl Serialize for Tree {
     }
 }
 
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum Entry {
     Tree(Oid),
     Blob(Oid),
 }
 
-/// Entry in a Tree result.
+impl PartialOrd for Entry {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Ord for Entry {
+    fn cmp(&self, other: &Self) -> Ordering {
+        match (self, other) {
+            (Entry::Tree(_), Entry::Tree(_)) => Ordering::Equal,
+            (Entry::Tree(_), Entry::Blob(_)) => Ordering::Less,
+            (Entry::Blob(_), Entry::Tree(_)) => Ordering::Greater,
+            (Entry::Blob(_), Entry::Blob(_)) => Ordering::Equal,
+        }
+    }
+}
+
+/// An entry that can be found in a tree.
+///
+/// Each entry consists of a [`TreeEntry::name`], its kind -- given as [`Entry`]
+/// -- and the header of its associated [`TreeEntry::commit`].
+///
+/// # Ordering
+///
+/// The ordering of a `TreeEntry` is first by its [`Entry`] kind where
+/// [`Entry::Tree`]s come before [`Entry::Blob`]. If both kinds are
+/// equal then they are next compared by the lexicographical ordering
+/// of their `name`s.
 #[derive(Clone, Debug)]
 pub struct TreeEntry {
     name: String,
@@ -158,7 +187,9 @@ impl TreeEntry {
 // To support `sort`.
 impl Ord for TreeEntry {
     fn cmp(&self, other: &Self) -> Ordering {
-        self.name.cmp(&other.name)
+        self.entry
+            .cmp(&other.entry)
+            .then(self.name.cmp(&other.name))
     }
 }
 
@@ -170,7 +201,7 @@ impl PartialOrd for TreeEntry {
 
 impl PartialEq for TreeEntry {
     fn eq(&self, other: &Self) -> bool {
-        self.name == other.name
+        self.entry == other.entry && self.name == other.name
     }
 }
 
@@ -188,7 +219,7 @@ impl From<fs::Entry> for Entry {
 #[cfg(feature = "serde")]
 impl Serialize for TreeEntry {
     /// Sample output:
-    /// ```
+    /// ```json
     ///  {
     ///     "kind": "blob",
     ///     "lastCommit": {
diff --git a/radicle-surf/t/src/source.rs b/radicle-surf/t/src/source.rs
index f0cfd3c..7fb75e2 100644
--- a/radicle-surf/t/src/source.rs
+++ b/radicle-surf/t/src/source.rs
@@ -1,3 +1,5 @@
+use std::path::PathBuf;
+
 use git_ref_format::refname;
 use radicle_surf::git::Repository;
 use serde_json::json;
@@ -139,3 +141,27 @@ fn repo_blob() {
     // Verify as_bytes.
     assert_eq!(content.as_bytes().len(), content.size());
 }
+
+#[test]
+fn tree_ordering() {
+    let repo = Repository::open(GIT_PLATINUM).unwrap();
+    let tree = repo
+        .tree(refname!("refs/heads/master"), &PathBuf::new())
+        .unwrap();
+    assert_eq!(
+        tree.entries()
+            .iter()
+            .map(|entry| entry.name().to_string())
+            .collect::<Vec<_>>(),
+        vec![
+            "bin".to_string(),
+            "special".to_string(),
+            "src".to_string(),
+            "text".to_string(),
+            "this".to_string(),
+            ".i-am-well-hidden".to_string(),
+            ".i-too-am-hidden".to_string(),
+            "README.md".to_string(),
+        ]
+    );
+}