Skip to content

Commit ca37915

Browse files
committed
fix: sorting of tree entries is now according to specificiation.
This means tree entries are compared as if they had a / appended to it.
1 parent d92a588 commit ca37915

File tree

5 files changed

+70
-20
lines changed

5 files changed

+70
-20
lines changed

gix-object/src/tree/mod.rs

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ pub struct EntryRef<'a> {
7272
pub filename: &'a BStr,
7373
/// The id of the object representing the entry.
7474
// TODO: figure out how these should be called. id or oid? It's inconsistent around the codebase.
75-
// Answer: make it 'id', as in `git2`
75+
// Answer: make it 'id', as in `git2`
7676
#[cfg_attr(feature = "serde", serde(borrow))]
7777
pub oid: &'a gix_hash::oid,
7878
}
@@ -84,11 +84,14 @@ impl<'a> PartialOrd for EntryRef<'a> {
8484
}
8585

8686
impl<'a> Ord for EntryRef<'a> {
87-
/// Entries compare by the common portion of the filename. This is critical for proper functioning of algorithms working on trees.
88-
/// Doing it like this is needed for compatibility with older, potentially broken(?) trees.
89-
fn cmp(&self, other: &Self) -> Ordering {
90-
let len = self.filename.len().min(other.filename.len());
91-
self.filename[..len].cmp(&other.filename[..len])
87+
fn cmp(&self, b: &Self) -> Ordering {
88+
let a = self;
89+
let common = a.filename.len().min(b.filename.len());
90+
a.filename[..common].cmp(&b.filename[..common]).then_with(|| {
91+
let a = a.filename.get(common).or_else(|| a.mode.is_tree().then_some(&b'/'));
92+
let b = b.filename.get(common).or_else(|| b.mode.is_tree().then_some(&b'/'));
93+
a.cmp(&b)
94+
})
9295
}
9396
}
9497

@@ -111,12 +114,14 @@ impl PartialOrd for Entry {
111114
}
112115

113116
impl Ord for Entry {
114-
/// Entries compare by the common portion of the filename. This is critical for proper functioning of algorithms working on trees.
115-
fn cmp(&self, other: &Self) -> Ordering {
116-
let common_len = self.filename.len().min(other.filename.len());
117-
self.filename[..common_len]
118-
.cmp(&other.filename[..common_len])
119-
.then_with(|| self.filename.len().cmp(&other.filename.len()))
117+
fn cmp(&self, b: &Self) -> Ordering {
118+
let a = self;
119+
let common = a.filename.len().min(b.filename.len());
120+
a.filename[..common].cmp(&b.filename[..common]).then_with(|| {
121+
let a = a.filename.get(common).or_else(|| a.mode.is_tree().then_some(&b'/'));
122+
let b = b.filename.get(common).or_else(|| b.mode.is_tree().then_some(&b'/'));
123+
a.cmp(&b)
124+
})
120125
}
121126
}
122127

gix-object/src/tree/write.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,16 @@ impl crate::WriteTo for Tree {
5353
Ok(())
5454
}
5555

56+
fn kind(&self) -> Kind {
57+
Kind::Tree
58+
}
59+
5660
fn size(&self) -> usize {
5761
self.entries
5862
.iter()
5963
.map(|Entry { mode, filename, oid }| mode.as_bytes().len() + 1 + filename.len() + 1 + oid.as_bytes().len())
6064
.sum()
6165
}
62-
63-
fn kind(&self) -> Kind {
64-
Kind::Tree
65-
}
6666
}
6767

6868
/// Serialization
@@ -96,6 +96,10 @@ impl<'a> crate::WriteTo for TreeRef<'a> {
9696
Ok(())
9797
}
9898

99+
fn kind(&self) -> Kind {
100+
Kind::Tree
101+
}
102+
99103
fn size(&self) -> usize {
100104
self.entries
101105
.iter()
@@ -104,8 +108,4 @@ impl<'a> crate::WriteTo for TreeRef<'a> {
104108
})
105109
.sum()
106110
}
107-
108-
fn kind(&self) -> Kind {
109-
Kind::Tree
110-
}
111111
}
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
version https://git-lfs.github.com/spec/v1
2+
oid sha256:9d0ebc472023fffe49d71303e5f41f8c6ca24e28d283468f2426791c6286ecf7
3+
size 10240
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#!/bin/bash
2+
set -eu -o pipefail
3+
4+
function baseline() {
5+
local revspec=${1:?First argument is the revspec of the object to create a baseline for}
6+
local basename=${2:?Second argument is the name of the baseline file}
7+
local baseline_file="$basename.baseline"
8+
git rev-parse "$revspec" | git cat-file --batch | tail -n +2 > "$baseline_file"
9+
truncate -s "$(($(stat -c '%s' "$baseline_file")-1))" "$baseline_file"
10+
}
11+
12+
git init -q
13+
mkdir file
14+
touch bin bin.d file.to file.toml file.toml.bin file0 file/a
15+
16+
git add .
17+
git commit -m "c1"
18+
19+
baseline @^{tree} tree

gix-object/tests/tree/mod.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,29 @@ mod from_bytes {
130130
}
131131
}
132132

133+
mod entries {
134+
use gix_object::{Tree, TreeRef};
135+
136+
#[test]
137+
fn sort_order_is_correct() -> crate::Result {
138+
let root = gix_testtools::scripted_fixture_read_only("make_trees.sh")?;
139+
let input = std::fs::read(root.join("tree.baseline"))?;
140+
141+
let mut tree = TreeRef::from_bytes(&input)?;
142+
let expected = tree.entries.clone();
143+
144+
tree.entries.sort();
145+
assert_eq!(tree.entries, expected);
146+
147+
let mut tree: Tree = tree.into();
148+
let expected = tree.entries.clone();
149+
tree.entries.sort();
150+
151+
assert_eq!(tree.entries, expected);
152+
Ok(())
153+
}
154+
}
155+
133156
mod entry_mode {
134157
use gix_object::tree::EntryMode;
135158

0 commit comments

Comments
 (0)